-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add out-of-tree Pyodide builds in CI for scikit-image
#7350
base: main
Are you sure you want to change the base?
Changes from 30 commits
12a2aec
139c62d
7969f4a
1fec286
5cc0762
dc1e3bc
929dbcf
7a90748
0dc7431
01c61ba
360e48a
12f0bd4
b529673
2adf430
9afa53a
fa3154e
ee9dbc2
46fa541
2c00ec5
fe1777d
b55ae6f
24598b7
c995211
c587e5c
be525a4
f466193
e4cc188
37364b2
d02105b
88903e2
0b1c933
19159a4
174f3e7
1df1753
e94b1cd
cdd9830
06230bd
e177755
a6754d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
# Copied from NumPy https://github.com/numpy/numpy/pull/25894 | ||
# https://github.com/numpy/numpy/blob/d2d2c25fa81b47810f5cbd85ea6485eb3a3ffec3/.github/workflows/emscripten.yml | ||
# | ||
|
||
name: Test Emscripten/Pyodide build | ||
|
||
on: | ||
pull_request: | ||
branches: | ||
- main | ||
- maintenance/** | ||
# TODO: refine after this is ready to merge | ||
push: | ||
# branches: | ||
# - main | ||
# - maintenance/** | ||
workflow_dispatch: | ||
|
||
env: | ||
FORCE_COLOR: 3 | ||
|
||
concurrency: | ||
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} | ||
cancel-in-progress: true | ||
|
||
permissions: | ||
contents: read # to fetch code (actions/checkout) | ||
|
||
jobs: | ||
build-wasm-emscripten: | ||
name: Build scikit-image distribution for Pyodide | ||
runs-on: ubuntu-22.04 | ||
# To enable this workflow on a fork, comment out: | ||
# if: github.repository == 'scikit-image/scikit-image' | ||
env: | ||
PYODIDE_VERSION: 0.25.1 | ||
# PYTHON_VERSION and EMSCRIPTEN_VERSION are determined by PYODIDE_VERSION. | ||
# The appropriate versions can be found in the Pyodide repodata.json | ||
# "info" field, or in Makefile.envs: | ||
# https://github.com/pyodide/pyodide/blob/main/Makefile.envs#L2 | ||
PYTHON_VERSION: 3.11.3 | ||
EMSCRIPTEN_VERSION: 3.1.46 | ||
NODE_VERSION: 18 | ||
steps: | ||
- name: Checkout scikit-image | ||
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 | ||
|
||
- name: Set up Python ${{ env.PYTHON_VERSION }} | ||
id: setup-python | ||
uses: actions/setup-python@0a5c61591373683505ea898e09a3ea4f39ef2b9c # v5.0.0 | ||
with: | ||
python-version: ${{ env.PYTHON_VERSION }} | ||
|
||
- name: Set up Emscripten toolchain | ||
uses: mymindstorm/setup-emsdk@6ab9eb1bda2574c4ddb79809fc9247783eaf9021 # v14 | ||
with: | ||
version: ${{ env.EMSCRIPTEN_VERSION }} | ||
actions-cache-folder: emsdk-cache | ||
|
||
# We need to apply a patch to the Emscripten SDK to work around a bug in | ||
# the file system API. This patch is already applied in the Emscripten SDK | ||
# upstream, but we need to apply it here because we are using an older | ||
# version of the Emscripten SDK with Pyodide. | ||
- name: Apply necessary patch(es) | ||
run: | | ||
patch -d $EMSDK/upstream/emscripten/ -p1 < tools/emscripten/patches/0001-fix-npy-file-access.patch | ||
|
||
- name: Install pyodide-build | ||
run: pip install "pydantic<2" pyodide-build==${{ env.PYODIDE_VERSION }} | ||
|
||
- name: Build scikit-image for Pyodide | ||
run: | | ||
pyodide build | ||
|
||
- name: Set up Node.js | ||
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2 | ||
with: | ||
node-version: ${{ env.NODE_VERSION }} | ||
|
||
- name: Set up Pyodide virtual environment and test scikit-image for Pyodide | ||
env: | ||
# Avoid using the default TkAgg backend, which requires GUI support | ||
MPLBACKEND: "Agg" | ||
run: | | ||
pyodide venv .venv-pyodide | ||
source .venv-pyodide/bin/activate | ||
npm install pyodide@${{ env.PYODIDE_VERSION }} | ||
node tools/emscripten/pytest_scikit_image_pyodide.js --pyargs skimage |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
from .version_requirements import is_installed | ||
import sys | ||
import platform | ||
|
||
has_mpl = is_installed("matplotlib", ">=3.3") | ||
|
||
is_wasm = (sys.platform == "emscripten") or (platform.machine() in ["wasm32", "wasm64"]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,9 @@ def test_mask_border_keypoints(): | |
) | ||
def test_plot_matches(shapes): | ||
from matplotlib import pyplot as plt | ||
from matplotlib import use | ||
|
||
use('Agg') | ||
Comment on lines
+71
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I won't have as much context as @lagru here, but for what it's worth: in SciPy we set the backend to Agg for testing, and it's my understanding that that in general is a robust strategy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really follow why This is before you try to use Agg programmatically There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand it enough either (perhaps Node.js is not properly letting environment variables pass to the Python interpreter). But sure, let me try to revert this change in a separate commit, again, at the end of addressing the review and we can see if things break in an isolated state. Either way, keeping just one of these options would be better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have done a bit of reading about this as someone who does not have much JavaScript experience, and I think what is happening here is that Pyodide runs Python code within the browser environment (or in a different environment if a browser is not present), isolated from the Node.js process that spawns the Pyodide interpreter – therefore, it doesn't directly inherit environment variables from Node.js. |
||
|
||
fig, ax = plt.subplots(nrows=1, ncols=1) | ||
|
||
|
@@ -112,6 +115,9 @@ def test_plot_matches(shapes): | |
) | ||
def test_plot_matched_features(shapes): | ||
from matplotlib import pyplot as plt | ||
from matplotlib import use | ||
|
||
use('Agg') | ||
|
||
fig, ax = plt.subplots() | ||
|
||
|
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.
Right now this is only building and testing and not uploading the wheel somewhere, correct?
It's fine if we keep that out of scope for this PR but I'd still like to talk about how we might include the wheel in our doc building workflow?
We might also upload it as part of nightly wheel building workflow similar to numpy.
In case we decide tackle this, may we ping you for support? :D
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.
Yes, that is correct. The doc builds workflow(s) aspect is still something I am experimenting with right now, but it has been completed for SciPy (scipy/scipy#20019) and PyWavelets (PyWavelets/pywt#728). The SciPy one is disabled right now, but the PyWavelets one is available and can be accessed on the PyWavelets documentation website at any of the examples under https://pywavelets.readthedocs.io/en/latest/ref/index.html. The issue we have at hand here is how to provide these wheels pre-installed into a JupyterLite-compatible kernel through either a custom Pyodide distribution or through any other mechanism, which I am actively working towards.
Absolutely! I would be more than happy to make the changes myself in a follow-up PR, considering that this would be beneficial for the documentation builds mentioned above – the plan will be to use these wheels, after they have been built, but only in the latest versions of the docs (the stable versions may use a pinned wheel from the Pyodide CDN), however, this is preliminary and I suspect things will change. I hope I can trouble you for a review on another PR soon after this one ;)
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.
Perfect. Then let's get to that after merging this PR. :D