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

Delegate wait_for_workers to cluster instances only when implemented #8441

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

consideRatio
Copy link
Contributor

@consideRatio consideRatio commented Jan 4, 2024

In #6700 the client started delegating a wait_for_workers call to a cluster instance if one was set, but that forced all cluster instances to implement such method. This PR makes it optional to implement such method.

This change is motivated by dask/dask-gateway#782 thanks to guidance from @TomAugspurger in a comment there. There a cluster instance was available, but wait_for_workers hasn't yet been implemented by the class defined by the dask/dask-gateway project.

  • Tests added / passed
    I've for the moment opted to not try to develop a test for this small fix without further input.
  • Passes pre-commit run --all-files

@consideRatio consideRatio requested a review from fjetter as a code owner January 4, 2024 11:03
@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

@consideRatio consideRatio force-pushed the pr/delegate-only-when-supported branch from 5d0466b to d6f2234 Compare January 4, 2024 11:03
@consideRatio
Copy link
Contributor Author

@jacobtomlinson as a reviewer of #6700 with relevant context I figure I'll give you a ping, no pressure though! Thanks for your work on this project!!!

Copy link
Contributor

github-actions bot commented Jan 4, 2024

Unit Test Results

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

    27 files  ± 0      27 suites  ±0   10h 2m 41s ⏱️ + 18m 26s
 3 951 tests ± 0   3 841 ✅ + 3    109 💤 ±0  1 ❌  - 3 
49 696 runs  +18  47 406 ✅ +27  2 289 💤  - 6  1 ❌  - 3 

For more details on these failures, see this check.

Results for commit 1fc1897. ± Comparison against base commit 7562f9c.

♻️ This comment has been updated with latest results.

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

This looks like a reasonable change.

It does lead me down the path of thinking that we should provide a cluster Protocol type that all cluster managers are expected to conform to though. Having implementations miss out methods and require us to check for them all over the place isn't great for maintainability.

But given this method requirement was added recently I think it's fine to add this now.

@fjetter
Copy link
Member

fjetter commented Jan 9, 2024

It does lead me down the path of thinking that we should provide a cluster Protocol type

I would rather suggest a proper super type but that's a conversation for another ticket.


CI seems to be failing due to some version conflict. That's weird. @consideRatio would you mind rebasing/merging main to see if this disappears?

@consideRatio consideRatio force-pushed the pr/delegate-only-when-supported branch from d6f2234 to d0bc4ca Compare January 9, 2024 16:53
@consideRatio
Copy link
Contributor Author

consideRatio commented Jan 9, 2024

Thank you for spending time reviewing this @jacobtomlinson @fjetter!!!


CI seems to be failing due to some version conflict.

Yeah =/ I ignored the errors as they seemed unrelated.

That's weird. @consideRatio would you mind rebasing/merging main to see if this disappears?

Rebased and force-pushed on latest main branch!

@consideRatio consideRatio force-pushed the pr/delegate-only-when-supported branch from d0bc4ca to 1fc1897 Compare January 9, 2024 16:56
@fjetter fjetter merged commit 964abea into dask:main Jan 10, 2024
31 of 33 checks passed
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.

4 participants