Automatically scatter large arguments in joblib connector #2020
Conversation
|
@TomAugspurger if you have a chance it would be great to update the docker image of your pangeo instance to test nested parallelism with this new feature (this requires upgrading the joblib branch too). |
|
I ran some tests on @TomAugspurger's pangeo instance with 30 workers (randomized search for a random forests) and everything looks fine: the CPUs get saturated and there is very little white space in the task stream view of the diagnostics page of the dask scheduler. |
|
I also checked that all the futures are garbage collected by the end of the grid search: |
| return False | ||
|
|
||
|
|
||
| class _WeakKeyDictionary: |
There was a problem hiding this comment.
Any reason not to subclass from MutableMapping, which would turn this into a full dict interface?
There was a problem hiding this comment.
I would need to implement __delitem__ (easy) and __iter__ not as simple but we have no use for those. So I don't think it's worth it.
distributed/joblib.py
Outdated
| if f is None and arg_id in self.data_futures: | ||
| f = self.data_futures[arg_id] | ||
|
|
||
| if f is None and call_data_futures is not None: |
There was a problem hiding this comment.
Should this be elif f is None... instead?
distributed/joblib.py
Outdated
| try: | ||
| f = call_data_futures[arg] | ||
| except KeyError: | ||
| if (call_data_futures is not None |
There was a problem hiding this comment.
call_data_futures will always be not None due to the if check above, this check is not needed.
distributed/joblib.py
Outdated
| f = call_data_futures[arg] | ||
| except KeyError: | ||
| if (call_data_futures is not None | ||
| and is_weakrefable(arg) |
There was a problem hiding this comment.
IIUC, _WeakKeyDictionary can handle non-weakrefable objects, so this check is not needed?
There was a problem hiding this comment.
Actually, it's an oversight on my side when implementing the _WeakKeyDictionary. I don't think it's possible to safely handle non-weakrefable objects like dict instances: if the id of a recently collected dict argument is reused we will compute the wrong thing. I will make the _WeakKeyDictionary refuse to accept non-weakrefable keys.
distributed/joblib.py
Outdated
| # The explicit call to clear is required to break a cycling reference | ||
| # to the futures. | ||
| self.call_data_futures.clear() | ||
| self.call_data_futures = None |
There was a problem hiding this comment.
Why set this to None? If call_data_futures always exists and is just cleared after running, then you never need to check if it's None below.
|
@jcrist I think I addressed your comments. Let's see of the CI likes it. |
|
I think the travis failure on |
|
Yeah, it looks like a couple of intermittent failures have crept in.
Several PRs are failing today.
…On Fri, Jun 1, 2018 at 5:38 PM, Olivier Grisel ***@***.***> wrote:
I think the travis failure on test_administration is unrelated to this PR.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2020 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASszLCkMX7d8FpBlSe8Vqu1F-uZvyX_ks5t4bRUgaJpZM4UV2gc>
.
|
|
CI seems green now. |
|
I think @jcrist gave a +1 in person. Merging this now. |
This requires: joblib/joblib#689 to work efficiently but should not crash or degrade performance when used with an older joblib.