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.

Sign up for GitHub

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

Jump to bottom

Make p2p shuffle submodules private #7186

Merged
merged 5 commits into from
Oct 26, 2022
Merged

Make p2p shuffle submodules private #7186

merged 5 commits into from
Oct 26, 2022

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented Oct 25, 2022

Builds on #7185

This makes all shuffle p2p modules "private" and exposes only the extensions (They are imported by the scheduler) and the actual graph generator.

With #7180 this should also include the layer

@fjetter fjetter changed the title Shuffle ext refactor 2 Make p2p shuffle submodules private Oct 25, 2022
@fjetter
Copy link
Member Author

fjetter commented Oct 25, 2022

cc @wence-

@fjetter fjetter force-pushed the shuffle_ext_refactor_2 branch 3 times, most recently from 400cc5a to f8ed836 Compare October 25, 2022 14:08
jrbourbeau
jrbourbeau reviewed Oct 25, 2022
View reviewed changes
Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice -- I haven't taken a detailed look, but in general I like the idea of having private modules instead of a lot of private functions (where it makes sense)

@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2022 •
edited
Loading

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  +       5         15 suites  +5   6h 23m 24s ⏱️ + 2h 41m 40s
  3 157 tests +       1    3 065 ✔️ +     12    84 💤  -   13    8 ❌ +2 
23 356 runs  +8 209  22 431 ✔️ +7 857  900 💤 +351  25 ❌ +1 

For more details on these failures, see this check.

Results for commit ed9e843. ± Comparison against base commit 3567d1f.

♻️ This comment has been updated with latest results.

@fjetter fjetter self-assigned this Oct 26, 2022
@fjetter fjetter mentioned this pull request Oct 26, 2022
Closed
fjetter
fjetter commented Oct 26, 2022
View reviewed changes
distributed/shuffle/_shuffle_extension.py
t = pa.Table.from_pandas(df)
# FIXME: If we do not preserve the index something is corrupting the
# bytestream such that it cannot be deserialized anymore
t = pa.Table.from_pandas(df, preserve_index=True)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the default. Now it's just explicit

@fjetter
Copy link
Member Author

fjetter commented Oct 26, 2022

test_bad_disk is already a known problem. Currently working on it but won't be fixing inthis PR.

Other failures are the websocket failures that happen everywhere right now

@fjetter fjetter merged commit 5dccad4 into dask:main Oct 26, 2022
@fjetter fjetter deleted the shuffle_ext_refactor_2 branch November 10, 2022 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrbourbeau jrbourbeau jrbourbeau left review comments

Assignees

@fjetter fjetter

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fjetter @jrbourbeau