Add out-of-tree Pyodide builds in CI for scikit-image#7350
Add out-of-tree Pyodide builds in CI for scikit-image#7350lagru merged 50 commits intoscikit-image:mainfrom
scikit-image#7350Conversation
Initial inspiration was taken from numpy/numpy#24603.
Locally, this version seems to successfully create a wheel when running "pyodide build" opposed to using "python -m build ...".
Based on numpy/numpy#25894. This comment updates versions for Pyodide, Emscripten, and improves some names for job steps. Multiple reusable actions have also been updated in accordance with the latest versions available. Co-Authored-By: Lars Grüter <[email protected]>
This commit loads and re-saves all of the `.npy` files to `.npz` NumPy compressed file format and saves them to the 'data' field in the Npz objects. The test cases where they have been used have been updated to use this attribute, and the SHA-256 checksums in the registry for each of the files have been updated in accordance with this change.
|
For context: a lot of the Some of the changes will make the tests with additional data being downloaded fail with |
|
Some progress: adding a JS script to run the |
agriyakhetarpal
force-pushed
the
add-emscripten-builds-out-of-tree
branch
from
ac6d92d to
12f0bd4
Compare
agriyakhetarpal
force-pushed
the
add-emscripten-builds-out-of-tree
branch
2 times, most recently
from
970d0a3 to
da1c1ed
Compare
|
More progress: I have finally got a green check (🎉) for the Emscripten job by skipping and fixing tests as necessary: https://github.com/scikit-image/scikit-image/actions/runs/8348607683/job/22850925405?pr=7350. I will now proceed to clean up this PR and fix the rest of the tests. It might require fixing the |
Try setting Agg backend for Matplotlib testing Don't mark Matplotlib tests, correct set backend
agriyakhetarpal
force-pushed
the
add-emscripten-builds-out-of-tree
branch
from
da1c1ed to
00fefee
Compare
Co-Authored-By: Lars Grüter <[email protected]>
|
The test warnings/failures have been fixed and are now passing, plus I cleaned up the workflow added. This should be ready for review again. I can rebase and rewrite history if needed, or let this be squash merged, both options would work. I'll follow these changes up by working towards #7350 (comment). |
lagru
left a comment
lagru
left a comment
There was a problem hiding this comment.
I've added some minor comments or questions but this looks ready to merge IMO. Thank you so much for leading this effort @agriyakhetarpal and for the impressively fast iteration cycle on your side! 👌
Co-Authored-By: Lars Grüter <[email protected]>
stefanv
left a comment
stefanv
left a comment
There was a problem hiding this comment.
This is great, thanks @agriyakhetarpal! I left some minor comments.
| contents: read # to fetch code (actions/checkout) | ||
|
|
||
| jobs: | ||
| build-wasm-emscripten: |
There was a problem hiding this comment.
Ideally, we'd store the built wheel as an artifact, grab it from the docs build, and upload it as part of the docs. @agriyakhetarpal it should be possible to mpip or similar from there?
|
|
||
| - name: Build scikit-image for Pyodide | ||
| run: | | ||
| pyodide build |
There was a problem hiding this comment.
This approach is totally fine, just wondering about the advantage of the manual vs cibuildwheel approach?
There was a problem hiding this comment.
I am happy to implement cibuildwheel, but it was very recently merged (pypa/cibuildwheel#1456) and there hasn't been a release yet. I did try it for NumPy (numpy/numpy#26570), pandas (pandas-dev/pandas#58647), and PyWavelets (PyWavelets/pywt#744), and all of them work – so if it would be okay to use using pypa/cibuildwheel@main for now and merge, I am happy to implement the changes as necessary.
Co-Authored-By: Stefan van der Walt <[email protected]>
This reverts commit 0bcf3c0.
|
Thanks @agriyakhetarpal! Long-term we probably want this to run only on |
|
Thank you very much @agriyakhetarpal! |
Thanks for the merge! Is there a reason why, though – I imagine there could be uncaught errors through PRs dealing with compiled code that may make it to In other words, I'm going to follow up with a PR right now that uploads these wheels to Anaconda.org, as we discussed above – with that, |
|
I guess there's a slight concern about the number of CPU cycles spun, but it's not a big deal. And it would be great to have this part of the devel docs workflow! |
…sting (#7525) This PR follows up on changes made in gh-7350. Here's a small, point-wise summary noting the changes here: - It updates the Emscripten/Pyodide CI job added in gh-7350 and removes the workaround JavaScript-based test suite file, since we fixed the `s_cmp` OpenBLAS unresolved symbol coming from SciPy upstream, having updated SciPy in-tree in Pyodide (see pyodide/pyodide#4719, pyodide/pyodide#5012, pyodide/pyodide#5031, and more). This should make the testing slightly cleaner than before, and the test suite now runs till the end without any Pyodide fatal errors being reported. The [Pyodide xbuildenv](https://github.com/pyodide/pyodide/releases/tag/0.27.0a2) can now be used to set up a virtual environment in which the tests can be run in the same way they were before. The [`pyodide-build` tool](https://github.com/pyodide/pyodide-build) has been unvendored from the Pyodide repository and now infers the Pyodide version from the releases, downloading the relevant files from the GitHub release. Hence, the `PYODIDE_VERSION` environment variable is no longer required. - There were some tests that have been failing on 32-bit platforms, which were last discussed quite a while ago: #3091. They surprisingly passed in a WASM 32-bit environment here without issues, which is great. I have updated the skipping conditions to accommodate this. - There's a known problem with the pytest bytecode generation when running the tests with the xbuildenv interpreter – I disabled the pytest internal cache plugin to fix that, but ignoring the warning (pypa/cibuildwheel#1966 (comment)) works equally as well. Please let me know if there is a preference. :) Additional context - Pyodide CI support was first discussed in gh-7265. - At the time of writing, Pyodide 0.27alpha2 is used in the CI job; once 0.27 stable comes out (should be soon) and `cibuildwheel` updates to it, that (plus this PR) will help unblock another PR that I had opened a while back: gh-7440. I can rebase that PR when ready. - pyodide/pyodide#4871 (comment) --- Co-authored-by: Lars Grüter <[email protected]>
4 participants
Description
This pull request is meant to supersede #7265 based on @lagru's suggestion, and it adds a CI job to build and test
scikit-imagein a Pyodide virtual environment through WASM wheels. Following this, it would be possible to include JupyterLite notebooks in the documentation in order to runscikit-image's code snippets which come with docstring-based examples.It is to be highlighted that this was completed last month for NumPy: (numpy/numpy#25894) and for PyWavelets – an optional dependency for
scikit-image: (PyWavelets/pywt#701), all thanks to @rgommers for the help.I was working on this last month, and I have rebased my branch to account for the latest changes upstream.
cc @stefanv from discussions on the previous PR.
Checklist
./doc/examplesfor new featuresRelease note
We use changelist to
compile each pull request into an item of the release notes. Please refer to
the instructions
and past release notes
for guidance and examples.