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

Set Client.as_current when entering ctx #6527

Merged
merged 11 commits into from
Jul 7, 2023

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented Jun 8, 2022

Most of the time we actually prefer using the current client and not the default client. Whenever we're using the client in a ctx manager, we should set the current client

TODO: tests

cc @graingert

@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2022

Unit Test Results

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

       15 files  ±0         15 suites  ±0   6h 33m 35s ⏱️ + 8m 3s
  2 853 tests ±0    2 738 ✔️  - 30    82 💤 +1  30 +26  3 🔥 +3 
21 137 runs  ±0  20 157 ✔️  - 34  944 💤 +2  33 +29  3 🔥 +3 

For more details on these failures and errors, see this check.

Results for commit e7337a4. ± Comparison against base commit ea2c80f.

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

github-actions bot commented Dec 13, 2022

Unit Test Results

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

       20 files         20 suites   11h 5m 53s ⏱️
  3 708 tests   3 600 ✔️    106 💤 2
35 866 runs  34 115 ✔️ 1 749 💤 2

For more details on these failures, see this check.

Results for commit 2538ac1.

♻️ This comment has been updated with latest results.

@fjetter fjetter force-pushed the use_current_instead_of_default branch from 36a50d7 to 5046fb9 Compare December 14, 2022 14:15
@fjetter fjetter changed the title Prefer current over default Client Set Client.as_current when entering ctx Dec 14, 2022
Comment on lines +3370 to +3371
c = await Client(s.address, asynchronous=True)
try:
Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out we can only set current when entering ctx when set_as_default is also set. Otherwise this breaks tests like this.

That also makes sense considering that default_client and Client.current are effectively aliases

@fjetter fjetter marked this pull request as ready for review December 14, 2022 14:17
@crusaderky crusaderky self-requested a review July 5, 2023 19:28
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.

Just a trivial nitpick

distributed/tests/test_client.py Show resolved Hide resolved
distributed/tests/test_client.py Show resolved Hide resolved
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