Skip to content

Conversation

@IvanIsCoding
Copy link
Contributor

@IvanIsCoding IvanIsCoding commented May 11, 2025

Related to Qiskit/rustworkx#1447

In short, this adds a very experimental rustworkx to Pyodide. If this is the wrong place to submit a recipe, let me know.

At the current status, the other rustworkx author has not yet merged the Pyodide support. It also might take a while for us to publish 0.17.0. So I called it a pre-release with 0.17.0a3 and hosted it in my GitHub fork's release.

Overall, this seems to be working on the browser but failing with Python from pyodide venv so I will open an issue on pyodide-build asking a couple questions. (edit: report in pyodide/pyodide-build#201)

url: https://github.com/IvanIsCoding/rustworkx/releases/download/v0.17.0a3/rustworkx-0.17.0a3.tar.gz
sha256: 8bd0c295134e2b0c03808d4e69428b41153849db6488839084a78793c337f191
build:
script: export RUSTFLAGS="-C target-feature=+atomics,+bulk-memory,+mutable-globals -C link-arg=-s -C link-arg=ALLOW_MEMORY_GROWTH=1 -C link-arg=-s -C link-arg=MODULARIZE=1 -C link-arg=-s -C link-arg=EXPORT_NAME=createModule -C link-arg=-s -C link-arg=EXPORT_ES6=1 -C link-arg=-s -C link-arg=ASSERTIONS=1"
Copy link
Member

Choose a reason for hiding this comment

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

I think you should get rid of this RUSTFLAGS setting. It surely won't with this.

Suggested change
script: export RUSTFLAGS="-C target-feature=+atomics,+bulk-memory,+mutable-globals -C link-arg=-s -C link-arg=ALLOW_MEMORY_GROWTH=1 -C link-arg=-s -C link-arg=MODULARIZE=1 -C link-arg=-s -C link-arg=EXPORT_NAME=createModule -C link-arg=-s -C link-arg=EXPORT_ES6=1 -C link-arg=-s -C link-arg=ASSERTIONS=1"

If you do need to add something to RUSTFLAGS try to do it like this:

   export RUSTFLAGS="$RUSTFLAGS -C <some new flag>"

The stuff that is in there by default is important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree there are too many flags. From https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags I thinl that pyodide build interferes with.cargo/config.toml. I will try to cut the number of flags, many of them were cargo culted (no pun intended). For the rest, I will see if setting RUSTFLAGS="" catches the options from .cargo/config.toml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reality is the build just need s"-C", "target-feature=+atomics,+bulk-memory,+mutable-globals" in addition to the default flags. But for pyodide build setting RUSTFLAGS doesn't seem to be additive?

Copy link
Member

@hoodmane hoodmane May 12, 2025

Choose a reason for hiding this comment

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

You can do export RUSTFLAGS="$RUSTFLAGS -Ctarget-feature=+atomics,+bulk-memory,+mutable-globals. But bulk-memory and mutable-globals are on by default (at least they should be...). Atomics probably isn't.

@github-actions
Copy link
Contributor

github-actions bot commented May 12, 2025

Package Build Results

Total packages built: 35
Total build time: 0:03:34

Package Build Times (click to expand)
Package Build Time
openssl 2m 54s
rustworkx 2m 29s
sqlite3 1m 41s
liblzma 1m 5s
test 24s
regex 12s
cpp-exceptions-test 6s
pydecimal 5s
lzma 5s
pydoc_data 4s
buffer-test 4s
cpp-exceptions-test2 4s
MarkupSafe 4s
ssl 4s
sharedlib-test-py 3s
fpcast-test 3s
hashlib 3s
atomicwrites 2s
packaging 2s
sharedlib-test 1s
pytest 1s
attrs 1s
pyparsing 1s
setuptools 1s
Jinja2 1s
pytz 1s
py 0s
six 0s
exceptiongroup 0s
micropip 0s
pluggy 0s
pytest-asyncio 0s
iniconfig 0s
tblib 0s
more-itertools 0s

Longest build: openssl (2m 54s)
Packages built in more than 10 minutes: 0

@hoodmane
Copy link
Member

Package Build Results

Wow that's fancy @ryanking13

@IvanIsCoding
Copy link
Contributor Author

Ok, I reused the RUSTFLAGS. It will be pretty evident if the script does not capture the defaults, the tests will fail I think.

For the other target features, I didn't get a very conclusive answer by running rustc +nightly -Z unstable-options --print target-features --target=wasm32-unknown-unknown. It just listed all available features. Atomic for sure is an unstable option, but I am not 100% confident the other two are enabled by default. So I will leave them even if they are somewhat redundant

@IvanIsCoding IvanIsCoding requested a review from hoodmane May 12, 2025 22:26
Copy link
Member

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

Looks good to me if tests pass.

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Thanks, @IvanIsCoding ! The tests are passing, and I had two comments, this should be good to merge after them. I'm glad you were able to figure out the out-of-tree build, too.

@IvanIsCoding
Copy link
Contributor Author

IvanIsCoding commented May 15, 2025

Ok, I will be able to add a check on my CI: Qiskit/rustworkx#1450. I think I found a way to make things work with a pinned Pyodide Build 0.30.2, Pyodide 0.27.5, and Emscripten 3.1.58 via https://pixi.sh/.

Soon that Pyodide version will be outdated, but I think as long as a build with 0.27.5 keeps passing, the tests in pyodide-recipes will catch any trivial errors with newer versrirons.

@agriyakhetarpal
Copy link
Member

Sounds good to have a smoke test, but I'm also a little confused about your PRs – why not run the full test suite in the CI using pyodide venv and installing the WASM wheel within it? We don't run the test suite here as the recipes are what have smoke tests, so that we can get to distributing/updating packages faster, plus we unvendor the tests using rudimentary mechanisms to reduce wheel size. However, in your own CI, there is no such limitation and you could run the tests on every commit? This is what many scientific Python packages are doing – they either build the wheel using pyodide build + run the tests by installing the WASM wheel, pytest, etc. in a Pyodide virtual environment, or they use cibuildwheel's --platform pyodide option which does the same thing with a neater interface.

@agriyakhetarpal
Copy link
Member

BTW, the Python interpreter provided by pyodide venv is also just some code running with Node.js as a part of a polyglot binary – it should be more Pythonic to use, and you can avoid adding the JS script this way. :)

@agriyakhetarpal
Copy link
Member

Another thing I noticed from the pixi setup – while it mentions that it is possible only with with linux-x86-64; I'd like to note that linux-arm64, macos-amd64, and macos-arm64 should all work equally well. We test for macOS in pyodide-build's CI.

@IvanIsCoding
Copy link
Contributor Author

IvanIsCoding commented May 15, 2025

Sounds good to have a smoke test, but I'm also a little confused about your PRs – why not run the full test suite in the CI using pyodide venv and installing the WASM wheel within it? We don't run the test suite here as the recipes are what have smoke tests, so that we can get to distributing/updating packages faster, plus we unvendor the tests using rudimentary mechanisms to reduce wheel size. However, in your own CI, there is no such limitation and you could run the tests on every commit? This is what many scientific Python packages are doing – they either build the wheel using pyodide build + run the tests by installing the WASM wheel, pytest, etc. in a Pyodide virtual environment, or they use cibuildwheel's --platform pyodide option which does the same thing with a neater interface.

We can move to somewhere as this PR is approved but for the full test suite: I think rustworkx will be able to run a full test soon. We use unittest so to be honest I think I might be able to just mount our test code and write something very basic unittest.TestLoader and unittest.TextTestRunner.

Why not use pyodide venv? I wanted to have the least dependencies and commands possible.
. I pinned on a Pyodide version from npm, Node is pretty stable and it seems like pyodide does a good job with its own lockfile.

And for the Linux part in Pixi: you opened conda-forge/emscripten-feedstock#35. If it got closed I'd add macOS support for aarch64. I'd also add Linux support for aarch64 but they don't have that either. And Windows users should use WSL I don’t want to deal with Emscripten on Windows.

@IvanIsCoding
Copy link
Contributor Author

Qiskit/rustworkx#1450 now has the full test suite. Fortunately, all tests pass. The only ones that are skipped are the ones that required graphviz. Honestly, the result is better than anticipated.

@agriyakhetarpal
Copy link
Member

Why not use pyodide venv? I wanted to have the least dependencies and commands possible.
. I pinned on a Pyodide version from npm, Node is pretty stable and it seems like pyodide does a good job with its own lockfile

Makes sense! That said, if you were to move, the only extra dependency would be virtualenv (used to create the virtual environment), and the virtual environment will also use the same lockfile (so you'll get NumPy 2.0.2 there, too).

And for the Linux part in Pixi: you opened conda-forge/emscripten-feedstock#35. If it got closed I'd add macOS support for aarch64. I'd also add Linux support for aarch64 but they don't have that either. And Windows users should use WSL I don’t want to deal with Emscripten on Windows.

Ah, that's right. No one has responded there, I'll take a look now!

Qiskit/rustworkx#1450 now has the full test suite. Fortunately, all tests pass. The only ones that are skipped are the ones that required graphviz. Honestly, the result is better than anticipated.

Thank you, this is awesome!

@agriyakhetarpal
Copy link
Member

I don't think there has been anything else to do here, so I guess we can merge now!

@agriyakhetarpal agriyakhetarpal merged commit ca1405a into pyodide:main May 16, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants