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

ENH, CI: Add Emscripten to CI #21895

Merged
merged 4 commits into from Nov 15, 2022
Merged

ENH, CI: Add Emscripten to CI #21895

merged 4 commits into from Nov 15, 2022

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Jul 1, 2022

This is using a branch of Pyodide that finishes support for out of tree builds. This is blocked on Pyodide:

  • Merge changes from my branch into main Pyodide
  • Release Pyodide v0.21.0a3 (hopefully) with out of tree build support

Other than that, I think it is nearly ready.

@mattip
Copy link
Member

mattip commented Jul 1, 2022

The lint failures due to too-long-lines are OK, but could you clean up the too-many-blank-lines ones?

Any idea where the CI run of this is, I don't seem to see it in the summary.

@hoodmane
Copy link
Contributor Author

hoodmane commented Jul 1, 2022

I need to fix the trigger, right now it's only running on my fork not on the PR.

@hoodmane hoodmane force-pushed the emscripten-ci branch 2 times, most recently from 2eb664b to 75392a3 Compare July 1, 2022 21:35
@hoodmane hoodmane force-pushed the emscripten-ci branch 2 times, most recently from 2588f96 to 12c656f Compare July 2, 2022 01:18
@charris charris changed the title Add Emscripten to CI ENH, CI: Add Emscripten to CI Jul 5, 2022
emscripten/emscripten_runner.js Outdated Show resolved Hide resolved
emscripten/emscripten_runner.js Outdated Show resolved Hide resolved
.github/workflows/emscripten.yml Show resolved Hide resolved
hoodmane added a commit to pyodide/pyodide that referenced this pull request Jul 6, 2022
This completes the out of tree build CLI. This PR is paired up with:
numpy/numpy#21895
I have also successfully built scikit-learn, statsmodels, pandas, and
astropy with this.

The last thing we need to do after this is set up deployment of the
cross build environment. We can deploy one version to s3 for each
tagged commit. I will do that in a separate PR after this is merged.
@hoodmane hoodmane marked this pull request as ready for review July 14, 2022 08:31
@hoodmane
Copy link
Contributor Author

I updated this to use Pyodide v0.21.0. Would appreciate review.

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 @hoodmane. This is a large diff, but overall it looks very reasonable to me - pretty much everything seems to work, except for:

  • floating-point exceptions
  • starting a subprocess
  • threading
  • invoking a compiler
  • memmap

Those all make perfect sense. I assume that that's going to stay unchanged for quite a while?

.github/workflows/emscripten.yml Outdated Show resolved Hide resolved
.github/workflows/emscripten.yml Outdated Show resolved Hide resolved
.github/workflows/emscripten.yml Show resolved Hide resolved
emscripten/emscripten_runner.js Outdated Show resolved Hide resolved
numpy/core/tests/test_umath.py Outdated Show resolved Hide resolved
@rgommers
Copy link
Member

This does need a small rebase or merging in main to resolve the merge conflict.

@rgommers
Copy link
Member

I'm looking through the build log, and seeing that scipy and pythran are pulled in, plus setuptools-rust is built:

Successfully built setuptools-rust
Installing collected packages: ply, typing_extensions, setuptools, semantic-version, pycparser, numpy, gast, setuptools-rust, scipy, cffi, beniget, pythran
Successfully installed beniget-0.4.1 cffi-1.15.0 gast-0.5.3 numpy-1.23.1 ply-3.11 pycparser-2.21 pythran-0.11.0 scipy-1.8.1 semantic-version-2.10.0 setuptools-64.0.3 setuptools-rust-1.3.1.dev24+g5e8c380 typing_extensions-4.3.0

Doesn't seem like those should be needed?

@rgommers
Copy link
Member

No BLAS/LAPACK found, only:

INFO: numpy_linalg_lapack_lite:
INFO:   FOUND:
INFO:     language = c

Is that expected to remain the case? Not sure what the status is of porting a BLAS/LAPACK implementation to Pyodide.

@rgommers
Copy link
Member

All SIMD optimizations get disabled:

WARN: CCompilerOpt.__init__[1012] : unable to detect CPU architecture which lead to disable the optimization. check dist_info:<<
('emscripten_3_1_14_wasm32', 'cc', '-Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -O2 -g0 -fPIC -g2')
>>
WARN: CCompilerOpt.__init__[1028] : unable to detect compiler type which leads to treating it as GCC. this is a normal behavior if you're using gcc-like compiler such as MinGW or IBM/XLC.check dist_info:<<
('emscripten_3_1_14_wasm32', 'cc', '-Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -O2 -g0 -fPIC -g2')

I assume that that's fine for now - SSE instruction support is very spotty still (and anything else non-existent it looks like): https://emscripten.org/docs/porting/simd.html#compiling-simd-code-targeting-x86-sse-instruction-set

@rgommers
Copy link
Member

The build phase looks good. For the test phase, there's >500 lines of warnings under warnings summary. Would it be possible to get emscripten_runner.js or pytest to suppress that output?

@rgommers
Copy link
Member

I reviewed everything except emscripten_runner.js; I think we can rely on @hoodmane and @rth's expertise for that file. Overall this looks close to merge-ready. I don't think we had a discussion about whether we want this CI job (or did we?), but I'm +1. It's still a little experimental right now, but I'd say we can merge and see how it goes. In case there are a lot of issues (not too likely it seems, given the limited number of reasons for test skips) we can always reconsider.

@hoodmane
Copy link
Contributor Author

I'm looking through the build log, and seeing that scipy and pythran are pulled in, plus setuptools-rust is built:
...
Doesn't seem like those should be needed?

Currently the Pyodide build system has a bunch of hacks to get specific packages to work. Over time we should work on reducing them, but I am pretty willing to introduce hacks if they get things to work. I made a PR to remove the setuptools-rust dependency:
pyodide/pyodide#3045

@hoodmane
Copy link
Contributor Author

No BLAS/LAPACK found
...
Is that expected to remain the case? Not sure what the status is of porting a BLAS/LAPACK implementation to Pyodide.

We have a BLAS/LAPACK implementation based on CLAPACK (which is quite old: v3.2.1) + f2c applied to a few newer functions that scipy needs. We don't use it for numpy currently. I am not sure why, the decision not to use it in numpy predates me. Maybe @rth can explain.

In any case, it would add a bit more complexity to use it. Probably something to look into in the future. Again, it would be great to get a working fortran compiler and switch to LAPACK 3.10.

@hoodmane
Copy link
Contributor Author

The build phase looks good. For the test phase, there's >500 lines of warnings under warnings summary. Would it be possible to get emscripten_runner.js or pytest to suppress that output?

Mostly they are PendingDeprecationWarning. Do you have some standard way of suppressing them in the other tests?

@hoodmane
Copy link
Contributor Author

I reviewed everything except emscripten_runner.js

If we hold off on this a bit longer until the work in pyodide/pyodide#2976 is completed, we can remove this script and use the following:

python -m venv .venv-host
source .venv-host/bin/activate
pip install pyodide-build
pyodide venv .venv-pyodide
source .venv-pyodide/bin/activate
pip install -r test_requirements.txt
# Inside the .venv-pyodide, the following will run tests in Pyodide =)
python -b ./runtests.py -n -v --mode=full --durations 10 --coverage

I am planning to release Pyodide v0.22.0a1 as soon as we merge the command line runner stuff.

@rth
Copy link
Contributor

rth commented Aug 31, 2022

Is that expected to remain the case? Not sure what the status is of porting a BLAS/LAPACK implementation to Pyodide.

There is a plan to start using BLIS pyodide/pyodide#227 as it's probably easier to build for WASM than OpenBLAS. Not much progress for now, though I think it essentially works flame/blis#491 without SIMD support. Though of course, we are receptive if you have other suggestions about the BLAS we should use.

@rgommers
Copy link
Member

Re BLAS/LAPACK, thanks, that all makes sense. Performance isn't critical yet I guess - first make it all work. BLIS does seem like a good choice for BLAS, and it'll work fine with NumPy and SciPy. libFlame is still a work in progress though I believe, and not moving fast. So it's BLIS + Netlib LAPACK vs. OpenBLAS. Still makes sense to go for the former, OpenBLAS is pretty hairy internally.

If we hold off on this a bit longer until the work in pyodide/pyodide#2976 is completed, we can remove this script and use the following:

Great! I'd also be fine with merging this PR as is and removing the .js file once it's no longer needed - whatever you prefer.

Mostly they are PendingDeprecationWarning. Do you have some standard way of suppressing them in the other tests?

The warning filters in pytest.ini are supposed to take care of this. Also, pytest has command-line flags for this, see https://stackoverflow.com/questions/40710094/how-to-suppress-py-test-internal-deprecation-warnings

@rgommers
Copy link
Member

rgommers commented Oct 5, 2022

from this comment: If we hold off on this a bit longer until the work in pyodide/pyodide#2976 is completed, we can remove this script and use the following:

Pyodide 0.22.0a1 is now available and the linked PR merged. So maybe the script can be removed now?

@hoodmane
Copy link
Contributor Author

 Stack (most recent call first):
  <no Python frame>
ExitStatus {
  name: 'ExitStatus',
  message: 'Program terminated with exit(0)',
  status: 0
}
Error: Process completed with exit code 1.

So close...

@hoodmane
Copy link
Contributor Author

Okay I think this is ready to merge.

Copy link
Contributor

@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

There are some linting errors (lines too long) that should be easy to fix, see build log.

From the sideline (I am not a numpy maintainer), this is quite nice to see this moving forward again!

.github/workflows/emscripten.yml Show resolved Hide resolved
Copy link
Contributor

@rth rth 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, Hood! Great that you managed to make it work in the venv without the extra JS script.

The only thing I'm wondering about is whether it would be better to differentiate

The only unfortunate thing is @pytest.mark.xfailif doesn't exist.

As otherwise now it would show,

= 19491 passed, 2186 skipped, 1305 deselected, 37 xfailed, 2 xpassed in 370.63s (0:06:10) =

and in those 2k skipped tests it's a bit tedious to review what's skipped due to WASM and what to something else.

numpy/testing/_private/utils.py Outdated Show resolved Hide resolved
.github/workflows/emscripten.yml Show resolved Hide resolved
numpy/lib/tests/test_io.py Outdated Show resolved Hide resolved
numpy/lib/tests/test_format.py Outdated Show resolved Hide resolved
numpy/core/tests/test_umath.py Outdated Show resolved Hide resolved
numpy/core/tests/test_regression.py Outdated Show resolved Hide resolved
@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 11, 2022

xfail for things that are potentially fixable. For instance, the fp exception might be fixable, pyodide/pyodide#156 (comment) is probably outdated now, but it's still something about FP exception handling in musl unless you see a fundamental reason why it wouldn't work.

I think it's not a problem with musl floating point exceptions, but rather with wasm floating point exceptions. And the fact that they don't exist. Per the spec:

All operators use “non-stop” mode, and floating-point exceptions are not otherwise observable. In particular, neither alternate floating-point exception handling attributes nor operators on status flags are supported. There is no observable difference between quiet and signalling NaNs.

https://webassembly.github.io/spec/core/exec/numerics.html#floating-point-operations

My belief is that many of the floating point tests are unfixable without either software emulating floating point exceptions (this would be a large amount of work and the result would be very slow) or until the wasm spec and vms are updated to relax these limitations. The spec does say:

Note: Some of these limitations may be lifted in future versions of WebAssembly.

But note that there is no existing proposal to add fp exception support, so IMO such support is at least 3 years away and probably further.

@hoodmane
Copy link
Contributor Author

Thanks for the review @rth!

@hoodmane
Copy link
Contributor Author

The only unfortunate thing is @pytest.mark.xfailif doesn't exist.

I just looked this up and apparently @pytest.mark.xfail can be used this way:
https://docs.pytest.org/en/6.2.x/skipping.html#condition-parameter
But the reason I decided to skip all these tests was indeed because I didn't realize that @pytest.mark.xfail has a condition parameter.

See for example `build_test.yml` for the same snippet.
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 very clean now, it's all green, and it's a pretty fast CI job. So let's get this in. I pushed one change to ensure we use the same permission guard and concurrency settings as in other CI jobs.

Thanks a lot @hoodmane & all reviewers!

@rgommers rgommers merged commit a7abbee into numpy:main Nov 15, 2022
@rgommers rgommers added this to the 1.24.0 release milestone Nov 15, 2022
@hoodmane hoodmane deleted the emscripten-ci branch November 15, 2022 19:30
@rgommers
Copy link
Member

This job is now failing for the last 1-2 days because of:

Run actions/setup-python@v4
Version 3.10.2 was not found in the local cache
Error: Version 3.10.2 with arch x64 not found
The list of all available versions can be found here: https://raw.githubusercontent.com/actions/python-versions/main/versions-manifest.json

Checking the link, 3.10.2 for x86 is listed, so not sure what is up there. @hoodmane does this ring a bell? Is it safe to switch to a newer 3.10.x version?

@hoodmane
Copy link
Contributor Author

I think @ryanking13 fixed a similar issue on our repo. It should be fine to switch to a newer Python 3.10.x. After the switch to Python 3.11 we should maybe start doing patch updates...

@ryanking13
Copy link

ryanking13 commented Nov 25, 2022

This job is now failing for the last 1-2 days because of:

Run actions/setup-python@v4
Version 3.10.2 was not found in the local cache
Error: Version 3.10.2 with arch x64 not found
The list of all available versions can be found here: https://raw.githubusercontent.com/actions/python-versions/main/versions-manifest.json

Checking the link, 3.10.2 for x86 is listed, so not sure what is up there. @hoodmane does this ring a bell? Is it safe to switch to a newer 3.10.x version?

ubuntu22.04 (ubuntu-latest) doesn't have python 3.10.2 available with setup-python actions.

I think you can either use ubuntu20.04 or use newer python 3.10.x.

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.

None yet

7 participants