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 import of loky package when _multiprocessing module is missing #309

Closed
wants to merge 7 commits into from

Conversation

hoodmane
Copy link

@hoodmane hoodmane commented Jan 10, 2022

I added a test which adds a dummy version of _multiprocessing to the path which just raises an ImportError and then tries to import loky. Then I added try blocks to catch all import errors from _multiprocessing.

Of course, it's not expected that loky will work very well when _multiprocessing is empty, but it is at least useful for it to import successfully.

See also joblib/joblib#1246 which adds the analogous test to joblib.

@hoodmane hoodmane changed the title Add test for correct handling of missing _multiprocessing module Fix top level import of loky package when _multiprocessing module is missing Jan 10, 2022
@rth
Copy link
Contributor

rth commented Jan 10, 2022

Thanks for working on this @hoodmane !

Let's see what other people think, maybe there is a better solution to joblib/joblib#1246 that wouldn't involve wrapping module imports in try except blocks. For instance, saying that if multiprocessing is not available loky is never imported in joblib could also work (but there are likely lots of inter-dependecies there as well)

Otherwise, there is some irony in a feature request requesting to use loky without multiprocessing :)

@hoodmane
Copy link
Author

There are a LOT of places that joblib imports from loky, but only a few places where loky imports from multiprocessing, so I think it's more natural to fix the loky imports. A particularly difficult aspect is that quite a few classes are imported from loky and then subclassed. We need a class of that name so that class SubBlah(SuperBlah) doesn't fail, but our plan is still for SubBlah to raise an error if ever instantiated.

there is some irony in a feature request requesting to use loky without multiprocessing

Yeah it is a bit silly.

Copy link
Collaborator

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Let's reconsider this once the current work on chunking and merging #304 is complete which hopefully should happen soon.

from .cloudpickle_wrapper import wrap_non_picklable_objects
from .process_executor import BrokenProcessPool, ProcessPoolExecutor


__all__ = ["get_reusable_executor", "cpu_count", "wait", "as_completed",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the __all__ list reflect the actual importable symbols?

try:
from .reusable_executor import get_reusable_executor
from .process_executor import BrokenProcessPool, ProcessPoolExecutor
except ImportError:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a comment to explain why:

Suggested change
except ImportError:
except ImportError:
# On environments like pyodide that do not implement the _multiprocessing
# module in the standard library, we still want loky to be importable without
# failing, even if not really usable.

@@ -16,20 +16,24 @@
import threading
import collections


HAS_CONCURRENT_FUTURES = False
if sys.version_info[:2] >= (3, 3):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather not merge this PR while we are in the process or dropping support for Python <3.7.

@hoodmane
Copy link
Author

hoodmane commented Feb 2, 2022

Thanks for the review @ogrisel!

I would rather not merge this PR while we are in the process or dropping support for Python <3.7.

Yeah I'll wait until #303 is done then there will surely be merge conflicts and simplifications to make here.

@hoodmane hoodmane closed this Feb 2, 2022
@hoodmane hoodmane reopened this Feb 2, 2022
@ogrisel
Copy link
Collaborator

ogrisel commented Feb 10, 2022

Is this really needed since joblib/joblib#1256 has been merged in joblib?

I assume that no-one will actually use loky on a platform without multiprocessing but it might help support other libraries that optionally use loky but not by default.

Let me know what you think, otherwise we can close.

@tomMoral
Copy link
Collaborator

I am in favor of closing as loky is really linked to multiprocessing, and therefor it would be strange to install it on a platform that does not have multiprocessing. It is not like joblib where we have a fallback option and it seems okay to ask downstream libraries to only install/import loky when it is actually usable.

But if you think it is worth it, I am fine with protecting the top level import.

@ogrisel
Copy link
Collaborator

ogrisel commented Feb 10, 2022

I am in favor of closing as loky is really linked to multiprocessing, and therefor it would be strange to install it on a platform that does not have multiprocessing.

Some library that can run on pyiodide might have a dependency that is imported when importing the lib even though it is not used by default. But I am not sure if it is the case for any popular library or not, once joblib is taken care of.

@hoodmane
Copy link
Author

Well I am happy with the fix in joblib. I think if we run into other packages that use loky we should also try to patch them to handle missing multiprocessing. This way they probably won't need to download a copy of loky at all.

Thanks @tomMoral and @ogrisel!

@hoodmane hoodmane closed this Feb 10, 2022
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