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

WIP: CI: switch Emscripten job to Meson #24603

Closed
wants to merge 1 commit into from

Conversation

rgommers
Copy link
Member

This is one of the last CI jobs to be converted to Meson. Still a work in progress.

Note that we need to cross-compile here. Using pyodide build fails on not having a cross file. The source() function in pyodide_build/cli/build.py needs a --backend-flags parameter so that we can pass config-settings arguments to the backend. In the meantime, we use python -m build ....

One can also try this locally with:

$ python -m build -wnx -Csetup-args=--cross-file=\$PWD/tools/ci/emscripten.meson.cross  -Csetup-args=-Dallow-noblas=true

as long as both pyodide-build and the exact version of Emscripten (currently 3.1.32, can be installed with https://emscripten.org/docs/tools_reference/emsdk.html) are available. The configure stage of the build looks fine, the build itself fails halfway through due to:

include/python3.11/pyport.h:601:2: error: "LONG_BIT definition appears wrong for platform (bad gcc/glibc config?)."
#error "LONG_BIT definition appears wrong for platform (bad gcc/glibc config?)."

That issue is due to a static assert in pyconfig.h, also discussed at: pyodide/pyodide#2494 (comment)

See also pyodide/pyodide#2238 and the patches to _numpyconfig.h in https://github.com/pyodide/pyodide/tree/main/packages/numpy.

tl;dr this needs some work, and it seems like cross-compilation is hard because Pyodide doesn't ship its own Python interpreter that we can actually target during a direct cross build.

Cc @hoodmane @rth. I hope I can bother you with this. If I got any of the above wrong or if you've got a suggestion to get past the LONG_BIT error, I'd love to hear it. Also, a couple of questions:

  • If I didn't overlook a way to pass --config-settings flags to the backend in the pyodide build CLI, would you accept a PR to Pyodide to add that? It should match backend-flags in meta.yaml recipes I think.
  • Since scikit-image can be cross-compiled, I had expected to be able to make that work for numpy too. But after reading this comment, that may not be true? Is that still the case, and do you see a road to a regular cross-compile working for NumPy? It's actually not clear to me how the distutils-based build works right now, it doesn't seem to be doing the "playback" thing explained in that comment.

@rgommers rgommers marked this pull request as draft August 31, 2023 15:13
endian = 'little'

[properties]
longdouble_format = 'IEEE_QUAD_LE'
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only addition compared to the upstream emscripten.meson.cross file in Pyodide - necessary for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Most of this file can be removed, only this is needed:

[properties]
longdouble_format = 'IEEE_QUAD_LE'

@hoodmane
Copy link
Contributor

cc @ryanking13 @henryiii

@rgommers rgommers added this to the 2.0.0 release milestone Sep 9, 2023
@rgommers
Copy link
Member Author

I decided that it's easier to finish this only after upgrading the build of numpy that Pyodide does in-tree to Meson and 1.26.x first. I've made a start on that, but I need to do some more digging. There was also a pip bug that made that a little more difficult. And pyodide/pyodide#4118 is the same LONG_BIT error as I'm seeing in this PR.

Next steps I plan to take (not necessarily in this order):

  • disable this CI job and remove all setup.py files
  • finish unvendoring meson-python from this repo after the next meson-python release
  • upgrade numpy in Pyodide to 1.26.0 + Meson
  • if needed, ensure the out-of-tree Pyodide build can take the same backend-flags as the in-tree build
  • re-enable this CI job on the main branch

@rth
Copy link
Contributor

rth commented Sep 11, 2023

Thanks for working on this @rgommers ! I don't have useful suggestions about the errors you are facing but I agree it would probably be easier to do this in the in-tree build first. Also cc @lesteve who worked on the scikit-image meson build.

If I didn't overlook a way to pass --config-settings flags to the backend in the pyodide build CLI, would you accept a PR to Pyodide to add that?

I think overall we could be happy to accept any changes needed to make numpy with meson work. But since pyodide build aims to be a drop-in replacement for python -m build, we need to double check in particular whether this would still be OK with cibuildwheel, or if there is some other recommended way of passing those config settings.

@rgommers
Copy link
Member Author

But since pyodide build aims to be a drop-in replacement for python -m build, we need to double check in particular whether this would still be OK with cibuildwheel, or if there is some other recommended way of passing those config settings.

That should be fine for cibuildwheel I think. python -m build -Ckey=value will add key=value to the config_settings dict and then pass that on to the backend. -C/--config-setting is implemented in build, and -C/--config-settings in pip (note the extra s for pip in the long form). I am missing that flag from pyodide build --help.

@ryanking13
Copy link

That should be fine for cibuildwheel I think. python -m build -Ckey=value will add key=value to the config_settings dict and then pass that on to the backend. -C/--config-setting is implemented in build, and -C/--config-settings in pip (note the extra s for pip in the long form). I am missing that flag from pyodide build --help.

Currently pyodide build passes extra command line arguments to config-settings (backend-flags), so pyodide build key1=val1 is equivalent to python -m build -C key1==val1 (it is not documented sorry). But yes, we should change it to -C/--config-settings to match the behavior of pypabuild and pip.

@rgommers
Copy link
Member Author

Thank you @ryanking13, that's exactly what I needed.

lagru added a commit to scikit-image/scikit-image that referenced this pull request Dec 11, 2023
@rgommers rgommers modified the milestones: 2.0.0 release, 2.1.0 release Jan 31, 2024
hoodmane referenced this pull request Feb 9, 2024
Moving it to Meson is going to take a bit of time, and in the meantime
we have to disable it, because it's the only job that still needs
the `setup.py` based build and we'd like to remove support for building
with `setup.py`.
@hoodmane
Copy link
Contributor

hoodmane commented Feb 9, 2024

@ryanking13 Do you know what the status of this is? It would be good if we could look into this again. I would look into it myself but so far I don't know much about meson...

@rgommers
Copy link
Member Author

rgommers commented Feb 9, 2024

I've been meaning to get back to this PR. I believe with @ryanking13's changes for improved Meson/meson-python support in Pyodide, this should be unblocked. Timing wise I realistically won't revisit it before the NumPy 2.0 release settles down though (any week now ...).

@ryanking13
Copy link

Yes, now that we're building numpy with meson in-tree, I don't think it's going to be hard to make it work in out-of-tree too. We've recently added a patch for python-meson >= 0.15 so I think we need to release pyodide-build (0.25.1).

before the NumPy 2.0 release settles down

Maybe emscripten CI should also wait for numpy 2.0 to be released? I've seen Pyodide builds break in many cases when packages do major releases, so it might be better to wait and see if the numpy 2.0 build works in-tree before applying it.

@rgommers
Copy link
Member Author

rgommers commented Feb 11, 2024

Maybe emscripten CI should also wait for numpy 2.0 to be released? I've seen Pyodide builds break in many cases when packages do major releases, so it might be better to wait and see if the numpy 2.0 build works in-tree before applying it.

That seems reasonable to me. EDIT: no real reason not to do it now though, it's just not a priority compared to 2.0-critical tasks.

Copy link
Member Author

@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.

I added some comments to this file about what is still relevant. @agriyakhetarpal is going to have a fresh attempt at a new/updated CI job.

@@ -52,14 +52,23 @@ jobs:
actions-cache-folder: emsdk-cache

- name: Install pyodide-build
run: pip install "pydantic<2" pyodide-build==$PYODIDE_VERSION
run: pip install "pydantic<2" build pyodide-build==$PYODIDE_VERSION
Copy link
Member Author

Choose a reason for hiding this comment

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

build isn't needed anymore, I think pyodide-build will do fine.

cp pyproject.toml.setuppy pyproject.toml
CFLAGS=-g2 LDFLAGS=-g2 pyodide build
# Note that we need to cross-compile here. Using `pyodide build`
# fails on not having a cross file. The `source() function in
Copy link
Member Author

Choose a reason for hiding this comment

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

This is no longer true, the cross file is shipped by pyodide now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Update: still true actually, the cross file won't get applied as long as we have vendored-meson.

We also need the fix from pyodide/pyodide#4502 to actually pick up vendored-meson. With that, an out of tree build should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am on it – I shall be adding a patch file to port the above PR into vendored-meson, as discussed on Slack.

# tl;dr this needs some work, and it seems like cross-compilation is
# hard because Pyodide doesn't ship its own Python interpreter that
# we can actually target during a direct cross build.
python -m build --wheel -Csetup-args=--cross-file=$PWD/tools/ci/emscripten.meson.cross -Csetup-args=-Dallow-noblas=true
Copy link
Member Author

Choose a reason for hiding this comment

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

The -Dallow-noblas=true is no longer needed, the fallback to internal code when BLAS is missing is automatic.

Copy link
Member Author

Choose a reason for hiding this comment

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

The -Csetup-args=--cross-file=$PWD/tools/ci/emscripten.meson.cross will still be needed I think, only to add the longdouble format, just in slightly different format. From this comment I think it should be:

pyodide build setup-args="--cross-file=$PWD/tools/ci/emscripten.meson.cross"

Choose a reason for hiding this comment

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

Pyodide bundles emscripten.meson.cross used in in-tree build, so probably you can try that one too.

PYODIDE_MESON_CROSS_FILE=`pyodide config get meson_cross_file`
pyodide build -C "setup-args=\"${PYODIDE_MESON_CROSS_FILE}\""

@@ -77,4 +86,4 @@ jobs:
run: |
source .venv-pyodide/bin/activate
cd ..
python numpy/runtests.py -n -vv
pytest --pyargs numpy -m "not slow"
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is still needed.

@@ -106,6 +107,7 @@ max_features_dict = {
's390x': S390X_FEATURES,
'arm': ARM_FEATURES,
'aarch64': ARM_FEATURES,
'wasm32': {},
Copy link
Member Author

Choose a reason for hiding this comment

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

the changes in this file will still be needed.

endian = 'little'

[properties]
longdouble_format = 'IEEE_QUAD_LE'
Copy link
Member Author

Choose a reason for hiding this comment

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

Most of this file can be removed, only this is needed:

[properties]
longdouble_format = 'IEEE_QUAD_LE'

agriyakhetarpal added a commit to agriyakhetarpal/numpy that referenced this pull request Feb 23, 2024
Copied with updates and suggestions received from numpy#24603 on 23/02/2024.
agriyakhetarpal added a commit to agriyakhetarpal/numpy that referenced this pull request Feb 23, 2024
Copied with updates and suggestions received from numpy#24603 on 23/02/2024.
agriyakhetarpal added a commit to agriyakhetarpal/numpy that referenced this pull request Feb 27, 2024
This commit performs the following actions:

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

Co-Authored-By: Ralf Gommers <[email protected]>
agriyakhetarpal added a commit to agriyakhetarpal/numpy that referenced this pull request Feb 27, 2024
This commit performs the following actions:

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

Co-Authored-By: Ralf Gommers <[email protected]>
agriyakhetarpal added a commit to agriyakhetarpal/numpy that referenced this pull request Feb 27, 2024
This commit performs the following actions:

1. Adds WASM builds to the CPU family for Meson configurations, but without SSE or SIMD instructions.
2. Removes CPU feature detection message for an unsupported architecture.
3. Enables `IEEE_QUAD_LE` longdouble format for the wasm32 target (cross-builds).
4. Enables run for Emscripten/Pyodide wheels by setting the `if:` condition to `true`.
5. Uses recursive submodules to ensure that vendored-meson is received.
6. Moves the Meson cross file to `tools/ci/emscripten/` (i.e., creates a separate Emscripten folder to store relevant files)
7. Adds a patch for vendored-meson detection for Pyodide and applies this Pyodide-meson patch in the Emscripten CI jobs
8. Builds wasm32 wheels without BLAS and LAPACK support (see numpy#24750 (comment))
9. 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]>
agriyakhetarpal added a commit to agriyakhetarpal/numpy that referenced this pull request Feb 27, 2024
This commit performs the following actions:

1. Adds WASM builds to the CPU family for Meson configurations, but without SSE or SIMD instructions.
2. Removes CPU feature detection message for an unsupported architecture.
3. Enables `IEEE_QUAD_LE` longdouble format for the wasm32 target (cross-builds).
4. Enables run for Emscripten/Pyodide wheels by setting the `if:` condition to `true`.
5. Uses recursive submodules to ensure that vendored-meson is received.
6. Moves the Meson cross file to `tools/ci/emscripten/` (i.e., creates a separate Emscripten folder to store relevant files)
7. Adds a patch for vendored-meson detection for Pyodide and applies this Pyodide-meson patch in the Emscripten CI jobs
8. Builds wasm32 wheels without BLAS and LAPACK support (see numpy#24750 (comment))
9. 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]>
@rgommers rgommers removed this from the 2.1.0 release milestone Feb 27, 2024
@rgommers
Copy link
Member Author

This PR is superseded by gh-25894, things seem to work there. So I'll close this PR. Thanks for the help everyone!

@rgommers rgommers closed this Feb 27, 2024
@hoodmane
Copy link
Contributor

Thanks for working on this @rgommers and @agriyakhetarpal!

rgommers added a commit to agriyakhetarpal/numpy that referenced this pull request Feb 29, 2024
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]>
agriyakhetarpal pushed a commit to agriyakhetarpal/scikit-image that referenced this pull request Mar 18, 2024
lagru added a commit to scikit-image/scikit-image that referenced this pull request Jun 10, 2024
* Test emscripten workflow inspired by NumPy

Adds a CI job to build and test scikit-image in a Pyodide virtual
environment through WASM wheels. Following this, it would be
possible to include JupyterLite notebooks in the
documentation in order to run scikit-image's code snippets
which come with docstring-based examples.

Initial inspiration was taken from
numpy/numpy#24603.

Co-authored-by: Lars Grüter <[email protected]>
Co-authored-by: Stefan van der Walt <[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.

5 participants