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.

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

[SCSB-155] build with Numpy 2.0 #8527

Conversation

zacharyburnett
Copy link
Collaborator

@zacharyburnett zacharyburnett commented May 31, 2024

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 🤖

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

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • All comments are resolved
  • Make sure the JIRA ticket is resolved properly

Copy link

codecov bot commented May 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zacharyburnett zacharyburnett changed the title pin numpy>=2.0.0rc2 in build-system.requires [SCSB-155] build with Numpy 2.0 release candidate May 31, 2024
@zacharyburnett zacharyburnett marked this pull request as ready for review June 4, 2024 14:23
@zacharyburnett zacharyburnett requested a review from a team as a code owner June 4, 2024 14:23
@braingram
Copy link
Collaborator

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

@zacharyburnett
Copy link
Collaborator Author

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

Copy link
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

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?

@stscirij
Copy link
Contributor

stscirij commented Jun 6, 2024

Looks like the tests ran with numpy < 2.0

@braingram
Copy link
Collaborator

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: numpy/numpy#24300 (comment)

We might want to consider making a patch release with numpy<2 as I don't see this in the release branch:

"numpy>=1.22",

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.

pyproject.toml Outdated Show resolved Hide resolved
@hbushouse
Copy link
Collaborator

hbushouse commented Jun 6, 2024

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:

  • all the downstream packages have numpy 2.0 supporting versions (drizzle, stcal, spherical_geometry, stsci.stimage, opencv, etc...). The community packages are largely updated: Ecosystem compatibility with numpy 2.0 numpy/numpy#26191 I'm not so sure for the stsci ones.
  • we've tested again with numpy 2.0 (to be sure the numpy pin in our install requirements isn't a lie)

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

@zacharyburnett
Copy link
Collaborator Author

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
Copy link
Collaborator

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

@hbushouse
Copy link
Collaborator

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
Copy link
Collaborator

Oof, I see now that I edited #8527 (comment) 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
Copy link
Collaborator

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
Copy link
Collaborator

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

pyproject.toml Outdated Show resolved Hide resolved
@zacharyburnett zacharyburnett changed the title [SCSB-155] build with Numpy 2.0 release candidate [SCSB-155] build with Numpy 2.0 Jun 18, 2024
@braingram
Copy link
Collaborator

braingram commented Jun 25, 2024

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

@hbushouse hbushouse added this to the Build 11.1 milestone Jun 25, 2024
@zacharyburnett zacharyburnett enabled auto-merge (squash) July 26, 2024 19:21
@zacharyburnett zacharyburnett merged commit f97dc68 into spacetelescope:master Jul 26, 2024
25 of 33 checks passed
@zacharyburnett zacharyburnett deleted the pin_numpy_build_requirement branch July 29, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants