Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BLD: fix building against numpy dev #140

Merged
merged 1 commit into from
Mar 8, 2024
Merged

BLD: fix building against numpy dev #140

merged 1 commit into from
Mar 8, 2024

Conversation

neutrinoceros
Copy link
Contributor

xref numpy/numpy#25943

This should fix the error seen in lastest nightly builds

@neutrinoceros neutrinoceros force-pushed the bld/numpy_2_compat/drop_dtype_elsize branch 4 times, most recently from f0da0c6 to 5250b4b Compare March 8, 2024 09:01
@neutrinoceros
Copy link
Contributor Author

If I understand the upstream change correctly, there is no C API that works both in numpy 1 and numpy 2 here. In other words, my patch fixes building against numpy 2 but breaks building against numpy 1. In the long game, this isn't necessarily an issue since building against numpy 2 is supposed to be the one way to produce backward compatible artifacts, so we should soon require numpy>=2.0 at build time anyway, but in the interval between now and the actual release (candidate), there's the question of how we should handle it in CI.

It should be possible (and relatively easy) to use pre-processor directives to preserve build time compat with numpy 1, so I'll try that.

@neutrinoceros neutrinoceros force-pushed the bld/numpy_2_compat/drop_dtype_elsize branch from f7356d7 to 5250b4b Compare March 8, 2024 10:01
@neutrinoceros
Copy link
Contributor Author

I've made a couple unsuccessful attempts at keeping build-time compat with all both versions but now I feel I don't actually understand how it's supposed to work (if at all).
From my standpoint, it looks like the API that's available at runtime depends on the installed version of numpy, but we also can't build a universal binary, regardless the version of numpy we use at compile-time, since it would require invoking undefined symbols (i.e., dtype->elsize is undefined in numpy 2, and PyDataType_ELSIZE is undefined in numpy 1).
I must be missing something here, since the natural conclusion from here would be that we can't build backward compatible binaries, in direct contradiction to the big promise in numpy 1.25 and 2.0.

@neutrinoceros
Copy link
Contributor Author

@seberg your input would be invaluable here 😅

@seberg
Copy link

seberg commented Mar 8, 2024

You should add:

// Backported NumPy 2 API (can be removed if numpy 2 is required)
#if NPY_ABI_VERSION < 0x02000000
#define PyDataType_ELSIZE(descr) ((descr)->elsize)
#endif

since you don't want to break compiling with NumPy 1.x, but otherwise looks good.

seberg
seberg reviewed Mar 8, 2024
View reviewed changes
erfa/ufunc.c.templ Outdated
@@ -376,7 +376,7 @@ ufunc_loop_matches(PyUFuncObject *self,
*/
if (types[i] == NPY_VOID && dtypes != NULL) {
int op_descr_type_num = op_descr->type_num;
int dtype_elsize = dtypes[i]->elsize;
int dtype_elsize = PyDataType_ELSIZE(dtypes[i]);
Copy link

Choose a reason for hiding this comment

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

Suggested change
int dtype_elsize = PyDataType_ELSIZE(dtypes[i]);
npy_intp dtype_elsize = PyDataType_ELSIZE(dtypes[i]);

Probably also still wrong in NumPy :). Not that it matters in practice, I don't think NumPy actually allows that large voids yet. Just setting the stage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course ! I lost this bit while cleaning the branch, thanks for catching my mistake !

@neutrinoceros
Copy link
Contributor Author

Thanks a lot @seberg !

@neutrinoceros neutrinoceros force-pushed the bld/numpy_2_compat/drop_dtype_elsize branch from 99ae2fe to ecae457 Compare March 8, 2024 10:33
@neutrinoceros neutrinoceros marked this pull request as ready for review March 8, 2024 10:33
neutrinoceros
neutrinoceros commented Mar 8, 2024
View reviewed changes
pyproject.toml Outdated
@@ -3,6 +3,6 @@ requires = [
"setuptools",
"setuptools_scm>=6.2",
"jinja2>=2.10.3",
"numpy"
"numpy>=1.25",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #122 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's now an actual requirement since my patch is using a constant that's not defined in earlier versions.

Copy link

Choose a reason for hiding this comment

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

I do think that NPY_ABI_VERSION was always defined (if it's not I should change the pattern).

FWIW, I never quite understood what is the advantage/disavantage, but overall it shouldn't matter much? For wheel builds you want it, but it's not technically a strict requirement for building. But if someone builds with --no-build-isolation it is ignored anyway...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to remove it if that's what @mhvk prefers

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it out - I'd like to introduce a minimum version only if it is actually needed.

@pllim
Copy link
Contributor

pllim commented Mar 8, 2024

Thanks, all! Would be nice if you could trigger nightly build after merged via workflow_dispatch. 🙏

pyerfa/.github/workflows/publish.yml

Line 7 in 7847a2f

workflow_dispatch:

mhvk
mhvk reviewed Mar 8, 2024
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Thanks, @neutrinoceros! This looks good and happy that the solution from @seberg works! Only, let's not set a minimum version unless it causes failures otherwise.

pyproject.toml Outdated
@@ -3,6 +3,6 @@ requires = [
"setuptools",
"setuptools_scm>=6.2",
"jinja2>=2.10.3",
"numpy"
"numpy>=1.25",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it out - I'd like to introduce a minimum version only if it is actually needed.

@neutrinoceros neutrinoceros force-pushed the bld/numpy_2_compat/drop_dtype_elsize branch from ecae457 to bff2fee Compare March 8, 2024 16:13
@neutrinoceros
Copy link
Contributor Author

done !

mhvk
mhvk approved these changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Super, thanks!

@mhvk mhvk merged commit 4a637c2 into liberfa:main Mar 8, 2024
23 checks passed
@neutrinoceros neutrinoceros deleted the bld/numpy_2_compat/drop_dtype_elsize branch March 8, 2024 16:51
@neutrinoceros
Copy link
Contributor Author

@mhvk can you re-trigger the publish workflow manually so that it actually uploads and hopefully fix astropy devdeps too ?

@mhvk
Copy link
Contributor

mhvk commented Mar 8, 2024

@mhvk can you re-trigger the publish workflow manually so that it actually uploads and hopefully fix astropy devdeps too ?

Not sure I know how to do that, cc @astrofrog...

(in the worst case, we make a new point release...)

@pllim
Copy link
Contributor

pllim commented Mar 8, 2024

Please go to https://github.com/liberfa/pyerfa/actions/workflows/publish.yml and you should see an option (I don't see it here because I have no write access). It looks like this:

Screenshot 2024-03-08 132439

@pllim
Copy link
Contributor

pllim commented Mar 8, 2024

Once you have done that, a new entry would appear here:

https://github.com/liberfa/pyerfa/actions/workflows/publish.yml?query=event%3Aworkflow_dispatch

@mhvk
Copy link
Contributor

mhvk commented Mar 8, 2024

OK, thanks, I've done that and something is afoot!

@pllim
Copy link
Contributor

pllim commented Mar 8, 2024

Upload step is green. Thanks!

@pllim
Copy link
Contributor

pllim commented Mar 8, 2024

For completeness, astropy devdeps job is green again. Thanks, all!

https://github.com/astropy/astropy/actions/runs/8205309742

@mhvk
Copy link
Contributor

mhvk commented Mar 8, 2024

Thanks, @neutrinoceros and @pllim! So nice if all I really have to do is to press a button (and even get explained which button to press).

This was referenced Apr 11, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seberg seberg seberg left review comments

@avalentino avalentino avalentino approved these changes

@mhvk mhvk mhvk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@neutrinoceros @seberg @pllim @mhvk @avalentino