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, BLD, TST: Re-enable Emscripten/Pyodide CI job for NumPy #25894

Merged
merged 3 commits into from
Feb 29, 2024

Conversation

agriyakhetarpal
Copy link
Contributor

Description

This PR supersedes gh-24603. It adds a CI job to test NumPy v2.0.0.dev0 against a Pyodide (wasm32) runtime. Some of the key changes here are:

  1. A patch has been added to the ci/tools/emscripten/ directory based on an upstream change to Pyodide at Update numpy to 1.26.4 and don't set MESON env variable pyodide/pyodide#4502. This patch ensures that the correct Meson build system (i.e., vendored-meson) is found during the build process.
  2. A new requirements file has been created that contains pure-Python dependencies that can be installed inside a Pyodide virtual environment. It has been placed in the same directory as above.
  3. Some ancillary files have been added to ensure that the WASM wheel is compiled without specialised CPU instructions (SIMD, etc.) and that the build procedure does not need to build against BLAS or LAPACK, which are currently unavailable on WebAssembly.
  4. Various tests have been skipped, such as:
    • All f2py tests, since Fortran cannot run in WASM
    • Some tests for np.where() that require floating point exception support
    • Some tests related to the use of subprocesses to retrieve configurations for how NumPy has been built
    • A bug in Cython, i.e., [BUG] CythonCImports shadowing functionality can confuse pickle cython/cython#5411, has now been resolved. The import cython line was breaking the test discovery because Cython is not supported in Pyodide in-tree yet.
    • and so on

Thanks to @rgommers and the notes provided on gh-24603, all tests pass! Here's a workflow run from my fork where they can be observed: https://github.com/agriyakhetarpal/numpy/actions/runs/8065629246

`ninja` is unavailable for WASM for now, I have added a new requirements file for this and used a `sys_platform != 'emscripten'` platform marker to ignore the `ninja` dependency on WASM-based platforms.
This commit performs the following:

1. Skip `RuntimeWarnings` on exotic `np.where()` tests on WASM because of the lack of floating point exception support
2. Skip NumPy config tests that use subprocess module on WASM
3. Ignore threaded tests for PRNGs on WASM
4. Remove numpygh-5411 Cython `AttributeError` check. See cython/cython#5411, which is now resolved for Cython>3, and we are at Cython>=3.0.6.
4. For f2py, check compilers only if not on WASM
5. Skip pickle serialisation tests for `stringdtype` on WASM runtimes
@agriyakhetarpal
Copy link
Contributor Author

Pyodide build is passing here as well: https://github.com/numpy/numpy/actions/runs/8066720365

@agriyakhetarpal agriyakhetarpal changed the title CI, TST: Re-enable Emscripten/Pyodide CI job for NumPy CI, BLD, TST: Re-enable Emscripten/Pyodide CI job for NumPy Feb 27, 2024
@agriyakhetarpal
Copy link
Contributor Author

It looks like the build with nightly OpenBLAS wheels is failing – is there something I can do to help here? (cc: @mattip)

@charris
Copy link
Member

charris commented Feb 27, 2024

RE:Blas Is this based on the current tip of NumPy main?

@agriyakhetarpal
Copy link
Contributor Author

RE:Blas Is this based on the current tip of NumPy main?

Yes, this has been rebased on top of the main branch, IIUC and contains the latest changes.

@ngoldbaum
Copy link
Member

The BLAS failures are happening on every PR and are unrelated.

Copy link
Member

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

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

All the changes to tests make sense. I have a slight preference for marking tests with xfail(strict=True) for cases where we expect the test might pass in the future. I didn’t review the github actions changes but the job passes so probably close enough :)

meson_cpu/meson.build Outdated Show resolved Hide resolved
@agriyakhetarpal
Copy link
Contributor Author

All the changes to tests make sense. I have a slight preference for marking tests with xfail(strict=True) for cases where we expect the test might pass in the future.

I just checked; I have been mostly conforming to the pytest skip markers that were already present. Out of the tests, Fortran (f2py) support should be unlikely to come up anytime soon and can be skipped entirely, but the floating-point exception support could be marked with @pytest.mark.xfail(strict=True). However, I am not aware of aware of the developments or the effort required to make them possible on the Pyodide side of things.

Copy link

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Looks good to me for pyodide-build side. Thanks for working on this @agriyakhetarpal!

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.

This looks good, nice. A few more small comments/requests.

the floating-point exception support could be marked with @pytest.mark.xfail(strict=True)

I'd prefer not to do that. There are already lots of other identical skips for both floating-point errors and subprocesses, so keeping these the same is both nicer and less churn.

.github/workflows/emscripten.yml Show resolved Hide resolved
.github/workflows/emscripten.yml Outdated Show resolved Hide resolved
meson_cpu/meson.build Outdated Show resolved Hide resolved
meson_cpu/meson.build Outdated Show resolved Hide resolved
@agriyakhetarpal
Copy link
Contributor Author

I have resolved all the review comments that were posted; this should now be polished enough for another review, or for merging as and when needed.

This commit performs the following actions:

1. Adds WASM builds to the CPU family for Meson configurations, but
   without SSE or SIMD instructions.
2. Enables `IEEE_QUAD_LE` longdouble format for the wasm32 target
   (cross-builds).
3. Enables run for Emscripten/Pyodide wheels by setting the `if:`
   condition to `true`.
4. Uses recursive submodules to ensure that vendored-meson is received.
5. Moves the Meson cross file to `tools/ci/emscripten/` (i.e., creates a
   separate Emscripten folder to store relevant files)
6. Adds a patch for vendored-meson detection for Pyodide and applies
   this Pyodide-meson patch in the Emscripten CI jobs
7. Builds wasm32 wheels without BLAS and LAPACK support (see
   numpy#24750 (comment))
8. Forces coloured and prettified outputs for test runs

Some of these changes have been copied with updates and suggestions
received from numpy#24603 on 23/02/2024 and authorship is preserved with this
commit.

[skip cirrus] [skip circle] [skip azp]

Co-Authored-By: Ralf Gommers <[email protected]>
Co-authored-by: Hood Chatham <[email protected]>
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.

All looks good now. I did a slight cleanup of the commits that reverted something. In it goes. Nice work @agriyakhetarpal, and thanks for the reviews @hoodmane, @ryanking13 and @ngoldbaum.

@rgommers rgommers merged commit 3c4ee6b into numpy:main Feb 29, 2024
54 of 56 checks passed
@rgommers rgommers added this to the 2.0.0 release milestone Feb 29, 2024
@agriyakhetarpal agriyakhetarpal deleted the enable-emscripten-ci branch February 29, 2024 08:46
agriyakhetarpal added a commit to agriyakhetarpal/scikit-image that referenced this pull request Feb 29, 2024
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]>
agriyakhetarpal added a commit to agriyakhetarpal/scikit-image that referenced this pull request Feb 29, 2024
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]>
agriyakhetarpal added a commit to agriyakhetarpal/scikit-image that referenced this pull request Feb 29, 2024
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]>
agriyakhetarpal added a commit to agriyakhetarpal/scikit-image that referenced this pull request Feb 29, 2024
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]>
agriyakhetarpal added a commit to agriyakhetarpal/scikit-image that referenced this pull request Feb 29, 2024
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]>
agriyakhetarpal added a commit to agriyakhetarpal/scikit-image that referenced this pull request Mar 18, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants