search

spacetelescope / jwst

Python library for science observations from the James Webb Space Telescope
https://jwst-pipeline.readthedocs.io/en/latest/
Other
570 stars 167 forks source link

[SCSB-155] build with Numpy 2.0 #8527

Closed zacharyburnett closed 3 months ago

zacharyburnett commented 5 months ago

resolves SCSB-155

Numpy 2.0 is scheduled for release on June 19th. Due to the C ABI updates, Python projects that build C extensions are required to build against Numpy 2.0 in order to use Numpy 2.0 in runtime. This is backwards-compatible with Numpy versions in runtime as far back as 1.26

this PR pins numpy>=2.0.0rc2 in build-system.requires

[!NOTE] this PR was generated automatically by batchpr :robot:

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 60.47%. Comparing base (d26cc5a) to head (69ae763).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #8527 +/- ## ========================================== + Coverage 59.93% 60.47% +0.53% ========================================== Files 372 369 -3 Lines 38330 38408 +78 ========================================== + Hits 22973 23226 +253 + Misses 15357 15182 -175 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

braingram commented 5 months ago

Would you run regression tests with this change with numpy 1.x?

zacharyburnett commented 5 months ago

Would you run regression tests with this change with numpy 1.x?

sure, here: https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FJWST-Developers-Pull-Requests/detail/JWST-Developers-Pull-Requests/1494/pipeline

stscirij commented 5 months ago

Looks like the tests ran with numpy < 2.0

braingram commented 5 months ago

So are we ready, willing, able to merge this now? We're OK with switching from numpy 1.x to 2.0? I'm actually surprised that the regtests did NOT show any differences. I would've expected at least small numerical differences. What am I missing?

The changes in this PR don't quite switch numpy to 2.0. As @stscirij noted the tests all ran with numpy 1.x (as expected) since many of the jwst dependencies do not yet have released versions that support numpy 2.0.

The main change in this PR is that this makes the jwst C extension compatible with both numpy 2.0 and numpy 1.x. This is related to the 5th item in the "Key guidance for users and downstream package authors" list here: https://github.com/numpy/numpy/issues/24300#issue-1828940995

We might want to consider making a patch release with numpy<2 as I don't see this in the release branch: https://github.com/spacetelescope/jwst/blob/f57f0b04cd93443ff36254ef389d98396544088f/pyproject.toml#L27 If this isn't pinned a user that installs the pipeline after the numpy 2 release may end up also installing numpy 2.0 and experience issues.

hbushouse commented 5 months ago

OK, I was confused by the line here: https://github.com/spacetelescope/jwst/pull/8527/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R147 which looks to me like it's requiring numpy >= 2.0. But I guess that's just for certain kinds of builds or something?

That entry is used primarily for pypi releases and local installs where the package is "built". The numpy docs cover several combinations: https://numpy.org/devdocs/dev/depending_on_numpy.html#numpy-2-0-specific-advice but the tldr; is this say "install numpy 2.0 when building this package". A user could build this with numpy 2.0 and run with numpy 1.x (if for example another dependency required numpy<2).

We could delay this PR until:

If we delay (or not) we should still consider pinning numpy<2 in the release branch and making a release with the pin.

zacharyburnett commented 5 months ago

I'm not so sure for the stsci ones.

I'm tracking Numpy 2.0 builds and testing here: https://github.com/orgs/spacetelescope/projects/45/views/2

I've only added SCSB ones so far within this "calibration pipeline" sphere of dependencies

  • we've tested again with numpy 2.0 (to be sure the numpy pin in our install requirements isn't a lie)

I'll go through the devdeps testing and make sure

hbushouse commented 5 months ago

What's the status and intent of this? Are stakeholders happy enough to merge it?

hbushouse commented 5 months ago

In line https://github.com/spacetelescope/jwst/pull/8527/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R27 should we have an upper limit pin on numpy in order to avoid using 2.0 yet?

braingram commented 5 months ago

Oof, I see now that I edited https://github.com/spacetelescope/jwst/pull/8527#issuecomment-2153100591 instead of "Quote reply" Sorry about that @hbushouse

In line https://github.com/spacetelescope/jwst/pull/8527/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R27 should we have an upper limit pin on numpy in order to avoid using 2.0 yet?

Pinning to "<2.0" in the release branch seems worth doing (as we haven't been able to test against numpy 2.0 due to dependency issues). Pinning in the main branch will make devdeps testing difficult.

I'm happy with the changes as they currently stand.

I just added the devdeps label so we can see the remaining failures (likely mostly from dependencies not having releases with numpy 2.0 support).

braingram commented 5 months ago

I just added the devdeps label so we can see the remaining failures (likely mostly from dependencies not having releases with numpy 2.0 support).

The label didn't trigger the jobs but perhaps the resync when the changelog is updated will trigger this.

braingram commented 5 months ago

This could use 2.0 (instead of rc) when it's updated.

braingram commented 4 months ago

Any idea why the docs failed? EDIT: I see, it hadn't yet picked up the numpy<2 pin