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: reintroduce compatibility shim for compiling against numpy 1.x #147

Merged
merged 2 commits into from
Apr 11, 2024
Merged

BLD: reintroduce compatibility shim for compiling against numpy 1.x #147

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b758b5
TST: add a CI job to check building against numpy 1.x
neutrinoceros Apr 11, 2024
1ef2e23
BLD: reintroduce compatibility shim for compiling against numpy 1.x
neutrinoceros Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions .github/workflows/ci_workflows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,35 @@ jobs:
python -m pip install --editable .[test]
(nm -u erfa/ufunc.*.so | grep eraA2af) || exit 1
(python -c 'import erfa' 2>&1 | grep -n 'too old') > /dev/null && (echo 'liberfa too old, skipping tests'; exit 0) || python -m pytest


build_against_numpy_1:
runs-on: ubuntu-latest

steps:
- name: Checkout code
uses: actions/checkout@v4
with:
fetch-depth: 0
submodules: true
- name: Set up Python
uses: actions/setup-python@v5
with:
# this is mainly meant to be useful on old or exotic archs
# so we use our oldest-supported Python
python-version: '3.9'
- name: Install build-time dependencies
run: |
python -m pip install --upgrade wheel setuptools setuptools_scm jinja2 'numpy<2.0'
- run: python -m pip list
- name: Build
# using --no-build-isolation allows us to skip pass build-system metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

It still seems to me we're papering over the main problem, that this all comes about because we have the requirement of numpy>=2.0.0rc1 in pyproject.toml. At least, it looks to me that, as is, a pip install pyerfa on the astropy side for s390 etc will still force installation of numpy 2.0 to produce the pyerfa binaries and thus fail (unless we add --no-build-isolation there too, but why should astropy have to worry about this?).

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess to put it a bit more constructively, I think this test should verify that pip install pyerfa is possible on a system that does not have numpy 2.0 (and cannot build it either).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way I see it, it's only a trade-off for where we want things to be simple and where we accept that they get more complicated.
Having numpy>=2.0.0rc1 gives a behavior that's almost always desirable, and crucially, doesn't prevent working with numpy 1.x when needed. Whereas not having it moves the complexity to something much more crucial: the releasing process. I personally feel it's preferable to put the extra workload on rare archs.

Copy link
Contributor

Choose a reason for hiding this comment

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

My problem is that we're making problems on rare archs outside of our own package - I dislike that astropy has to work around a choice of convenience made in pyerfa.

I also generally dislike that we effectively state falsehoods: numpy>=2.0.0.rc1 is in fact not required to build pyerfa; it is just something that's there because it can help create a package that is useful in more circumstances (which for a general pip install is likely irrelevant).

Anyway, this is slightly orthogonal to the issue at hand, which really just reflects that pip is not a great package manager (and that creating a good package manager is incredibly hard). Let's first of all make sure astropy can build at all!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's first of all make sure astropy can build at all!

To be clear, astropy has no build issue. What's broken is building pyerfa on rare arch's without numpy 2, and it happens that this is only tested in the astropy repo

Copy link
Contributor

Choose a reason for hiding this comment

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

What's broken is building pyerfa on rare arch's without numpy 2

Indeed, and my suggestion was mostly meant to ensure that on the astropy side one doesn't have to give any special flags to get pyerfa to build properly when numpy 2.0 is not available.

But obviously without reintroducing the *elsize* stuff that will not be possible at all, so let's get that in first!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ready when you are :)

# from pyproject.toml
# In particular this allows us to use an older version of numpy than we
# normally require.
# building in --editable mode to avoid PYTHONPATH issues
run: |
python -m pip install --editable . --no-build-isolation

- name: Check
run: python -c "import erfa"
Loading