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

ci: add job to test with Emscripten #1272

Merged
merged 22 commits into from
Sep 12, 2024
Merged

ci: add job to test with Emscripten #1272

merged 22 commits into from
Sep 12, 2024

Conversation

ariostas
Copy link
Collaborator

@ariostas ariostas commented Aug 14, 2024

This PR resumes the work started in #1026. The package is installed and used with pyodide, just how it was done in #1026. I considered switching to using emscripten-forge and xeus-python since there are some significant advantages for deployment, but currently it is much easier to set up these tests with pyodide.

The main difficulty with the tests are network requests and thread spawning. Network issues don't end up being as bad for deployments since often files are hosted on the same server/website (and there's ways to get around CORS protections), so I'll see if I can get some very simple http test working. As for thread spawning, it's more of an issue since there are multiple functions that try to spawn threads without being explicit that they are doing so. Currently WASM doesn't support multiple threads, so various things don't work.

I hand-picked a few test files that work, but probably the best approach will be to introduce a new pytest marker to indicate which tests should be attempted. And maybe it would be good to decide which subset of the functionality should be available in WASM and write some simple tests for it to make it more explicit.

@ariostas
Copy link
Collaborator Author

ariostas commented Sep 3, 2024

I think this is starting to be in good shape, so it would be nice to get some feedback.

There are quite a few things that changed since my initial comment.

  • Instead of creating a Pyodide venv and running the normal tests, I switched to using pytest-pyodide, which uses selenium to run the Pyodide on an actual web browser so that it is fully-featured and realistic.
  • I made a few small changes in reading functions so that they don't spawn extra threads (which is not supported in Pyodide yet, but should be supported later on).
  • I added a tests-wasm folder for tests specific to wasm. I wrote a decorator so that we can also use files from skhep_testdata easily. For now, it only tests reading TTree and RNTuple files locally and over http.

@ariostas ariostas marked this pull request as ready for review September 3, 2024 20:35
@ariostas ariostas requested a review from jpivarski September 10, 2024 15:46
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! After merging this, I'll add "Test build / pyodide-build" to the list of required tests.

My only comment about the code: sys.platform == "emscripten" appears in multiple places; can this become a global boolean in _util.py? There aren't (currently) any examples in Uproot, but here are some examples of this in Awkward:

https://github.com/scikit-hep/awkward/blob/6b39e43ae538606e59e84797ba022da515b55218/src/awkward/_util.py#L16-L18

This is useful because there can sometimes be multiple ways to check which platform something is on (sys.platform versus os.name, maybe others), and some method may be found to be better in the future. Using a global boolean makes this a one-time fix.

I'm glad this is using Selenium and a separate tests-wasm directory. ✓

Please merge this PR when you're done and remind me to make the new test required if I forget. Thanks!

@ariostas
Copy link
Collaborator Author

Thank you Jim, that's a great suggestion.

I introduced uproot._util.wasm which is set to True when the platform is Emscripten or WASI. In general, I had to think a bit whether to name things with WASM, Emscripten, or Pyodide suffixes, but I think now it's reasonable.

  • uproot._util.wasm is for any WASM platform (Emscripten/WASI) since I would expect that the tweaks I made work everywhere. I'll test on xeus-python (Emscripten-based) at some point, but I'm not sure how to test things on WASI.
  • The extra dependencies test-pyodide are specific to Pyodide testing.
  • The tests-wasm directory for now only contains Pyodide tests, but I could imagine adding other platforms later and marking them appropriately.

@jpivarski jpivarski merged commit 1d5a17a into main Sep 12, 2024
25 checks passed
@jpivarski jpivarski deleted the ariostas/ci-emscripten branch September 12, 2024 19:55
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