Skip to content

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

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

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

Conversation

agriyakhetarpal
Copy link
Contributor

@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
Contributor 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
Contributor Author

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

@agriyakhetarpal
Copy link
Contributor 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
Contributor 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
Contributor 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
Contributor Author

We've cut a 0.27.3 release today, which bundles pcodec; all tests pass on my fork: https://github.com/agriyakhetarpal/numcodecs/actions/runs/13545498292?pr=529

@agriyakhetarpal agriyakhetarpal marked this pull request as ready for review February 26, 2025 14:17
@agriyakhetarpal
Copy link
Contributor Author

It looks like #569 is going to change a few things here – I have to note that Pyodide will need to set the provisioned NUMCODECS_USE_SYSTEM_LIBS variable to OFF, as we cross-compile from Linux to wasm32-emscripten and two patches are being applied to Blosc, for which we have to build it from the vendored sources.

I'm still happy to discuss with upstream maintainers if they're interested in such patches as I mentioned above in this PR, and I assume it helps both them and numcodecs that the patches aren't big at all.

@joshmoore
Copy link
Member

(Relaunched)

@agriyakhetarpal
Copy link
Contributor Author

Thanks, I fixed the drop in the coverage – it's just that Codecov doesn't know about this WASM environment setup, so it can't be aware that this code path is being tested elsewhere.

@joshmoore
Copy link
Member

Re-running jobs after:

Exception: Request failed after too many retries. URL: https://ingest.codecov.io/upload/github/zarr-developers::::numcodecs/upload-coverage
[PYI-4825:ERROR] Failed to execute script 'main' due to unhandled exception!

@dstansby dstansby added this to the 0.16.0 milestone Mar 4, 2025
@dstansby dstansby removed this from the 0.16.0 milestone Apr 7, 2025
@agriyakhetarpal
Copy link
Contributor Author

I've addressed the merge conflicts introduced in the release notes file at the time of the 0.16 release. Thanks!

I wanted to note that there is a recent PR at #737, which also concerns the wasm32-unknown-emscripten target. The reason why the Pyodide CI job here can bypass that and work despite those changes is because we drop a curated list of unsupported arguments when cross-compiling. However, #737 will be a helpful change indeed.

@agriyakhetarpal
Copy link
Contributor Author

Hi! I've resolved the new merge conflicts that had sprung up here. Also, I made sure that we're testing against the same Pyodide version as we're building the WASM wheels for. If you'd like me to, I can also try against the latest version of Pyodide to check for any regressions (which will use Emscripten 4.0.9 instead of 3.1.58) – cibuildwheel v3 soon will help doing this (i.e., testing against nightly or alpha versions of Pyodide), and a beta (3.0.0b1) is already out for testing.

Additionally, I enabled Zarr v3 as a dependency (it uses threads which won't work in WASM, so perhaps it makes sense to not add it or skip any new tests that will fail due to it). zfpy is also available in Pyodide 0.27.6 (which I've updated to, including a bump to Node 22), so that helps us not skip those tests. I had added a WASM build for pcodec and enabled it some time earlier.

@agriyakhetarpal
Copy link
Contributor Author

@agriyakhetarpal
Copy link
Contributor Author

The sister PR to accompany this PR, which adds Pyodide support for Zarr, is now also ready for review (marked above). Thank you!

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.

3 participants