Conversation
arquolo
force-pushed
the
master
branch
11 times, most recently
from
63bfce8 to
929d599
Compare
arquolo
force-pushed
the
master
branch
3 times, most recently
from
938bc4c to
093515c
Compare
|
Thanks for working on this @arquolo. Feel free to simplify the tox.ini and Azure Pipelines config accordingly. |
|
I've already done it locally. Is that normal that linux-pypy3 job is 6 times slower than others? |
That's kind of expected since our test suite keeps spawing new subprocesses and the PyPy process start overhead is significantly larger than CPython's. |
Feel free to open a dedicated PR for this. |
|
That's strange, linux-pypy3 failed, given that all f153b15 commit has changed is adding/removing of some newlines. |
arquolo
force-pushed
the
master
branch
6 times, most recently
from
f792261 to
d3fe090
Compare
ogrisel
left a comment
ogrisel
left a comment
There was a problem hiding this comment.
There are still a few version-related cleanups from this PR that we would like to pickup.
However there are also unrelated changes that are better reviewed and documented as independent PRs (see below for details).
I also don't understand why the deletion of loky/backend/managers.py and other files still show-up here despite the merge of master in this branch. Those files have already been deleted from master. Maybe a new merge is necessary?
loky/backend/reduction.py
Outdated
| register(type(_C().f), _reduce_method) | ||
| register(type(_C.h), _reduce_method) | ||
| dispatch_table[type(_C().f)] = _reduce_method | ||
| dispatch_table[type(_C.h)] = _reduce_method |
There was a problem hiding this comment.
I prefer to keep the register public API to keep a the public API of this module compatible with the API of the reduction module of multiprocessing.
There was a problem hiding this comment.
Maybe then modify register to allow use as decorator?
There was a problem hiding this comment.
I like most of the code style simplifications in this PR to make the code base more Python-3 idiomatic, in particular, w.r.t. all that is related to bytes to str decoding. However there are still unrelated changes that deserve their own separate PR (in particular the packaging dependency).
| max_nfds = resource.getrlimit(resource.RLIMIT_NOFILE)[0] | ||
| open_fds = {fd for fd in range(3, max_nfds)} | ||
| open_fds.add(0) | ||
| open_fds = {*range(max_nfds)} |
There was a problem hiding this comment.
Note to other reviewers: 1 and 2 are part of keep_fds so will never be closed by the subsequent lines. Hence this code simplification should be harmless.
| python_requires='>=3.7', | ||
| install_requires=['cloudpickle'], | ||
| tests_require=['pytest', 'psutil'], | ||
| tests_require=['packaging', 'pytest', 'psutil'], |
There was a problem hiding this comment.
Adding a new dependency deserves a dedicated PR with its own changelog entry.
This will make it harder to vendor loky inside joblib. But maybe we should stop vendoring, or add the packaging dependency to joblib as well, but this discussion should happen outside of this PR.
There was a problem hiding this comment.
Actually it's just for the tests, so this is fine.
|
I isolated the |
Co-authored-by: Olivier Grisel <[email protected]>
|
All the changes we wanted to merge have been merged in sub-PR. The remaining change changes the public API of the reduction module too much and we intentionally wanted to keep it close to the original multiprocessing module. Note that once we drop support for Python 3.8 we will probably to clean up more backport code from the |
7 participants
Duplicate for this as there's no activity for 1.5 years