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.

Sign up for GitHub

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

Jump to bottom

Build wheels for emscripten target #110

Merged
merged 3 commits into from
Jun 22, 2022
Merged

Build wheels for emscripten target #110

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa66dea
Build wheels for emscripten target
messense Jun 22, 2022
fdacefc
Only build emscripten wheels on push to main and on release
messense Jun 22, 2022
2d853ed
Update ci.yml
messense Jun 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,53 @@ jobs:

- run: make rust-benchmark

build_wasm_emscripten:
name: build wasm emscripten
# only run on push to main and on release
if: "success() && (startsWith(github.ref, 'refs/tags/') || github.ref == 'refs/heads/main')"
needs: [test, lint]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3

- uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: nightly
components: rust-src
target: wasm32-unknown-emscripten
override: true

- uses: mymindstorm/setup-emsdk@v11
with:
version: 3.1.13
actions-cache-folder: emsdk-cache

- name: set up python
uses: actions/setup-python@v4
with:
python-version: '3.10'
Comment on lines +173 to +181
Copy link
Contributor

Choose a reason for hiding this comment

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

Pyodide should probably make a github action so that it is easier for people to keep the version in sync. You should be able to specify a target Pyodide version and get the relevant dependencies. In order to do that, we should probably start storing this information maybe in a file like version_info.json
@ryanking13 @rth


- name: set package version
run: python .github/set_version.py
if: "startsWith(github.ref, 'refs/tags/')"

- name: Sync Cargo.lock
run: cargo update -p pydantic-core
if: "startsWith(github.ref, 'refs/tags/')"

- run: pip install -U --pre maturin

- name: build wheels
run: maturin build --release --target wasm32-unknown-emscripten --out dist -i 3.10
Copy link
Contributor

Choose a reason for hiding this comment

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

@messense It might be sensible to disable ABI3 for Emscripten wheels for now, because at least if we are using them with Pyodide we can only target one Python version at a time. There isn't (guaranteed to be) ABI compatibility between different Emscripten versions:
emscripten-core/emscripten#15917

Other Python distributions are unlikely to use exactly the same Emscripten version as us, and we update Emscripten more often than we update Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be sensible to disable ABI3 for Emscripten wheels for now

I think that should be done in pyo3 if desirable. cc @davidhewitt

And pydantic-core doesn't support ABI3 so it's not a problem in here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that disabling ABI3 at the PyO3 level probably is overkill? As far as PyO3 is concerned, building with ABI3 will just restrict the symbols that are available for use. Seems like it would be reasonable for maturin / setuptools_rust to ignore the abi3 flags and create version-specific wheels anyway - I don't think there'd be a problem with doing that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course it's harmless to generate abi3 wheels as long as the cost isn't very high.


- run: ls -lh dist

- uses: actions/upload-artifact@v3
with:
name: wasm_wheels
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since PyPI does not allow uploading wasm32 wheels right now, I choose to not putting them in pypi_files artifact.

pypi/warehouse#10416

Copy link
Contributor

Choose a reason for hiding this comment

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

We have been discussing making a Pyodide wheel warehouse for people making out of tree builds, but there hasn't been a strong motivation so far because very few people are making out of tree builds yet.

Copy link
Member

Choose a reason for hiding this comment

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

@hoodmane do you have a link for this?

if micropip can install via a URL, we could upload the wasm wheel as a github release artifact and install straight from there?

I'd love to try using this package in web-assembly, but I'm not sure where to start.

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR added the ability to install binary wheels:
pyodide/pyodide#2591
If you use the nightly Pyodide you can just say await micropip.install("url_to_wheel") and you'll be good to go, at least as long as all your binary dependencies are included in Pyodide.

Copy link
Contributor

Choose a reason for hiding this comment

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

Easiest way to run tests is in node with file:relative/path/towheel.whl (also requires nightly Pyodide).

path: dist

build:
name: build on ${{ matrix.platform || matrix.os }} (${{ matrix.target }} - ${{ matrix.manylinux || 'auto' }})
# only run on push to main and on release
Expand Down