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

Add out-of-tree Pyodide builds in CI for numcodecs #529

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

agriyakhetarpal
Copy link

@agriyakhetarpal agriyakhetarpal commented May 21, 2024

This PR adds a CI job (emscripten-ci.yaml) that builds WASM wheels for numcodecs for use in Pyodide-based environments using the Emscripten toolchain. This was discussed in one of the community meetings for Zarr where it was said that this change would be welcome as a pull request, and follow-up work after this is complete is planned:

  1. A job that publishes nighty WASM wheels for numcodecs to a PyPI-like distribution channel (not sure where, this might require requesting access on the https://github.com/scientific-python/upload-nightly-action repository (which corresponds to https://anaconda.org/scientific-python-nightly-wheels/)
  2. A corresponding job for out-of-tree testing for Zarr that builds up on this job (and subsequently its nightly pushes)
  3. Interactive documentation improvements with Sphinx that make use of these nightly wheels in some form (longer-term goal, soon but not very soon I guess)

TODO:

  • Unit tests and/or doctests in docstrings
  • Tests pass locally
  • Docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • Docs build locally
  • GitHub Actions CI passes
  • Test coverage to 100% (Codecov passes)

Additional context

Some xrefs for reviewers and project maintainers:

@agriyakhetarpal agriyakhetarpal changed the title Add out-of-tree Pyodide wheels in CI for numcodecs Add out-of-tree builds for Pyodide wheels in CI for numcodecs May 21, 2024
@agriyakhetarpal agriyakhetarpal changed the title Add out-of-tree builds for Pyodide wheels in CI for numcodecs Add out-of-tree Pyodide builds in CI for numcodecs May 21, 2024
@agriyakhetarpal
Copy link
Author

The workflow is currently going to fail because the file system access in Emscripten has a bug with saving .npy files – I had started on this PR roughly two weeks back, and I have now rebased on top of the changes from the main branch. Here is the related issue that I opened on the Pyodide repository a few hours ago: pyodide/pyodide#4779

@rgommers and I had fixed a similar error in PyWavelets/pywt#701 by converting all of the .npy files present to .npz files. If this is a suitable workaround at this time and does not render other PRs un-mergeable, please let me know and I would be happy to apply this fix here (it works/worked for loading .npz files, it should most likely work for saving them as well).

@joshmoore
Copy link
Member

Thanks, @agriyakhetarpal. I've launched the workflows.

@agriyakhetarpal
Copy link
Author

Ah, I think the miniconda action needs updating. I'll push a commit to fix that in a moment.

@agriyakhetarpal
Copy link
Author

Never mind, looks like #528 is already on it – I can wait and rebase later after that gets merged. For the relevant jobs here, the Pyodide wheel build succeeds while the tests fail because of the reasons described above, and the rest of the test suite passes.

@agriyakhetarpal
Copy link
Author

agriyakhetarpal commented May 22, 2024

Marking this PR as a draft till there is a fix for this on the Pyodide and Emscripten ends. I will put my focus on an accompanying CI job and associated fixes for the Zarr repository.

@agriyakhetarpal
Copy link
Author

It's been a while since this PR has been opened, could we get the ball rolling by temporarily skipping the previously failing test for now? I feel that numcodecs's CI can benefit from adding this workflow for the rest of the test suite, at least. I've revisited the merge conflicts in the previous few commits and updated the workflow file in 764f34e, I'll see how the builds go.

@agriyakhetarpal
Copy link
Author

I've created pyodide/pyodide#5169 and pyodide/pyodide#5172 (please see above) for adding some missing dependencies to Pyodide. As a temporary measure, I've skipped the sole test that requires an updated version of the crc32c package: numcodecs/tests/test_checksum32.py::test_crc32c_checksum in 53f2096. I'll mark this PR as ready for review for now. @joshmoore, could you please trigger the workflows for me again?

@agriyakhetarpal
Copy link
Author

https://github.com/agriyakhetarpal/numcodecs/actions/runs/11746955955 via my fork is the first passing workflow run, with a few caveats:

  1. A (one) crc32c test is temporarily disabled (see above, will be available in the next Pyodide release)
  2. zfpy is not available yet (see above, will be available in the next Pyodide release)
  3. Zarr v3 tests are temporarily disabled until we upgrade to Zarr v3 in Pyodide, hopefully by the next release
  4. I had to skip a doctest that was using threads in a messy manner, but it should work.

The rest of the skips/xfails are the usual WASM limitations where the multiprocessing/threading modules are not available, and we have a pretty significant portion of the test suite passing without issues. I've compiled crc32c and zfpy with Emscripten locally in the time being and triggered the test suite for numcodecs with them, which didn't reveal anything problematic. I'm open to including them in the CI job temporarily if needed – they don't take too long to compile.

@agriyakhetarpal agriyakhetarpal marked this pull request as ready for review November 8, 2024 17:59
Copy link
Author

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Update from my previous comment above: I've cleaned up and merged the latest changes from the main branch, and agriyakhetarpal/numcodecs/actions/runs/12572995519/job/35045552245 via my fork passes all tests. We released https://github.com/pyodide/pyodide/releases/tag/0.27.0 yesterday, and I've bumped to it here.

Overall, this is the result from the test suite: 786 passed, 49 skipped, 36 xfailed in 23.22s, which does seem quite promising.

Gentle ping, @joshmoore!

Copy link
Author

Choose a reason for hiding this comment

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

I am also happy to upstream this patch (and the one below) to Blosc if it is suggested to do so. :)

Comment on lines +87 to +89
# Install the built numcodecs WASM wheel and relevant dependencies
pip install $(ls dist/*.whl)"[msgpack,crc32c,test,test_extras]"
# TODO: get zfpy built in Pyodide and install it here
Copy link
Author

Choose a reason for hiding this comment

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

zfpy has been built in Pyodide, but against NumPy v2 (as described above). There is a chance that the tests should work with NumPy >=2, too if I were to install it separately instead of using the [zfpy] extra. I'm open to test that out if asked.

Comment on lines +7 to +10
on:
# TODO: refine after this is ready to merge
[push, pull_request, workflow_dispatch]

Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
on:
# TODO: refine after this is ready to merge
[push, pull_request, workflow_dispatch]
on: [push, pull_request]
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

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