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

Fix build failure on Python 3.12 #1714

Merged
merged 8 commits into from
Jan 30, 2024

Conversation

jhunkeler
Copy link
Contributor

@jhunkeler jhunkeler commented Dec 14, 2023

Resolves N/A

Ref #1696 and #1702

This adds stregion to the requirements-dev.txt file to fix the build error on Python 3.12.

Checklist for maintainers

  • added entry in CHANGELOG.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

@jhunkeler jhunkeler requested review from mdlpstsci and a team as code owners December 14, 2023 15:28
mcara
mcara previously approved these changes Dec 14, 2023
@nden
Copy link
Collaborator

nden commented Dec 14, 2023

This failed for me locally as well, while building stregion locally from master passes. Not sure why this fails.

@s-goldman
Copy link
Collaborator

@jhunkeler Feel free to merge this if it is ready to merge.

@jhunkeler
Copy link
Contributor Author

jhunkeler commented Dec 14, 2023

Not ready to merge yet but I'm getting closer.

  1. tox's commands_pre scripts are not executed until after the editable build is finished. drizzlepac requires stregion to be installed, so that explains why it fails to build.

  2. requirements-dev.txt is incompatible with tox's requirement parser. The behavior suggests it tokenizes the first two words and drops everything else from the line. In response I'm simplifying the requirements file and moving those pip arguments into tox.ini under the devdeps target.

@jhunkeler
Copy link
Contributor Author

jhunkeler commented Dec 15, 2023

Just an FYI:
The requirements file as it is right now implies we want numpy-2.0 (numpy>=0.0.dev0). However, the ecosystem is too dirty and plenty of packages in the wild are preventing pip from selecting 2.0.0.dev0. In cases where I've managed to trick numpy to install, an endless stream of packages immediately fail at import-time due to compatibility issues with the 2.0 ABI.

It'll be a while before we can test against 2.0.

@nden
Copy link
Collaborator

nden commented Dec 15, 2023

Let's try with the latest numpy then. I want to see the deprecation warnings so we can fix them. Or is there a test already running with the latest numpy?

@jhunkeler
Copy link
Contributor Author

@nden At the moment the job is picking up numpy-1.26.2. Do want the tests to use the latest 1.2x development package (numpy<2.0.0.dev0) instead of the released version?

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ed40f61) 33.80% compared to head (0b6bed1) 31.14%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1714      +/-   ##
==========================================
- Coverage   33.80%   31.14%   -2.66%     
==========================================
  Files         126      159      +33     
  Lines       31157    35102    +3945     
  Branches     5710        0    -5710     
==========================================
+ Hits        10532    10933     +401     
- Misses      19439    24169    +4730     
+ Partials     1186        0    -1186     

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

@jhunkeler
Copy link
Contributor Author

jhunkeler commented Dec 15, 2023

Ah, never mind. There doesn't appear to be any dev packages for the 1.x release series.

Good news though. The -devdeps tests ran and 3 failed on Python 3.12 against astropy-6.1.dev.

#git+https://github.com/astropy/astroquery.git#egg=astroquery
--extra-index-url https://pypi.anaconda.org/astropy/simple astropy --pre

--extra-index-url https://pypi.anaconda.org/scientific-python-nightly-wheels/simple
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need the index-url to install the nightly builds of numpy and scipy?

Copy link
Contributor Author

@jhunkeler jhunkeler Jan 17, 2024

Choose a reason for hiding this comment

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

this

I think I managed to avoid putting these in the file by adjusting the index for the devdeps target. The extra arguments were causing tox's parser to fail. I'll double-check, though I think adding the extra index then asking for >=0.0.dev0 is roughly equivalent to calling pip install --extra-index-url https://index/here --pre.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the nightly builds for numpy. 2.x didn't work at all when I tried. Pip consistently reverted it back to 1.x because at least one or more packages elsewhere were pinning numpy<2.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see. Were these our packages or third party?

Copy link
Contributor Author

@jhunkeler jhunkeler Jan 17, 2024

Choose a reason for hiding this comment

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

I wish I'd kept the logs. I'll find out.
Meanwhile you helped me realize I'd forgotten the .dev0 suffix on the numpy line... So that's fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. contourpy 1.2.0 requires numpy<2.0,>=1.20, but you have numpy 2.0.0.dev0 which is incompatible.
matplotlib 3.8.2 requires numpy<2,>=1.21, but you have numpy 2.0.0.dev0 which is incompatible.
pandas 2.1.4 requires numpy<2,>=1.22.4; python_version < "3.11", but you have numpy 2.0.0.dev0 which is incompatible.
scikit-learn 1.3.2 requires numpy<2.0,>=1.17.3, but you have numpy 2.0.0.dev0 which is incompatible.

If I satisfy the requirements by installing the development versions for the packages above, pytest greets me with an endless wall of text telling me to rebuild all packages from source that haven't been linked to numpy 2.0, followed by a stream of ABI errors:

RuntimeError: module compiled against ABI version 0x1000009 but this version of numpy is 0x2000000

Drizzlepac has so many dependencies. Pip's incompatibility warnings only display packages with explicit pins (<2.0). It doesn't account for wheels providing packages with incompatible linkage.

--extra-index-url https://pypi.anaconda.org/scientific-python-nightly-wheels/simple
numpy>=0.0.dev0
#git+https://github.com/astropy/astroquery.git
numpy<2.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we test with numpy 2.0 dev?

nden
nden previously approved these changes Jan 22, 2024
Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

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

@jhunkeler Feel free to merge when you think it's ready

* Only applies to the latest interpreter. < 3.12 job use release requirements.
* Remove extraneous arguments (i.e. --extra-index-url, --pre) due to incompatibilities with tox
* Do not produce egg packages. Flat packages and wheels are preferred.
* Prevent the usage of numpy 2.0 for now
* Use "test" extras at install-time to avoid repeating dependencies in the config
* Remove pytest and ci_watson (already provided by "test" extras)
* Add setuptools to prevent pkg_resources error

from pkg_resources import get_distribution, DistributionNotFound

E   ModuleNotFoundError: No module named "pkg_resources"
* Use configure devdeps target to use development packages
* Remove commands_pre call. The actions are taken too late in the build process to be effective.
@jhunkeler
Copy link
Contributor Author

Here is a listing of all packages that fail to import when numpy 2.0 is installed in the devdeps tox environment. The ABI errors appear to manifest when dependencies not linked with 2.x are imported. For example, if astropy is linked to 2.x and it imports a package linked to 1.x, the error is thrown.

Script used: https://gist.github.com/jhunkeler/ee5513cb73eb96acac259ca1033df4e6

Command: check_numpy_abi.sh .tox/devdeps/ | sort

Output:

scanning 113 package(s) in [..]/.tox/devdeps for numpy ABI issues...
asdf_astropy    (0x1000009 != 0x2000000)
astrocut        (0x1000009 != 0x2000000)
astropy         (0x1000009 != 0x2000000)
astroquery      (0x1000009 != 0x2000000)
blosc2          (0x1000009 != 0x2000000)
crds            (0x1000009 != 0x2000000)
erfa            (0x1000009 != 0x2000000)
fitsblender     (0x1000009 != 0x2000000)
gwcs            (0x1000009 != 0x2000000)
matplotlib      (0x1000009 != 0x2000000)
numexpr         (0x1000009 != 0x2000000)
pandas          (0x1000009 != 0x2000000)
photutils       (0x1000009 != 0x2000000)
pyvo            (0x1000009 != 0x2000000)
skimage         (0x1000009 != 0x2000000)
sklearn         (0x1000009 != 0x2000000)
stregion        (0x1000009 != 0x2000000)
stwcs           (0x1000009 != 0x2000000)
tables          (0x1000009 != 0x2000000)
tables.libs     (0x1000009 != 0x2000000)
tweakwcs        (0x1000009 != 0x2000000)

@jhunkeler jhunkeler merged commit 18de0f5 into spacetelescope:master Jan 30, 2024
15 of 17 checks passed
mdlpstsci pushed a commit to mdlpstsci/drizzlepac that referenced this pull request Feb 21, 2024
Co-authored-by: Steve Goldman <32876747+s-goldman@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants