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

Make ShuffleSchedulerExtension.remove_worker more robust #7921

Merged
merged 2 commits into from Jun 16, 2023

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Jun 15, 2023

If processing the transactions causes a task to get released, this removes the shuffle from self.states. Therefore, we must process them outside of the loop.

Under normal circumstances, remove_worker does not cause tasks to transition to released, so writing a test for this is hard. However, it can happen if you really screw with your cluster:

Traceback (most recent call last):
  File "/opt/coiled/env/lib/python3.9/site-packages/distributed/scheduler.py", line 5057, in remove_worker
    result = plugin.remove_worker(scheduler=self, worker=address)
  File "/opt/coiled/env/lib/python3.9/site-packages/distributed/shuffle/_scheduler_extension.py", line 297, in remove_worker
    for shuffle_id, shuffle in self.states.items():
RuntimeError: dictionary changed size during iteration
  • Tests added / passed
  • Passes pre-commit run --all-files

@github-actions
Copy link
Contributor

Unit Test Results

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

       20 files  +       2         20 suites  +2   12h 26m 51s ⏱️ + 2h 15m 18s
  3 681 tests ±       0    3 568 ✔️ +       7     108 💤  -   10  5 +3 
35 600 runs  +3 644  33 831 ✔️ +3 505  1 764 💤 +136  5 +3 

For more details on these failures, see this check.

Results for commit 1088a9d. ± Comparison against base commit 4d8dbad.

@fjetter fjetter merged commit c4b36ab into dask:main Jun 16, 2023
20 of 27 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.

None yet

2 participants