-
Notifications
You must be signed in to change notification settings - Fork 7
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
Support Pyodide 0.27 and bump to Python >=3.12 #57
base: main
Are you sure you want to change the base?
Support Pyodide 0.27 and bump to Python >=3.12 #57
Conversation
Thanks @agriyakhetarpal for taking over the maintenance! |
Pyodide 0.26.4 and 0.27.3 fail with a Tracing this further, I see that Pyodide 0.24.1 kept the I have to note that 0.25.1 and 0.27.3 fail with different errors, so it is also likely that changes in the order of the imports, etc., are causing things to fail. Importing |
Regarding encodings I don't remember the exact details, but indeed the overall idea was that they were taking space so utf8 + a few specific ones should be enough to start Python. But maybe indeed the list of minimal required encodings changed.
Yeah sorry part of it might also be relying on internal emscripten APIs so if those parts (in discovery.js) changed that would explain the breakage. |
Thanks! I fixed the There is no proper error message in the CI logs, but I am able to reproduce the failure locally, which does have a stack trace:
Tap to expand full error message
I'm not sure how to debug that so far as it's quite cryptic, so any suggestions would be welcome in case you're familiar with it. :) |
I think I did encounter it the past, but I'm not 100% sure/remember the cause. It's raised here but in this case the error is misleading, because we are not trying to load file_packager (legacy pre-wheel) files. Probably something is not quite right with the sequence in which dynamic libraries are loaded (like if something changed in pyodide in that respect). If you don't find it, ask Hood :) |
Maybe related to pyodide/pyodide#4726, see @hoodmane's comments in the code there. It looks like _module.API.loadDynlib would be calling I don't know maybe monkeypatching locateFile back to the original value would be enough to make it work? |
One mention of In The error message is here: https://github.com/pyodide/pyodide/blob/0f3e5f029275482882174b20770caf00290ec4f2/src/js/pyodide.ts#L284-L287 Edit: ah, thanks, let me take a look at your comments! |
Nah I think it was working in 2023 and that PR is from 2021. But this part does keep breaking with emscripten or pyodide updates, I last fixed in it #34 |
In the meantime, I can confirm that pyodide/pyodide#4726 did not have an effect, as it first turned up in 0.26.0a5 and this test passes with Pyodide 0.26.4 locally (I'll confirm that with CI in a moment). |
93b917b
to
886a1bb
Compare
This generally happens when we try to locate the shared library from the file system but when it fails (if file is not there, or there was an error while loading the library, etc). As Roman said, emscripten will fallback to When building pyodide.asm.js, setting |
Thanks for the insights, @rth and @ryanking13! I've been able to isolate the issue – it's coming from the fact that OpenBLAS isn't being correctly bundled against Pyodide 0.27.3:
As it can be seen above, Later, this shows up here: This manifests later when we try to import extension modules in SciPy that rely on OpenBLAS, such as Here is a brief summary from my extended logging: Pyodide 0.26.4
and
and later
Pyodide 0.27.3
and in this case, the error message is indeed not helpful; it hides the traceback. To be more specific, the actual error is masked a few layers underneath by this error: Tap to show error message
I did not need to use debug builds of Pyodide to get these traces so far. I can put together more logging in a new PR after this one through a The only relevant changes in this area that made it to the Pyodide 0.27 alphas were through pyodide/pyodide#4876 and pyodide/pyodide#4871. I don't think we changed anything after them up to the arrival of 0.27 stable. My hunch is that these PRs will provide the necessary information for us to proceed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a self-review: I'm not fully sure whether my changes are correct, but I'm open to suggestions as this is the first time I'm going through our dynamic library loading procedure and trying to understand it better.
Either way, this is ready for review and the tests are passing, marking it as such!
examples/netcdf4/app.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this test because we haven't had any other tests like it so far. Here, we have both a shared library (libhdf5
) and a static library (libnetcdf
), a package that depends on both of them (h5py
), and eventually netCDF4
– i.e., we weren't testing the integration with any static libraries, just shared ones (SciPy + OpenBLAS). I can drop it if you suggest I do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to ask if it would be okay if I could address missing coverage in a further PR? The change doesn't do much; it just fails more gracefully by printing a warning when loading a dynlib. What I understand here that this loader is an elementary check and that we don't load the libs in the order they are supposed to be loaded in – we just run over what comes first in the list. It could be the case that I might have made a mistake somewhere.
With this, the netcdf4
test does display this locally:
Warning: Failed to load /lib/python3.12/site-packages/h5py/_conv.cpython-312-wasm32-emscripten.so: Error: Didn't expect to load any more file_packager files!
Warning: Failed to load /lib/python3.12/site-packages/h5py/_errors.cpython-312-wasm32-emscripten.so: Error: Didn't expect to load any more file_packager files!
but at the same time, the code in examples/netcdf4/app.py
executes and runs completely fine till the end, which I don't understand.
@@ -123,7 +123,7 @@ def test_strip_module_docstrings(): | |||
) | |||
|
|||
|
|||
@settings(deadline=300) | |||
@settings(deadline=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get this test to pass within 500ms :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @agriyakhetarpal . Changes wise LGTM, but I haven't followed latest pyodide development.
pyodide_pack/runtime_detection.py
Outdated
# Always mark shared libraries as shared=True to ensure they're loaded | ||
# globally, for libraries that need these global symbols | ||
if extension == ".so": | ||
dll.shared = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about this? Is far as I remember cython generated objects are also .so and they don't need to be managed as global symbol.
I think the idea was more to look whether the shared_library
tag in packages https://github.com/pyodide/pyodide/blob/main/packages/openblas/meta.yaml#L6 and in which case load it.
But yeah, in any case this part is suboptimal so feel free to merge as is, and investigate this separately (if you are interested in this topic). Because updating to the latest Pyodide version is probably more important.
🤔 I tried the Thanks for the review in the meantime! |
Some more investigation for this particular example (
|
4e3717d
to
31a3f52
Compare
I don't exactly remember, but there might be some changes how CPython deepfreezes encodings module in Python 3.11 and 3.12. I think there are some room to investigate, but for now your change looks fine to me.
Right. I changed the library to be loaded locally if possible. If pyodide-pack is relying on whether the library is loaded locally or globally, I guess it should be changed. |
Sounds good, I'll leave that for exploration in another PR!
Yes, I've adjusted for it. I haven't found a solution to how the netCDF4 test can be fixed. It's good that I added such a test, though, as the rest of the tests were passing and we would have merged with broken functionality. The netCDF4 test passes on 0.24.1, so the failure with 0.27.1 is indeed related. Do we have a guarantee that the libraries in |
Closes #47
Closes #45