-
-
Notifications
You must be signed in to change notification settings - Fork 748
Avoid meta roundtrip in P2P shuffle #7895
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
Conversation
|
cc @wence-, @jrbourbeau |
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 20 files ± 0 20 suites ±0 10h 44m 12s ⏱️ - 1h 6m 57s For more details on these failures, see this check. Results for commit d8c9252. ± Comparison against base commit e31c864. This pull request removes 1 and adds 5 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
jrbourbeau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hendrikmakait -- I'm testing the dask/dask test suite against this PR over in dask/dask#10334
| @abc.abstractmethod | ||
| async def get_output_partition( | ||
| self, i: T_partition_id, key: str | ||
| self, i: T_partition_id, key: str, **kwargs: Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why allow a generic **kwargs instead of meta=? The code here looks functionally fine, but I have a slight preference for avoiding **kwargs if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no meta for array rechunking. The class hierarchy doesn't play out perfectly, we may want to get rid of it eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could still use meta=None instead of **kwargs though, no? (even if array re-chunking doesn't choose to use it for now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If people like that better, we can change it. I don't have strong opinions and plan to refactor this soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong opinions and plan to refactor this soon.
Makes sense - I don't have strong feelings either. Mostly just trying to digest the general code. That said, I am pretty interested to know about the refactoring plans.
FYI, we are currently experimenting with a decomposed version of P2PShuffleLayer (i.e. ShuffleTransfer(Blockwise) -> ShuffleBarrier(Layer) -> ShuffleUnpack(Blockwise)). I'd like to get feedback on the approach once dask/dask#10312 and a modified version of #7743 get in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been meaning to check those PRs out, I'll block some time for it next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're now using meta instead of **kwargs.
jrbourbeau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hendrikmakait
Closes dask/dask#10335
pre-commit run --all-files