-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Conversation
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. |
I need to fix the trigger, right now it's only running on my fork not on the PR. |
2eb664b
to
75392a3
Compare
2588f96
to
12c656f
Compare
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.
167731f
to
10500f3
Compare
I updated this to use Pyodide v0.21.0. Would appreciate review. |
There was a problem hiding this 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?
This does need a small rebase or merging in |
I'm looking through the build log, and seeing that
Doesn't seem like those should be needed? |
No BLAS/LAPACK found, only:
Is that expected to remain the case? Not sure what the status is of porting a BLAS/LAPACK implementation to Pyodide. |
All SIMD optimizations get disabled:
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 |
The build phase looks good. For the test phase, there's >500 lines of warnings under |
I reviewed everything except |
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 |
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. |
Mostly they are |
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. |
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. |
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.
Great! I'd also be fine with merging this PR as is and removing the
The warning filters in |
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... |
dadc573
to
9761a56
Compare
Okay I think this is ready to merge. |
There was a problem hiding this 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!
There was a problem hiding this 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
- skiped test that cannot be fixed in WASM, e.g. subprocess will never work
- xfail for things that are potentially fixable. For instance, the fp exception might be fixable, Issues with exception raising after setting numpy.errstate 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.
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.
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:
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:
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. |
Thanks for the review @rth! |
I just looked this up and apparently |
26f25da
to
fddb8a3
Compare
See for example `build_test.yml` for the same snippet.
There was a problem hiding this 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!
This job is now failing for the last 1-2 days because of:
Checking the link, |
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... |
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. |
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.
This is using a branch of Pyodide that finishes support for out of tree builds. This is blocked on Pyodide:
Other than that, I think it is nearly ready.