Fix top level import of loky package when _multiprocessing module is missing#309
Fix top level import of loky package when _multiprocessing module is missing#309hoodmane wants to merge 7 commits intojoblib:masterfrom
Conversation
hoodmane
changed the title
|
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 :) |
|
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
Yeah it is a bit silly. |
| from .process_executor import BrokenProcessPool, ProcessPoolExecutor | ||
|
|
||
|
|
||
| __all__ = ["get_reusable_executor", "cpu_count", "wait", "as_completed", |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
Please add a comment to explain why:
| 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. |
|
|
||
|
|
||
| HAS_CONCURRENT_FUTURES = False | ||
| if sys.version_info[:2] >= (3, 3): |
There was a problem hiding this comment.
I would rather not merge this PR while we are in the process or dropping support for Python <3.7.
|
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. |
|
I am in favor of closing as But if you think it is worth it, I am fine with protecting the top level import. |
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. |
4 participants
I added a test which adds a dummy version of
_multiprocessingto the path which just raises anImportErrorand then tries to importloky. 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
_multiprocessingis empty, but it is at least useful for it to import successfully.See also joblib/joblib#1246 which adds the analogous test to joblib.