Skip to content

Conversation

@jakirkham
Copy link
Member

It seems a few tests specified the Worker's name and address with ints. However Scheduler.coerce_address (used in the tests) doesn't actually allow ints. Also the WorkerState's docs for address imply it is a str, which is also said of the nanny address. Here we attempt to resolve this discrepancy by fixing these tests to use strs instead of ints.

@mrocklin
Copy link
Member

mrocklin commented Dec 2, 2020

Worker names can technically be ints. This comes up with LocalCluster I suspect. We can change this to make things always stringy.

@mrocklin
Copy link
Member

mrocklin commented Dec 2, 2020

@jacobtomlinson I'm not sure if you have time these days, but if so you might be well positioned to think about this problem

@jacobtomlinson
Copy link
Member

In SpecCluster worker names are ints. When creating new workers they are also enumerated to find the next number.

def new_worker_spec(self):
"""Return name and spec for the next worker
Returns
-------
d: dict mapping names to worker specs
See Also
--------
scale
"""
new_worker_name = self._new_worker_name(self._i)
while new_worker_name in self.worker_spec:
self._i += 1
new_worker_name = self._new_worker_name(self._i)
return {new_worker_name: self.new_spec}

@mrocklin do these numbers have any semantic meaning? Or are we just looking for unique names here? If its the latter perhaps this logic should be replaced with uuids?

@mrocklin
Copy link
Member

mrocklin commented Dec 2, 2020

They need to be unique.  It's nice if they're easy to call out, as in

client.submit(func, x, workers=[0, 1, 2, 3])
client.submit(func, y, workers=[4, 5, 6, 7])

I would be against UUIDs for this reason. My guess/hope is that there is some way to keep using ints in the spec, but to stringify names once they get to the worker

@jacobtomlinson
Copy link
Member

It's nice if they're easy to call out

I'm curious how often this comes up in practice. I've never used this pattern myself.

Also I wonder if this could also be achieved more robustly with something like

worker_names = [w for w in cluster.workers]
client.submit(func, x, workers=worker_names[:4])
client.submit(func, y, workers=worker_names[4:8])

@jakirkham
Copy link
Member Author

Interesting. Thanks for letting me know this is an expected case. Wouldn't have assumed that initially.

Using UUIDs or stringifying ints seem fine to me.

Mainly am interested in something with a fixed type here as opposed to several. The latter makes it harder to type things under-the-hood.

@mrocklin
Copy link
Member

mrocklin commented Dec 2, 2020

I'm curious how often this comes up in practice. I've never used this pattern myself.

It's not super common, but I've used it from time to time. If you're familiar with MPI style programming it's also pretty natural.

Also I wonder if this could also be achieved more robustly with something like

Yes, that would be more robust.

I'm inclined towards stringification personally. It's also the least disruptive. I'm also personally unlikely to do this work short term though. If others who are going to do this work have strong thoughts then I'm all for those thoughts.

@jakirkham jakirkham changed the title Use str for Worker name & address in tests [WIP] Use str for Worker name & address in tests Dec 2, 2020
@jakirkham jakirkham marked this pull request as draft December 2, 2020 22:30
@jakirkham
Copy link
Member Author

jakirkham commented Dec 3, 2020

Broke out the address bit in PR ( #4312 ) as it seems like we want that fix anyways.

Edit: NVM that's of no use on its own.

Base automatically changed from master to main March 8, 2021 19:04
@jakirkham jakirkham closed this Apr 9, 2022
@jakirkham jakirkham deleted the fix_wrk_name_add_tst branch April 9, 2022 02:53
@jakirkham
Copy link
Member Author

Going to abandon this. Not going to open a new issue since issue ( #5743 ) already encapsulates this issue well.

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.

3 participants