Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing __hash__ and __eq__ for BatchedSend #4273

Closed
jakirkham opened this issue Nov 24, 2020 · 5 comments · Fixed by #4275
Closed

Implementing __hash__ and __eq__ for BatchedSend #4273

jakirkham opened this issue Nov 24, 2020 · 5 comments · Fixed by #4275

Comments

@jakirkham
Copy link
Member

In Scheduler.report we collect a set of comms for the Clients. Looking more closely at the contents of self.client_comms, it appears to just contain BatchedSend objects. Putting these objects in a set will wind up using __eq__ and __hash__. Neither of these appear to be defined by BatchedSend. As a result a good chunk of time is spent hashing these objects by some fallback mechanism before adding them to the set in Scheduler.report. Would be good to have implementations of __eq__ and __hash__ for BatchedSend to avoid this issue.

@jakirkham
Copy link
Member Author

Alternatively we might investigate another way to write this code so it doesn't rely on hashing and comparing BatchedSend objects.

@jakirkham
Copy link
Member Author

Maybe another way to look at this is, when would a BatchedSend object be used for more than 1 Client?

cc @mrocklin (in case you have thoughts here 😉)

@mrocklin
Copy link
Member

It seems reasonable that there would be only one BatchedSend per Client. In this situation I would try implementing this (hopefully it's cheap to do), and then seeing if any tests fail. If no tests fail then we're good to go.

@mrocklin
Copy link
Member

We can also ask @jrbourbeau to do this if y'all are less familiar here. James is fairly adept at these sorts of changes.

@jakirkham
Copy link
Member Author

Found a good way to do this with a dict ( #4275 ), which should get us past this issue. It may be possible to simplify this further to a list, but that might not matter much.

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 a pull request may close this issue.

2 participants