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

Fix top level multiprocessing.connection import and update yt #3371

Merged
merged 2 commits into from Dec 20, 2022

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Dec 19, 2022

This updates yt. The new yt has a top level multiprocessing.connection import so we patch that so that it can be imported without raising an error.

@rth
Copy link
Member

rth commented Dec 19, 2022

Yeah, we have been going a bit back and forth on this. The problem is if we add such an empty file, then it's hard to direct at import time that Python is built without multiprocessing. This is occasionally done by CPython itself (e.g. here). So the general logic for missing functionality so far is if it's not available, the _ prefixed module should fail to import.

Some packages have now come to rely on the current functionality (e.g. joblib/joblib#1256 added a fix specifically for Pyodide), so we cannot easily change this behavior without breaking working code. We should probably have a test for the current behavior with a comment explaining it. Unless we have a solution we are confident would address all the use-cases, and I'm not convinced this is really it.

In this case, IMO it would be better to make changes to the multiprocessing module so it's importable but not usable without _multiprocessing but we would need to talk to CPyton devs about it, to make it more permanent. Though maybe the easiest is for astropy to handle the case where multiprocessing fails to import.

@jobovy
Copy link
Contributor

jobovy commented Dec 19, 2022

In this case, IMO it would be better to make changes to the multiprocessing module so it's importable but not usable without _multiprocessing but we would need to talk to CPyton devs about it, to make it more permanent. Though maybe the easiest is for astropy to handle the case where multiprocessing fails to import.

I think yt is the issue here, not astropy? I made the change to astropy to not fail from not being able to import _multiprocessing: astropy/astropy#12911. Although if import _multiprocessing worked, I think astropy would run without any pyodide-specific changes.

Arguably, depending on import _multiprocessing failing isn't a great way to detect that multi-processing is unavailable. Especially given that import multiprocessing works.

@rth
Copy link
Member

rth commented Dec 19, 2022

I think yt is the issue here, not astropy

Yes, wrong astrophysics related package :)

Arguably, depending on import _multiprocessing failing isn't a great way to detect that multi-processing is unavailable. Especially given that import multiprocessing works.

Agree that the situation is not ideal. IMO this kind of thing should be specified by CPython as to what should happen when a stdlib functionality is not available. I opened a discuss.python.org thread about it some time ago, but there was no clear conclusion, as far as I remember.

@hoodmane
Copy link
Member Author

Right. This also inconveniently breaks the tests. Let's just patch yt then.

@hoodmane
Copy link
Member Author

@rth how about this?

@hoodmane
Copy link
Member Author

Arguably, depending on import _multiprocessing failing isn't a great way to detect that multi-processing is unavailable. Especially given that import multiprocessing works.

It would be nice to have a setting multiprocessing.DISABLED or something like that.

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Sounds good to me, thanks!

It would be nice to have a setting multiprocessing.DISABLED or something like that.

Maybe, but we are not the only platform/build with these problems so we shouldn't have to invent new things either IMO. Similarly this kind of applies to many other optional stdlib modules, that may be excluded in the default Pyodide build. For instance, I just discovered there was a PEP 534 (deferred since 2016) which seems very relevant to this problem.

@hoodmane hoodmane changed the title Fix top level _multiprocessing import and update yt Fix top level multiprocessing.connection import and update yt Dec 20, 2022
@hoodmane hoodmane merged commit 68b3f3d into pyodide:main Dec 20, 2022
@hoodmane hoodmane deleted the top-level-_multiprocessing branch December 20, 2022 00:56
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

3 participants