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

Move wait_for_signals to private module and deprecate distributed.cli.utils #6367

Merged
merged 5 commits into from
May 20, 2022

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented May 18, 2022

Follow-up to #6366

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

@hendrikmakait hendrikmakait changed the title Move wait_for_signals and deprecate distributed.cli.utils Move wait_for_signals to private module and deprecate distributed.cli.utils May 18, 2022
from tornado.ioloop import IOLoop

warnings.warn(
"the distributed.cli.utils module is deprecated", DeprecationWarning, stacklevel=2
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this warning could point folks to an issue or some advice on migration. Both of the functions available in this package are no longer going to be available outside distributed. So projects that depend on them will need to some something else.

Copy link
Member

Choose a reason for hiding this comment

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

Also shouldn't this be calling from distributed._signals import wait_for_signals to avoid breakages?

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 should be no current dependencies apart from dask_scheduler.py and dask_worker.py unless someone has been very fast in following up with this change. I would not import wait_for_signals in distributed.cli.utils to keep it private.

Regarding the advice on migration: I'll try to come up with something, maybe @graingert has some good ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Ah right I see it's a new method! That makes sense.

@github-actions
Copy link
Contributor

Unit Test Results

       15 files  ±0         15 suites  ±0   7h 7m 58s ⏱️ + 10m 33s
  2 801 tests ±0    2 722 ✔️ ±0    78 💤 ±0  1 ±0 
20 770 runs  ±0  19 849 ✔️ ±0  920 💤 ±0  1 ±0 

For more details on these failures, see this check.

Results for commit b21314b. ± Comparison against base commit ff94776.

@jacobtomlinson jacobtomlinson merged commit fb3589c into dask:main May 20, 2022
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.

3 participants