Skip to content

BUG: Fix build of third-party extensions with Py_LIMITED_API#20818

Merged
seberg merged 3 commits intonumpy:mainfrom
lpsinger:test-third-party-c-extensions-limited-api
Jan 14, 2022
Merged

BUG: Fix build of third-party extensions with Py_LIMITED_API#20818
seberg merged 3 commits intonumpy:mainfrom
lpsinger:test-third-party-c-extensions-limited-api

Conversation

@lpsinger
Copy link
Contributor

@lpsinger lpsinger commented Jan 13, 2022 •
edited
Loading

Fixes #13784.

This PR consists of two commits:

@lpsinger lpsinger force-pushed the test-third-party-c-extensions-limited-api branch from e495910 to 52b2da3 Compare January 13, 2022 18:28
lpsinger added a commit to lpsinger/ligo.skymap that referenced this pull request Jan 13, 2022
@lpsinger
Work around Numpy build regression
8653078 
Work around a regression in Numpy 1.22.0 that broke building third
party packages using the limited Python C API. See
[numpy#20818](numpy/numpy#20818).
seberg
seberg reviewed Jan 13, 2022
View reviewed changes
@lpsinger
BUG: Fix build of third-party extensions with Py_LIMITED_API
8b70732 
The type `vectorcallfunc` is not defined by Python's limited API.
Guard its use with a `#ifdef` so that third-party extensions that
use the Numpy C API will build with Py_LIMITED_API defined.

This fixes a regression introduced in numpy#20315.
@lpsinger
TST: Test building third party C extensions with Py_LIMITED_API
a61474e 
Fixes numpy#13784.
@lpsinger lpsinger force-pushed the test-third-party-c-extensions-limited-api branch from 52b2da3 to a61474e Compare January 13, 2022 23:55
@lpsinger lpsinger requested a review from seberg January 14, 2022 00:12
@seberg seberg added the 09 - Backport-Candidate PRs tagged should be backported label Jan 14, 2022
@seberg seberg added this to the 1.22.2 release milestone Jan 14, 2022
@seberg seberg changed the title TST: Test building third party C extensions with Py_LIMITED_API BUG: Fix build of third-party extensions with Py_LIMITED_API Jan 14, 2022
seberg
seberg approved these changes Jan 14, 2022
View reviewed changes
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

Not sure if we want to move the test somewhere else (I don't think it is run in CI?), but I do not mind if we don't get it. So it looks good as is, but maybe Matti has an idea for the test.

@seberg
Copy link
Member

seberg commented Jan 14, 2022

@lpsinger it looks good, however the debug tests are failing. I am not sure I understand why? I now realize that yes, this gets run as a test so that is all good, anyway, and I was just silly.

But this extension must not be compiled if we are running in debug mode... Although, I am not clear why this many tests are failing.

@lpsinger
Copy link
Contributor Author

lpsinger commented Jan 14, 2022

@lpsinger it looks good, however the debug tests are failing. I am not sure I understand why? I now realize that yes, this gets run as a test so that is all good, anyway, and I was just silly.

But this extension must not be compiled if we are running in debug mode... Although, I am not clear why this many tests are failing.

Understood. I'll split the example project into two projects so that we can build them in separate tests, and apply something like the skipif statement from https://github.com/numpy/numpy/blob/main/numpy/tests/test_public_api.py#L468-L475 to it.

@lpsinger lpsinger force-pushed the test-third-party-c-extensions-limited-api branch from 47f3b30 to 1edd640 Compare January 14, 2022 01:49
@lpsinger
Copy link
Contributor Author

lpsinger commented Jan 14, 2022

@lpsinger it looks good, however the debug tests are failing. I am not sure I understand why? I now realize that yes, this gets run as a test so that is all good, anyway, and I was just silly.
But this extension must not be compiled if we are running in debug mode... Although, I am not clear why this many tests are failing.

Understood. I'll split the example project into two projects so that we can build them in separate tests, and apply something like the skipif statement from https://github.com/numpy/numpy/blob/main/numpy/tests/test_public_api.py#L468-L475 to it.

Note, I found that on my Mac, Py_DEBUG was defined as 0, not None. https://github.com/numpy/numpy/blob/main/numpy/tests/test_public_api.py#L468-L475 may be buggy...

@lpsinger
Copy link
Contributor Author

lpsinger commented Jan 14, 2022

OK, the debug build is passing now.

@lpsinger lpsinger requested a review from seberg January 14, 2022 02:13
@mattip
Copy link
Member

mattip commented Jan 14, 2022

This looks great, and the test will help keep regressions from creeping in. A couple of nits:

  • Should the test be marked "slow" so people who run np.test() will not run it?
  • You might want take a look at the new extbuild function to build modules. It is used in the test_mem_policy tests. Maybe it is overkill for this use case?

@lpsinger
Copy link
Contributor Author

lpsinger commented Jan 14, 2022

  • Should the test be marked "slow" so people who run np.test() will not run it?

I don't think so. The old cython test that I adapted this from wasn't marked as slow. In any case, that could be a separate PR.

  • You might want take a look at the new extbuild function to build modules. It is used in the test_mem_policy tests. Maybe it is overkill for this use case?

Oh, very slick. I'll try that.

@lpsinger
Copy link
Contributor Author

lpsinger commented Jan 14, 2022

  • You might want take a look at the new extbuild function to build modules. It is used in the test_mem_policy tests. Maybe it is overkill for this use case?

Oh, very slick. I'll try that.

Unfortunately, numpy.testing.extbuild.build_and_import_extension won't work for this use case. There is no way to inject the #define early enough in the code that it generates, because the first line is always #include <Python.h> and it adds the prologue after that. Let's stick with the current implementation of the test, shall we?

@seberg
Copy link
Member

seberg commented Jan 14, 2022

Lets put it in than, thanks!

@seberg seberg merged commit f40b105 into numpy:main Jan 14, 2022
@lpsinger lpsinger deleted the test-third-party-c-extensions-limited-api branch January 14, 2022 21:55
@alalek alalek mentioned this pull request Jan 16, 2022
Closed
@charris charris mentioned this pull request Jan 17, 2022
Merged
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jan 17, 2022
lpsinger added a commit to lpsinger/oldest-supported-numpy that referenced this pull request May 27, 2022
@lpsinger
Use Numpy 1.22.2 instead of 1.22.0
ea22160 
Numpy 1.22.0 had a regression that prevented building packages
against the Python limited C API, that was fixed in Numpy 1.22.2.

See numpy/numpy#20818.
@lpsinger lpsinger mentioned this pull request May 27, 2022
Merged
rgommers added a commit to scipy/oldest-supported-numpy that referenced this pull request May 28, 2022
@lpsinger @rgommers
Use Numpy 1.22.2 instead of 1.22.0 (#58)
4aace24 
Numpy 1.22.0 had a regression that prevented building packages
against the Python limited C API, that was fixed in Numpy 1.22.2.

See numpy/numpy#20818.

Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
lpsinger added a commit to lpsinger/oldest-supported-numpy that referenced this pull request Jun 2, 2022
@lpsinger
Use Numpy 1.22.2 instead of 1.21.0--1.22.1
c5434c9 
This is a follow up to scipy#58. The Numpy regression fixed in numpy/numpy#20818
was introduced in Numpy 1.21.0, not 1.22.0. My mistake. Sorry!
@lpsinger lpsinger mentioned this pull request Jun 2, 2022
Closed
@mattip mattip mentioned this pull request Aug 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seberg seberg Awaiting requested review from seberg

Assignees

No one assigned

Labels

00 - Bug 05 - Testing

Projects

None yet

Milestone

1.22.2 release

Development

Successfully merging this pull request may close these issues.

TEST: make sure third parties can define Py_LIMITED_API

4 participants

@lpsinger @seberg @mattip @charris