Use cibuildwheel to build pyodide wheels#382
Conversation
|
Thanks, @oscarbenjamin! I'll take a closer look when I log in to work later today, but FYI you can also remove the setup-node, setup-python, and setup-emscripten steps. The cibuildwheel action takes care of setting up Python, and the Pyodide code in cibuildwheel takes care of setting up Node.js and Emscripten (and is faster at doing that too). |
|
Thanks. I removed some things at first but then the second commit here (6d15e42) added back mymindstorm/setup-emsdk and that seemed to fix the basic problem from the first commit that failed immediately with |
|
Is there a reason you chose to build GMP, MPFR and FLINT as static libraries? I imagine that is a typical thing to do for pyodide. What I'm not sure about are the license implications because GMP, MPFR and FLINT are all LGPL and the usual thing to do with LGPL is dynamic linking so that someone can replace the shared libraries with their own versions. That makes the LGPL not contagious in the way that GPL is so you can mix together differently licensed things with LGPL as long as the LGPL parts are replaceable (which they clearly are if they are shared libraries). |
|
This error reared its head after the license files commit: |
|
Then the last commit had |
|
I wonder if both of those are intermittent failures, not related to the changes in the last two commits. |
|
In fact the second one is intermittent. It isn't to do with the pyodide build at all. I've been seeing that failure occasionally with the FLINT main branch CI job. Earlier I bumped the main FLINT version from 3.3.1 to 3.4.0 so it is possible that is why we first see this now. |
meson.build
Outdated
| dep_py = py.dependency() | ||
| if meson.is_cross_build() and host_machine.system() == 'emscripten' | ||
| # Avoid picking up the runner's host python.pc via pkg-config. | ||
| # For Pyodide, use the interpreter sysconfig data from pyodide-build instead. | ||
| dep_py = py.dependency(method: 'system') | ||
| else | ||
| dep_py = py.dependency() | ||
| endif |
There was a problem hiding this comment.
This was apparently necessary to avoid picking up the wrong Python headers at C compile time. Is there a better way?
There was a problem hiding this comment.
I'm not sure, actually. This should have worked, but I wonder if there is some sort of PATH mangling going around here. My first instinct was to of course check NumPy which has worked with Pyodide's build system since forever, and I found that it doesn't do this: https://github.com/numpy/numpy/blob/7baf0e7c71dfc2de9fa5022cf51cb4873efd4285/meson.build#L38-L39. I think it's probably okay for now, but should be good to get to the bottom of later.
There was a problem hiding this comment.
Are we using a Meson cross file? I remember it was there when I was looking at the PR last night, but I can't see why you removed it through the comments you posted as you worked through the PR. Could it be perhaps something to do with that?
There was a problem hiding this comment.
So, I think pyodide uses its own cross file. I added one but then removed it because this py.dependency thing was what actually made it work.
Now though (per your comment below) I just remove py_dep altogether and that seems to be fine.
I guess that py_dep was confusing the situation somehow when it isn't really needed...
The generated wheel is 150MB and expands to 450 MB when unzipped. I think this is because python-flint has over 30 extension modules that all link to these same libraries. Using static linking likely means that we have many copies of the same machine code across all those extension modules. By comparison the manylinux wheels are about 10 MB and the bulk of that is just libflint.so which expands to about 13MB when unzipped. |
oscarbenjamin
force-pushed
the
pr_pyodide_cleanup
branch
from
c189523 to
33f9617
Compare
|
I've changed it to build shared libraries and bundle them and it seems to be working now. Now the wheel is 15MB expanding to 36MB uncompressed. The bulk of that is just libflint.so weighing in at 27MB and then GMP and MPFR add up to 3MB so there are 30MB of shared libraries and then 6MB of all of python-flint's actual Python code and extension modules. I guess that statically linking was stripping some stuff to get the extension modules from 30MB of static library down to more like 10MB but in the new wheel those extension modules are more like 100KB so static linking is about 10MB of overhead per extension module and with over 30 extension modules that ends up much more than just shipping 30MB of separate shared library files. |
|
@agriyakhetarpal thanks for chiming in. As far as I am concerned this is done now. Let me know if you are thinking of reviewing it or otherwise I would just merge at this point. We can of course follow up after merge anyway as well (although I am planning a release ASAP). The changes here are:
|
|
I am taking a look now; thanks for the summary @oscarbenjamin! |
|
Great timing! I'm heading out to the pub and shop now but will be back in a couple of hours. |
agriyakhetarpal
left a comment
agriyakhetarpal
left a comment
There was a problem hiding this comment.
This is looking great, thanks a lot! I hope you are having fun outside! :) I took a look at the workflows and I have a few comments, nothing substantial at all or broken, so that's a good sign. Please feel free to take or leave the suggestion to bump the Pyodide version, lest it causes some new test failures (probably best to try out in another PR if it does).
Switching to a shared library should be alright. As a follow-up for whenever this PR makes it to a release I think we should also do that for the pyodide-recipes builds for libflint and co. The difference is that instead of being linked into the wheel and the python-flint shared objects, the libraries will start to be loaded at runtime, e.g., how OpenBLAS is separately loaded with SciPy. So in the browser console you'll start seeing something like Loading libflint, libgmp, libmpfr, python-flint and then Loaded libflint, libgmp, libmpfr, python-flint and so on.
P.S. I did previously notice you pinged me on a couple of other WASM-related items on SymPy and here sometime late last year. I missed responding to them because I was travelling, either on vacation, or the usual FOSS or scientific Python conference or so on, and those were merged/resolved in my absence. Hence, I never got to writing a comment/message on those; I apologise! However, please feel free to continue pinging me for things like this as you see fit :D
meson.build
Outdated
| dep_py = py.dependency() | ||
| if meson.is_cross_build() and host_machine.system() == 'emscripten' | ||
| # Avoid picking up the runner's host python.pc via pkg-config. | ||
| # For Pyodide, use the interpreter sysconfig data from pyodide-build instead. | ||
| dep_py = py.dependency(method: 'system') | ||
| else | ||
| dep_py = py.dependency() | ||
| endif |
There was a problem hiding this comment.
I'm not sure, actually. This should have worked, but I wonder if there is some sort of PATH mangling going around here. My first instinct was to of course check NumPy which has worked with Pyodide's build system since forever, and I found that it doesn't do this: https://github.com/numpy/numpy/blob/7baf0e7c71dfc2de9fa5022cf51cb4873efd4285/meson.build#L38-L39. I think it's probably okay for now, but should be good to get to the bottom of later.
| - run: | | ||
| pyodide venv .venv-pyodide | ||
| source .venv-pyodide/bin/activate | ||
| pip install wheelhouse/*.whl | ||
| pip install pytest hypothesis | ||
| python -m pytest -svra -p no:cacheprovider --pyargs flint |
There was a problem hiding this comment.
I think it would be better to move this to cibw-test-command territory, as that would avoid writing another job just to test the Pyodide wheels. You can build and test them in one go, which also saves CI time. cibuildwheel also takes care of syncing the Pyodide version, the pyodide-build version, etc. automatically because the environment is locked. Is there a specific reason to have another job?
There was a problem hiding this comment.
I set the test rig up this way a long time ago not for pyodide but for the other platforms when using cibuildwheel. The reason is that when the cibuildwheel step fails we don't end up with a downloadable artifact. If cibuildwheel succeeds then I want to be able to download the wheels even (especially!) if the wheels fail testing.
It is not often that I do that but it tends to be useful in exactly this situation when you are trying to get things working in the first place. I did in fact download the wheel earlier after e5dc593 where build succeeded but testing failed. It turned out that building the shared libraries with SIDE_MODULE=2 stripped out all the code so libflint.so was only about 300 bytes. The fix in 57f4aec was to set SIDE_MODULE=1 and then it worked.
Being able to download the broken wheel was very helpful in that situation.
As an aside here I did not find any clear documentation that explains precisely what emscripten's SIDE_MODULE=1 vs SIDE_MODULE=2 actually does although this is explained for MAIN_MODULE. The effect is clear if you download the wheels from the referenced commit though: in one case libflint.so is 300 bytes and in the other it is 27MB.
meson.build
Outdated
| dep_py = py.dependency() | ||
| if meson.is_cross_build() and host_machine.system() == 'emscripten' | ||
| # Avoid picking up the runner's host python.pc via pkg-config. | ||
| # For Pyodide, use the interpreter sysconfig data from pyodide-build instead. | ||
| dep_py = py.dependency(method: 'system') | ||
| else | ||
| dep_py = py.dependency() | ||
| endif |
There was a problem hiding this comment.
Are we using a Meson cross file? I remember it was there when I was looking at the PR last night, but I can't see why you removed it through the comments you posted as you worked through the PR. Could it be perhaps something to do with that?
|
Thanks for the review. I'm working through it now... |
|
I don't seem to be able to reply to this in the normal reivew/reply way so replying here:
I did add a cross file and that was my first attempt at getting meson to find the correct Python headers for the cross build. However, that did not work and then I found that using It definitely seemed like a cross file would be the right thing to use there but it did not give any discernible improvement so it was added in 5794e26 and removed in 920ced0. The in-between commit that seemed to actually fix the problem was afe2b14. Oh, also it didn't seem like the cross file was actually being used. I think there was something in the meson output that referenced a cross-file from pyodide as if pyodide-build was already forcing its own cross file and that was taking precedence. Maybe I didn't set up the cross file correctly? |
|
Yes, we have the WASM cross file being applied and picked up automatically for Meson projects. The difference for NumPy is that it uses its custom vendored Meson, in which case the cross file has to be added manually like this: https://github.com/numpy/numpy/blob/7baf0e7c71dfc2de9fa5022cf51cb4873efd4285/pyproject.toml#L232. I assume that is what is making it work without the We also have a copy of this cross file in the So scikit-learn is closer to what we want. One difference I have been able to find is that they don't use |
Oh, maybe we don't need it. I probably wrote that in the early days of learning meson... |
|
Okay, thanks. I have pushed all suggested changes apart from removing the separate test job because I prefer to keep it that way and it was useful to have it like that today even when debugging a problem with this wheel build. Each change is a separate commit and a separate 40 minute CI run. It is still early in the runs and so far there are some unrelated random CI failures but otherwise all looks good for the relevant jobs. |
|
Okay, Ci is green. I made all changes including removing dep_py and everything seems to work! |
agriyakhetarpal
left a comment
agriyakhetarpal
left a comment
There was a problem hiding this comment.
I am glad we got to the bottom of the Meson cross file issue. I checked all files, and everything looks in order and good to merge. I will follow up with my cibuildwheel-Pyodide-patches change upstream soon – it's not a lot of code, fortunately, because the patches are all included in and handled through the xbuildenv. Thanks a lot for working through this!
|
Great, thanks always for your help! I'm going to merge this and then hopefully that will mean that we have nightly pyodide wheels... I also opened a docs issue at autditwheel_emscripten: |
Yes, this is a bit under-explained. |
2 participants
CC @agriyakhetarpal
I'm not sure if there was a reason why the pyodide build didn't use cibuildwheel in the first place but it would be cleaner if we could combine all platforms into one cibuildwheel call.