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

Introduce PreloadManager to handle failures in preload setup/teardown #8078

Merged
merged 3 commits into from Aug 16, 2023

Conversation

hendrikmakait
Copy link
Member

This PR introduces a PreloadManager which deduplicates preload handling code from the Client, Nanny, Scheduler, and Worker. The PreloadManager also ensures that errors on preload.{start|teardown} don't raise.

  • Tests added / passed
  • Passes pre-commit run --all-files

@@ -1313,8 +1313,7 @@ async def _start(self, timeout=no_default, **kwargs):
for topic, handler in Client._default_event_handlers.items():
self.subscribe_topic(topic, handler)

for preload in self.preloads:
await preload.start()
await self.preloads.start()
Copy link
Member Author

Choose a reason for hiding this comment

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

There's an argument to be made that preload.start should raise here. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed; I don't think it's healthy to semi-quietly go online nonetheless.

distributed/client.py Show resolved Hide resolved
distributed/preloading.py Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

Unit Test Results

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

       21 files  +       1         21 suites  +1   10h 49m 43s ⏱️ - 29m 17s
  3 772 tests +     18    3 657 ✔️ +     12     109 💤 +  3  6 +4 
35 207 runs   - 1 107  33 439 ✔️  - 1 121  1 761 💤 +12  7 +3 

For more details on these failures, see this check.

Results for commit 420ac66. ± Comparison against base commit 9255987.

This pull request removes 14 and adds 32 tests. Note that renamed tests count towards both.
distributed.tests.test_cancelled_state ‑ test_execute_preamble_early_cancel[executing-False-deserialize_task]
distributed.tests.test_cancelled_state ‑ test_execute_preamble_early_cancel[executing-False-execute]
distributed.tests.test_cancelled_state ‑ test_execute_preamble_early_cancel[executing-True-deserialize_task]
distributed.tests.test_cancelled_state ‑ test_execute_preamble_early_cancel[executing-True-execute]
distributed.tests.test_cancelled_state ‑ test_execute_preamble_early_cancel[resumed-False-deserialize_task]
distributed.tests.test_cancelled_state ‑ test_execute_preamble_early_cancel[resumed-False-execute]
distributed.tests.test_cancelled_state ‑ test_execute_preamble_early_cancel[resumed-True-deserialize_task]
distributed.tests.test_cancelled_state ‑ test_execute_preamble_early_cancel[resumed-True-execute]
distributed.tests.test_preload ‑ test_failure_doesnt_crash
distributed.tests.test_scheduler ‑ test_dumps_task
…
distributed.cli.tests.test_dask_worker.test_listen_address_ipv6[tcp:..[ ‑ 1]:---nanny]
distributed.cli.tests.test_dask_worker.test_listen_address_ipv6[tcp:..[ ‑ 1]:---no-nanny]
distributed.diagnostics.tests.test_memray
distributed.diagnostics.tests.test_memray ‑ test_basic_integration_scheduler
distributed.diagnostics.tests.test_memray ‑ test_basic_integration_scheduler_report_args[False]
distributed.diagnostics.tests.test_memray ‑ test_basic_integration_scheduler_report_args[report_args0]
distributed.diagnostics.tests.test_memray ‑ test_basic_integration_workers[1]
distributed.diagnostics.tests.test_memray ‑ test_basic_integration_workers[False]
distributed.diagnostics.tests.test_memray ‑ test_basic_integration_workers[True]
distributed.diagnostics.tests.test_memray ‑ test_basic_integration_workers_report_args[False]
…

♻️ This comment has been updated with latest results.

@hendrikmakait hendrikmakait changed the title Introduce PreloadManager to handle preloads in a canonical way. Introduce PreloadManager to handle failures in preload setup/teardown Aug 8, 2023
@hendrikmakait hendrikmakait marked this pull request as ready for review August 8, 2023 07:25
Copy link
Collaborator

@crusaderky crusaderky left a comment

Choose a reason for hiding this comment

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

LGTM. I suggest to leave the change to crash in case of failing preload start to a separate PR.

Co-authored-by: crusaderky <crusaderky@gmail.com>
@hendrikmakait hendrikmakait merged commit 568ee7e into dask:main Aug 16, 2023
22 of 28 checks passed
@hendrikmakait hendrikmakait deleted the preload-manager branch August 16, 2023 08:40
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