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: make sure pyodide works without setup #1868

Merged
merged 2 commits into from
Jun 11, 2024
Merged

fix: make sure pyodide works without setup #1868

merged 2 commits into from
Jun 11, 2024

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Jun 10, 2024

Currently we are not always getting a version of Python that supports the pyodide platform in the action. This makes sure we do that.

Edit: This is not actually correct, the problem with or without this is cibuildwheel is looking for and can't find 3.12, even though it is running on 3.12. Running a setup-python (that updates the environment) before this fixes the problem. Maybe our discovery needs to be widened to include sys.executable?

@henryiii henryiii marked this pull request as draft June 10, 2024 16:46
@mayeut
Copy link
Member

mayeut commented Jun 10, 2024

We could:

@henryiii
Copy link
Contributor Author

I like a) for now, and then maybe work on c) longer term. Possibly d), but I like the much higher performance of not pulling a docker image if it's not needed.

@henryiii
Copy link
Contributor Author

Fixes the issue here: scikit-hep/boost-histogram#935

@henryiii henryiii marked this pull request as ready for review June 10, 2024 20:02
@henryiii henryiii requested a review from mayeut June 11, 2024 04:33
python-version: "3.8 - 3.12"
python-version: "3.12"
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to the PR but I was thinking that maybe, cibuildwheel could follow NEP 29 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's now superseded by SPEC 0. Is there a reason to? I personally prefer official EoL, though for cibuildwheel, it really doesn't matter as much, since CI or developers should have a recent Python; it's not user-facing. Any particular reason for being interested in following NEP 29/SPEC 0, 3.10+?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the link, I somehow missed the prominent note in NEP 29...

Is there a reason to?

Nothing else than it allows to get newer CPython features faster and, given the specific nature of cibuildwheel which you mentioned, I thought it might be interesting to follow the schedule of SPEC 0.

python 3.10 might not change much (mostly some typing, functools.cache). With Python 3.11+ at the end of the year we could drop all things in _compat and remove those conditionnal dependencies.

I might open a discussion about this at some point.

@mayeut
Copy link
Member

mayeut commented Jun 11, 2024

I like a) for now, and then maybe work on c) longer term. Possibly d), but I like the much higher performance of not pulling a docker image if it's not needed.

a) seems good enough for now.

Personally, I'd rather see d) than c) because it would allow local builds. I'm not worried at all about pulling an image (it should have a much much lower footprint than manylinux/musllinux images). I'm more worried about the image provenance/maintenance and the overhead by cibuildwheel copying into the container at the start (and maybe things like build cache).

@mayeut mayeut merged commit 8d5d84e into main Jun 11, 2024
22 of 24 checks passed
@mayeut mayeut deleted the henryiii-patch-1 branch June 11, 2024 06:48
@henryiii
Copy link
Contributor Author

If the image already had emscripten, etc, then it probably would be fine performance wise, and would work better on macOS. I wonder if we could fix the pytest warning (which becomes an error if you have warnings as errors) about the source directory not being writable for the cache folder if we used a container?

@henryiii
Copy link
Contributor Author

I'd like to make a release soon with this fix, so that users setting up the new Pyodide support don't have to add the extra setup. Anything else that needs to go in?

@mayeut
Copy link
Member

mayeut commented Jun 11, 2024

Anything else that needs to go in?

I think we're good. Just saw a doc fix pop-up that might need to go in.

@mayeut
Copy link
Member

mayeut commented Jun 11, 2024

On second thought, maybe we should wait for pip 24.1 ? I think the pip team wanted to get the release by June 17th.
There can still be another release of cibuildwheel once it lands of course.

@henryiii
Copy link
Contributor Author

henryiii commented Jun 11, 2024

Looks like b2 is coming soon, maybe we could at least wait for that. Patch releases should be pretty cheap, but this isn't too horribly inconvenient either, just one extra action is required.

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