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

remove deprecated code calls to IOLoop.make_current() #7240

Merged
merged 6 commits into from
Nov 9, 2022

Conversation

graingert
Copy link
Member

@graingert graingert commented Nov 2, 2022

move utils_test.pristine_loop into test_client.py - it's only used in test_client, and requires calling make_current
remove IOLoop.make_current() from Nanny._run

refs #6784
Closes #xxxx

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

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 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 23m 46s ⏱️ + 12m 30s
  3 169 tests +    2    3 085 ✔️ +  15    84 💤  - 11  0  - 2 
23 448 runs  +651  22 547 ✔️ +701  901 💤  - 45  0  - 5 

Results for commit 84b32f2. ± Comparison against base commit c137ac0.

♻️ This comment has been updated with latest results.

@graingert graingert force-pushed the remove-deprecated-make-current branch from fa10314 to 90961fb Compare November 2, 2022 12:54
@graingert graingert mentioned this pull request Nov 2, 2022
2 tasks
@graingert graingert force-pushed the remove-deprecated-make-current branch from 90961fb to 84b9384 Compare November 2, 2022 13:43
@graingert graingert marked this pull request as ready for review November 2, 2022 16:06
distributed/nanny.py Outdated Show resolved Hide resolved
Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

This PR generally looks good to me. I have one question that I would like answered before giving this a thumbs-up.

distributed/nanny.py Outdated Show resolved Hide resolved
distributed/nanny.py Outdated Show resolved Hide resolved
stack.callback(thread.join, timeout=2)
async with worker:
failure_type = None

try:
assert worker.address
except ValueError:
Copy link
Member

Choose a reason for hiding this comment

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

This is outside of the changes made in this PR, but do you have an idea why the ValueError would pop up here and why it's okay to ignore it?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's this ValueError

raise ValueError("cannot get address of non-running Server")
not sure it's safe to ignore tbh

Copy link
Member

Choose a reason for hiding this comment

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

@fjetter: Do you happen to know whether we should keep ignoring this error?

Co-authored-by: Hendrik Makait <hendrik.makait@gmail.com>
distributed/nanny.py Outdated Show resolved Hide resolved
distributed/nanny.py Outdated Show resolved Hide resolved
@hendrikmakait hendrikmakait self-requested a review November 7, 2022 14:08
Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @graingert!

@graingert graingert merged commit 88515db into dask:main Nov 9, 2022
@graingert graingert deleted the remove-deprecated-make-current branch November 9, 2022 12:05
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.

2 participants