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

AMM: Don't schedule tasks to paused workers #5431

Merged
merged 4 commits into from Oct 20, 2021

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Oct 15, 2021

In scope

  • Prevent Scheduler from assigning new tasks to workers that are paused or closing_gracefully
  • Fix a bunch of bugs in the Worker.status -> WorkerState.status sync
  • Treat closing_gracefully the same as running in most cases

Follow-ups

Out of scope

@crusaderky crusaderky self-assigned this Oct 15, 2021
@crusaderky crusaderky changed the title WIP AMM: Avoid workers in paused and closing_gracefully status Avoid workers in paused and closing_gracefully status Oct 18, 2021
@crusaderky crusaderky force-pushed the AMM/avoid_paused branch 2 times, most recently from 05a12e5 to 8bf6b0e Compare October 18, 2021 21:41
@crusaderky crusaderky changed the title Avoid workers in paused and closing_gracefully status AMM: Avoid workers in paused and closing_gracefully status Oct 18, 2021
@crusaderky crusaderky force-pushed the AMM/avoid_paused branch 2 times, most recently from 253d397 to 4c55e9b Compare October 19, 2021 11:03
@crusaderky crusaderky changed the title AMM: Avoid workers in paused and closing_gracefully status AMM: Don't schedule tasks to paused workers Oct 19, 2021
@crusaderky crusaderky marked this pull request as ready for review October 19, 2021 12:20
@crusaderky
Copy link
Collaborator Author

I further broke down this PR. It's now ready for review.

distributed/scheduler.py Show resolved Hide resolved
Comment on lines +5487 to +5490
client_msgs: dict = {}
worker_msgs: dict = {}
parent._transitions(recs, client_msgs, worker_msgs)
self.send_all(client_msgs, worker_msgs)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
client_msgs: dict = {}
worker_msgs: dict = {}
parent._transitions(recs, client_msgs, worker_msgs)
self.send_all(client_msgs, worker_msgs)
self.transitions(recs)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aren't client_msgs and worker_msgs filled in place by _transitions()? I fell for this already before.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if you're using the underscored method, you'll need to call the send yourself. If you use the non-underscored one this will be done. You basically just copied what the non-underscored method does,

def transitions(self, recommendations: dict):
"""Process transitions until none are left
This includes feedback from previous transitions and continues until we
reach a steady state
"""
parent: SchedulerState = cast(SchedulerState, self)
client_msgs: dict = {}
worker_msgs: dict = {}
parent._transitions(recommendations, client_msgs, worker_msgs)
self.send_all(client_msgs, worker_msgs)

distributed/tests/test_scheduler.py Show resolved Hide resolved
distributed/worker.py Outdated Show resolved Hide resolved
distributed/worker.py Show resolved Hide resolved
distributed/worker.py Outdated Show resolved Hide resolved
crusaderky and others added 3 commits October 20, 2021 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants