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

REL: use numpy pre-releases from PyPI instead of nightlies in wheel building workflow #143

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

neutrinoceros
Copy link
Contributor

No description provided.

@neutrinoceros
Copy link
Contributor Author

neutrinoceros commented Mar 31, 2024

failures on MacOS ARM seem unrelated, let me try to update CIBW too

@neutrinoceros
Copy link
Contributor Author

Ah, it's not used directly here, but through https://github.com/OpenAstronomy/github-actions-workflows/
Then let me see if it's actually related before I try to fix it upstream.

@neutrinoceros
Copy link
Contributor Author

I found where this problem is already being discussed upstream: actions/runner-images#9256 (comment)
It's an issue with the docker image used in GitHub Actions, not cibuildwheels. I think it's in good hands so I suggest we just wait a couple days for now.

@neutrinoceros
Copy link
Contributor Author

(this is probably not a blocker to this PR actually, but it could be blocking for a release)

@mhvk
Copy link
Contributor

mhvk commented Apr 1, 2024

I'm not quite sure this is quite right: what we want is to have our wheels be done with numpy 2.0, which is done .github/workflows/publish.yml -- those are currently set to use numpy-dev (so in fact should be OK!) and I think should now become numpy >=2.0.rc1.

In principle, there is no reason to restrict the general build to numpy >= 2.0 - after all, people can build things fine with numpy 1.25 or whatever, it just won't work under higher versions.

@neutrinoceros
Copy link
Contributor Author

See my answer in astropy/astropy#16252 (comment)

But, I indeed missed the specifics of this project's infra. Should we keep building wheels against numpy nightlies ? This seems needlessly risky to me now there's an ABI stable version we can use instead. I seems to me that the following patch would suffice

diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml
index 84d9566..30463f0 100644
--- a/.github/workflows/publish.yml
+++ b/.github/workflows/publish.yml
@@ -29,7 +29,7 @@ jobs:
       # the build isolation and explicitly install the latest developer version of numpy as well as
       # the latest stable versions of all other build-time dependencies.
       env: |
-        CIBW_BEFORE_BUILD: '${{ ((github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' || github.event_name == 'pull_request') && 'pip install --pre --extra-index-url https://pypi.anaconda.org/scientific-python-nightly-wheels/simple setuptools setuptools_scm jinja2 numpy') || '' }}'
+        CIBW_BEFORE_BUILD: '${{ ((github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' || github.event_name == 'pull_request') && 'pip install setuptools setuptools_scm jinja2 numpy>=2.0.0rc1') || '' }}'
         CIBW_BUILD_FRONTEND: '${{ ((github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' || github.event_name == 'pull_request') && 'pip; args: --no-build-isolation') || 'build' }}'
 
       test_extras: test

what do you think ?

@mhvk
Copy link
Contributor

mhvk commented Apr 1, 2024

Indeed, we should patch the publish workflow. With that, it is not clear to me what is the advantage of also doing pyproject.toml - doesn't it mean that nobody can just do pip install . with an older version of numpy?

@neutrinoceros
Copy link
Contributor Author

Unless passed the --no-build-isolation flag, pip completely ignores whatever's installed in the environment and will instead build an isolated throwaway env just for the purpose of building. So, if you're using this argument, yes, it'll throw an error if you have have an older numpy, but otherwise nothing visible is changed from a developer's perspective.

@mhvk
Copy link
Contributor

mhvk commented Apr 1, 2024

Ah, I see. I still do not quite see why we would do more than the minimal change to our publish script: once numpy 2.0 is actually out, that will be picked up anyway by default by pip, and before that there would seem to be little point to force using a release candidate for anything other than our wheels.

@neutrinoceros neutrinoceros changed the title BLD: build with numpy 2.0.0rc1 (or newer) REL: use numpy pre-releases from PyPI instead of nightlies in wheel building workflow Apr 1, 2024
@neutrinoceros
Copy link
Contributor Author

Just wanted to make sure we were on the same page before I pushed again. I've reverted the change to pyproject.toml

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

@neutrinoceros - this certainly looks good to me (and we can do pyproject.toml separately if that is wanted after all).

Before I merge: do I understand correctly that the wheel building also failed on its own already? I.e., the macos failure is unrelated to this PR, but rather an upstream issue in the process of being resolved, right? Did you get a sense of timescale? I.e., should I still wait a bit with creating a new version for pypi?

@neutrinoceros
Copy link
Contributor Author

That is correct. Upstream issue has been for two months but the current, more visible form of the problem only seems to have appeared a couple days ago. Maybe it'll get fixed promptly, and otherwise, it seems plausible that cibuildwheel could issue a patch to work around it, but I'm not taking bets. I'd advise to wait for a couple days and if nothing changes in the meantime, just push a release without the macOS AMD wheels; they are much less critical for testing and can always be pushed later when the situation improves.
Rest assured that I'll keep you updated as this affects my personal workflow :)

@mhvk
Copy link
Contributor

mhvk commented Apr 1, 2024

OK, thanks! Will merge this one now then.

@mhvk mhvk merged commit ed48e7e into liberfa:main Apr 1, 2024
22 of 23 checks passed
@neutrinoceros neutrinoceros deleted the bld/build_with_numpy2.0.0rc1 branch April 1, 2024 21:13
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.

2 participants