Merge global annotations on the client#4691
Conversation
fjetter
left a comment
There was a problem hiding this comment.
A few nitpicks but overall LGTM
| if not isinstance(v, (list, tuple, set)): | ||
| v = [v] |
There was a problem hiding this comment.
nitpick: We should have enough control about the other end to ensure this is always a tuple, don't we? I'm wondering if this is smth we rather want to do on the other end, i.e. keep code and types clean in this function since it is already quite large and complicated
There was a problem hiding this comment.
We can do it when we pack the annotations but I am not sure that is a good idea. The annotation packing is very generic and doesn't know anything about the meaning of the annotations.
There was a problem hiding this comment.
Note that some checking is already done client-side: https://github.com/dask/dask/blob/main/dask/base.py#L91-L102
There was a problem hiding this comment.
True, but it is only when we unpack/expand the annotation that we know if the callable is indeed returning a list of workers.
|
Also cc'ing @sjperkins for visibility |
…_global_annotations_on_client
|
I just merged the current |
This change depend on dask/distributed#4691
|
@jrbourbeau I am now doing the merging when packing layers, which should fix the issue where global annotations would persist. |
|
@jrbourbeau, the CI test failing is |
|
Apologies for the delayed reply. The |
In that case, I think this PR and dask/dask#7565 are ready to be merged :) |
jrbourbeau
left a comment
There was a problem hiding this comment.
Thanks @madsbk! Apologies for the delay in getting this merged
Note that merging in dask/dask#7565 along with this PR will cause tests to pass
Fixes #4551 by merging global annotations on when packing layers on the client.
This must be merged together with dask/dask#7565
Closes #4692
black distributed/flake8 distributed/isort distributed