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

Build and test PyWavelets Pyodide wheels in CI #701

Merged
merged 30 commits into from
Feb 22, 2024

Conversation

agriyakhetarpal
Copy link
Collaborator

This PR adds and configures a CI job to test wasm32 wheels for Pywavelets compiled via the Emscripten toolchain. Almost all tests pass, and some of them are skipped because of the current lack of threading for Pyodide (please refer to pyodide/pyodide#237).

It is likely that Pyodide has issues with file system access which was evident through OSErrors when trying to access .npy files, it's likely that this is a bug and I shall file it accordingly in the Pyodide issue tracker. The current workaround that has been adopted in this PR is to save the arrays loaded from these .npy files to compressed .npz file formats, which do not cause issues related to file permissions.

An IS_WASM check has been added to check for the platform and allow pytest to skip the concurrency-based tests after collection, as noted above.

agriyakhetarpal and others added 28 commits February 20, 2024 18:51
Similar to commit 9941e05, I can revert both later on
@agriyakhetarpal
Copy link
Collaborator Author

Pyodide wheel builds are succeeding and tests are passing on my fork, see here: https://github.com/agriyakhetarpal/pywt/actions/runs/8003758318

@agriyakhetarpal
Copy link
Collaborator Author

agriyakhetarpal commented Feb 22, 2024

Okay, so importlib.resources.files() is available on Python 3.9 and later we already support Python 3.9 and later, we might have to revert this change or look at other ways to access the data folder without the use of __file__, or reintroduce it and check if this was one of the issues that caused test failures earlier. Should be addressed by 7315b43.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @agriyakhetarpal, this looks good! Nice to have a Pyodide CI job that works.

Beyond the one inline comment, I thought I'd ask if you want to rewrite the commit history (e.g., one commit for the CI job, one for the npy->npz, one for the test changes, and one for the other unrelated bits) or if you want me to squash-merge it. Either way is fine with me.

pywt/tests/test_mra.py Outdated Show resolved Hide resolved
@agriyakhetarpal
Copy link
Collaborator Author

Thanks for the prompt review, @rgommers! It did take a bit of back and forth with the experiments – I don't have a strong opinion on rewriting history as such; it would be great if we can squash merge the PR, it's much easier

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Very nice!

 ============================= test session starts ==============================
platform emscripten -- Python 3.11.3, pytest-8.0.1, pluggy-1.4.0
...
 ================= 1030 passed, 6 skipped in 115.83s (0:01:55) ==================

Okay, in it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants