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

Do not rely on logging for SubprocessCluster #8398

Merged
merged 1 commit into from Dec 13, 2023

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Dec 12, 2023

Partially addresses #8392, #8393

This PR relies on the scheduler_file to propagate the scheduler address to the worker.

Note:
Conceptually, the same fix can be applied to the SSHCluster but things get mildly more complicated because one might have to deal with different file systems for the client and the scheduler.

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

Copy link
Contributor

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   11h 52m 36s ⏱️ + 14m 41s
  3 935 tests +  1    3 819 ✔️  -   2     110 💤 ±0    6 +3 
49 501 runs  +31  47 191 ✔️ +29  2 293 💤  - 3  17 +5 

For more details on these failures, see this check.

Results for commit 2c49f6b. ± Comparison against base commit c408b6a.

@hendrikmakait hendrikmakait merged commit f2e1e51 into dask:main Dec 13, 2023
21 of 35 checks passed
@hendrikmakait hendrikmakait deleted the logless-subprocess branch December 13, 2023 13:48
@crusaderky
Copy link
Collaborator

crusaderky commented Dec 14, 2023

with new_config_file(
{"distributed": {"logging": {"distributed": logging.CRITICAL + 1}}}
):
await asyncio.wait_for(_start(), timeout=2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand what's the purpose of this super-short timeout, and why the timeout integrated in gen_test is not what we want in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote the test before the fix and didn't want to wait the entire 30 seconds while fixing.

@crusaderky
Copy link
Collaborator

Attempt fix on #8413

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

3 participants