Build: Replace oldest-supported-numpy by pllim · Pull Request #14949 · astropy/astropy

pllim · 2023-06-14T21:19:37Z

Description

This pull request is to replace oldest-supported-numpy with numpy 1.25 that guarantee backward compatible C-API.

github-actions · 2023-06-14T21:20:16Z

github-actions · 2023-06-14T21:20:21Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

astrofrog · 2023-06-15T07:48:19Z

            "setuptools_scm>=6.2",
            "cython==0.29.34",
-            "oldest-supported-numpy",
+            "numpy==1.25",


Maybe this should be >=1.25

I wonder if this should perhaps also include <2 though I haven't had time to check if the backward compatibility is 'forever' or could break in 2.0

"A major release is highly unusual (NumPy is still at version 1.xx) and if it happens it will likely indicate an ABI break"

I think we should perhaps do >=1.25,<2

I thought someone once said that we should always do == pins on the build requirements? Isn't that why we do == for cython? Then why cython but not numpy?

Agreed about the >=1.25,<2.0 pin for Numpy,
For Cython, Numpy uses Cython>=0.29.34,<3.0 which seems better to me than pinning a specific version.

Hmmmpf, I guess you will have to compile it against our dev wheel, you don't even have to pass tests on it to do that.

The problem is, only if you use the 2.x wheel, can I provide updated headers to ensure compatibility (and force you to remove API that is I want to stop dragging around).

Sorry, somehow slipped me: Right now, NumPy doesn't actually enforce this! For precisely that reason. As long as we just remove things for the most part, I am happy to gamble that the wheels are effectively ABI compatible (even if that is a lie).

There might come a day where we want to do something that makes this impossible, but I am not sure that will come...
As soon as there is an actual NumPy rc1, that would be enforced though.

you will have to compile it against our dev wheel

I guess it is theoretically possible for our unreleased 6.0.dev. But we cannot do that for release branches; so does this mean we have to pin numpy maxversion for v5.3.x ? Though I also have no seen any failures related to this in our CI set up. My head hurts. 😆

Thanks for the info, @seberg ! I'll see what @astropy/astropy-project-release-team has to say since they are the ones doing releases. 🤞

Sorry, yes, just repeating in case it got lost: The NumPy dev wheels do not actually enforce this, they just pretend they are still 1.x ABI (it's a lie, but who cares). Maybe that will just stay that way, maybe I will find a big enough reason to break that "promise".

saimn · 2023-06-20T15:51:03Z

Ideally we should also use

#define NPY_TARGET_VERSION NPY_1_21_API_VERSION

but I don't think we have an easy way to do that right now with extension-helpers ...

saimn · 2023-06-20T15:54:27Z

Actually might be easy to do by looping on the result of get_extensions()
https://github.com/astropy/astropy/blob/main/setup.py#L69

pllim · 2023-06-20T16:05:11Z

@saimn , do you want to take over? You probably knows exactly what to do in the loop and stuff. Thanks!

Cadair · 2023-06-20T16:07:21Z

Actually might be easy to do by looping on the result of get_extensions() https://github.com/astropy/astropy/blob/main/setup.py#L69

This is nasty 😆

Extension helpers has the ability to drop the setup.py file completely now, so relying on logic in there is a bad idea. I don't have an alternative for you though so 🤷

saimn · 2023-06-20T17:18:04Z

Well there are various things for which a setup.py is still necessary, cython coverage, profiling, debugging...

astrofrog · 2023-06-20T18:16:17Z

Can't we just add an option in extension helpers to do this automatically for all extensions if it's going to be something common?

saimn · 2023-06-22T17:09:22Z

@astrofrog - Yes, I guess this could be done.

pllim · 2023-06-22T17:24:42Z

+# Specify the minimum version for the Numpy C-API
+for ext in ext_modules:
+    if ext.include_dirs and "numpy" in ext.include_dirs[0]:
+        ext.define_macros.append(("NPY_TARGET_VERSION", "NPY_1_21_API_VERSION"))


Does it crash if the extension code already has its own NPY_TARGET_VERSION somehow?

Also... so much for getting rid of setup.py lol

pllim · 2023-07-25T15:40:44Z

So... Are we keeping the stuff in setup.py here or no? I got lost in the discussions.

pllim · 2023-08-18T14:33:49Z

I am revisiting Python 3.12 related stuff. I realized that some upstream stuff with oldest-supported-numpy is giving us troubles if we have to build them from source. So this means we're affecting our downstream the same way if for whatever reason they cannot use our dev wheels (we don't have py312 wheels yet but I want to make it happen).

tl;dr -- Can I merge this???

astrofrog

Go forth and merge! I will think about how to avoid the setup.py code here, I think we can do it via extension-helpers. But this is fine for now.

with numpy 1.25 that guarantee backward compatible C-API. But we do not want exact pin. Specify the minimum version for the Numpy C-API Co-authored-by: Simon Conseil <[email protected]>

pllim · 2023-08-18T16:58:09Z

Let's run all the exotic stuff too, just in case.

neutrinoceros · 2023-08-18T16:59:13Z

            "setuptools_scm>=6.2",
            "cython==0.29.34",
-            "oldest-supported-numpy",
+            "numpy>=1.25,<2",


I would recommend adding a temporary version cap on the runtime dependency as well, see scipy/oldest-supported-numpy#76 (comment)

You mean install_requires? But then we won't be able to test against numpy dev in CI, no?

Yes I mean install_requires, and it won't be blocking for future-compatibility tests because 2.0.0dev0 compares as < 2.0 :)

So you are guarding against... numpy 3.0?

Nope, against 2.0

I guess I do not understand the logic of allowing 2.0.dev but preventing users from using the actual 2.0 later.

Thanks for your concern, which is valid. But I would like to discuss it separate from this one. Can you please propose a PR at your convenience?

I don't mind max pinning numpy in release branches, but I would rather not do it in the main branch. I think scipy only pin maxversion when they branch for release (at least the last I look and I don't look very often).

done at #15206 !

pllim added build no-changelog-entry-needed labels Jun 14, 2023

pllim added this to the v6.0 milestone Jun 14, 2023

pllim requested a review from a team June 14, 2023 21:19

pllim mentioned this pull request Jun 14, 2023

Drop oldest-supported-numpy from build requirements when numpy 1.25 is released #14915

Closed

github-actions Bot added the installation label Jun 14, 2023

astrofrog reviewed Jun 15, 2023

View reviewed changes

Cadair added the Build all wheels Run all the wheel builds rather than just a selection label Jun 15, 2023

pllim force-pushed the drop-oldest-supported-numpy branch from 4a0d0af to c7e3bf8 Compare June 20, 2023 14:45

pllim marked this pull request as ready for review June 20, 2023 15:20

pllim force-pushed the drop-oldest-supported-numpy branch from 0cd824f to 6fd7ebd Compare June 20, 2023 16:16

pllim commented Jun 22, 2023

View reviewed changes

pllim mentioned this pull request Jul 12, 2023

Implement basic build infrastructure and CI testing spacetelescope/pypolyclip#4

Merged

astrofrog approved these changes Aug 18, 2023

View reviewed changes

pllim force-pushed the drop-oldest-supported-numpy branch from c5e6804 to a208132 Compare August 18, 2023 14:59

pllim mentioned this pull request Aug 18, 2023

Stop using oldest-supported-numpy liberfa/pyerfa#107

Merged

This comment was marked as resolved.

Sign in to view

Build: Replace oldest-supported-numpy

6aba670

with numpy 1.25 that guarantee backward compatible C-API. But we do not want exact pin. Specify the minimum version for the Numpy C-API Co-authored-by: Simon Conseil <[email protected]>

pllim force-pushed the drop-oldest-supported-numpy branch from a208132 to 6aba670 Compare August 18, 2023 16:45

pllim mentioned this pull request Aug 18, 2023

TST: Test against Python 3.12 pre-prelease #14784

Merged

6 tasks done

pllim added the Extra CI Run cron CI as part of PR label Aug 18, 2023

neutrinoceros reviewed Aug 18, 2023

View reviewed changes

pllim merged commit ac21802 into astropy:main Aug 18, 2023

pllim deleted the drop-oldest-supported-numpy branch August 18, 2023 21:39

neutrinoceros mentioned this pull request Aug 19, 2023

BLD: set a temporary upper limit on runtime-requirement for numpy #15206

Closed

1 task

pllim mentioned this pull request Aug 29, 2023

MNT: Pin numpy<2 for v5.3.x #15234

Merged

1 task

WilliamJamieson mentioned this pull request Sep 19, 2023

Cannot build astropy in CI for exotic archs #15351

Closed

This was referenced Oct 14, 2023

Backport PR #15473: Ensure tables with masked and unmasked columns roundtrip properly (v5.0.x) #15481

Merged

MNT: Unpin numpy<2 in build requirements #15495

Closed

Conversation

pllim commented Jun 14, 2023

Description

Uh oh!

github-actions Bot commented Jun 14, 2023

Uh oh!

github-actions Bot commented Jun 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saimn commented Jun 20, 2023

Uh oh!

saimn commented Jun 20, 2023

Uh oh!

pllim commented Jun 20, 2023

Uh oh!

Cadair commented Jun 20, 2023

Uh oh!

saimn commented Jun 20, 2023

Uh oh!

astrofrog commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saimn commented Jun 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Jul 25, 2023

Uh oh!

pllim commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

pllim commented Aug 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

astrofrog commented Jun 20, 2023 •

edited

Loading

pllim commented Aug 18, 2023 •

edited

Loading