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

Build wheels for emscripten target #110

Merged
merged 3 commits into from Jun 22, 2022
Merged

Conversation

messense
Copy link
Contributor

@messense messense commented Jun 22, 2022

@codecov
Copy link

codecov bot commented Jun 22, 2022

Codecov Report

Merging #110 (2d853ed) into main (477ee17) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #110   +/-   ##
=======================================
  Coverage   96.76%   96.76%           
=======================================
  Files          40       40           
  Lines        3620     3620           
  Branches       28       28           
=======================================
  Hits         3503     3503           
  Misses        117      117           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 477ee17...2d853ed. Read the comment docs.

@messense messense marked this pull request as ready for review June 22, 2022 00:52
@messense messense mentioned this pull request Jun 22, 2022

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

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise, LGTM.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@samuelcolvin
Copy link
Member

LGTM, I'll leave this open for a day or so in case @hoodmane has any feedback or suggestions.

Comment on lines +173 to +181
- 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'
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

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

Copy link
Contributor

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

This generally looks good to me.

Out of tree builds for Pyodide are still in a pretty alpha state. I am not sure what is the best route forward for communicating with early adopters of out of tree Pyodide builds. Currently we only PyO3 and you trying to do this so it's easy to communicate about incoming breakages.

We have a lot of work to do on this in Pyodide...

@samuelcolvin samuelcolvin merged commit d01ac2c into pydantic:main Jun 22, 2022
@samuelcolvin
Copy link
Member

Thanks so much both, I've merged this to avoid breaking it in another PR, but happy to keep discussing on this PR or the issue, or elsewhere.

@messense messense deleted the emscripten branch June 22, 2022 14:59
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.

None yet

4 participants