Fix OpenBLAS s_cmp unresolved symbol error, update Emscripten CI testing#7525
Conversation
|
The Pyodide CI job is passing, as expected. This is ready for review, thanks! |
lagru
left a comment
There was a problem hiding this comment.
Thanks for updating! I gave it a first pass.
.github/workflows/emscripten.yml
Outdated
| build-wasm-emscripten: | ||
| name: Build scikit-image distribution for Pyodide | ||
| runs-on: ubuntu-22.04 | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Since we are running this on pull requests I wouldn't mind restricting the environment somewhat. It's confusing for contributors and annoying for maintainers if a workflow breaks across many PRs if a random update in the CI breaks stuff. It also makes it harder to pinpoint the culprit.
What do you think about pinning the image, and more so the Pyodide version again?
There was a problem hiding this comment.
That's a reasonable suggestion, thanks! I'll pin both the image and the Pyodide xbuildenv version here.
There was a problem hiding this comment.
bd6c352 keeps the image pinned to ubuntu-22.04 (I tried bumping to ubuntu-24.04 but it is in beta right now and might not be stable – it didn't have the required Python 3.12).
dbbb817 pins the Pyodide version. It's pinned to the 0.27.0alpha2 release, but I don't think we will be pushing any code in this release cycle that will be concerning for scikit-image 😁 So, we can either wait until 0.27 stable is out, or we can go ahead with this and I'll keep it in mind to bump this to v0.27-stable as soon as we release it :)
There was a problem hiding this comment.
Thanks. With regards to merging now or waiting, whatever you prefer. :)
stefanv
left a comment
There was a problem hiding this comment.
This looks great, thanks @agriyakhetarpal
lagru
left a comment
There was a problem hiding this comment.
Looks good now! I think I'll merge this now. We can bump the pyodide easily in a later PR. :)
Description
This PR follows up on changes made in gh-7350. Here's a small, point-wise summary noting the changes here:
scikit-image#7350 and removes the workaround JavaScript-based test suite file, since we fixed thes_cmpOpenBLAS unresolved symbol coming from SciPy upstream, having updated SciPy in-tree in Pyodide (see SciPy v1.13.0 pyodide/pyodide#4719, SciPy v1.14.0 pyodide/pyodide#5012, SciPy v1.14.1 pyodide/pyodide#5031, and more). This should make the testing slightly cleaner than before, and the test suite now runs till the end without any Pyodide fatal errors being reported.The Pyodide xbuildenv can now be used to set up a virtual environment in which the tests can be run in the same way they were before.
The
pyodide-buildtool has been unvendored from the Pyodide repository and now infers the Pyodide version from the releases, downloading the relevant files from the GitHub release. Hence, thePYODIDE_VERSIONenvironment variable is no longer required.Additional context
cibuildwheelupdates to it, that (plus this PR) will help unblock another PR that I had opened a while back: Usecibuildwheelto build WASM/Pyodide wheels forscikit-image, push nightlies to Anaconda.org #7440. I can rebase that PR when ready.Checklist
Docstrings for all functionsA gallery example in./doc/examplesfor new featuresRelease note
For maintainers and optionally contributors, please refer to the instructions on how to document this PR for the release notes.