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

Replace legacy commands with 'dask worker' and 'dask scheduler'. #399

Closed
wants to merge 2 commits into from

Conversation

wilson
Copy link

@wilson wilson commented Feb 2, 2023

Hi there.
I'm not 100% the change I made re: GPU workers is correct.
I believe it to be, but I haven't used it in anger.
Looking at the dask-cuda project, it does look like dask-cuda-worker is deprecated just like dask-worker.

Anyway, this PR fixes a pair of warnings:

distributed/cli/dask_scheduler.py:140: FutureWarning: dask-scheduler is deprecated and will be removed in a future release; use `dask scheduler` 
distributed/cli/dask_worker.py:264: FutureWarning: dask-worker is deprecated and will be removed in a future release; use `dask worker` instead

In doing this, I realized that the worker_command option is not used anywhere, so this PR also removes the remaining references to it.

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

This look great thanks.

In dask-cuda we replaced dask-cuda-worker with dask cuda worker. That's all that should be changed. Sorry you went down the resources path, that's not needed.

@wilson
Copy link
Author

wilson commented Feb 2, 2023

This look great thanks.

In dask-cuda we replaced dask-cuda-worker with dask cuda worker. That's all that should be changed. Sorry you went down the resources path, that's not needed.

Oh, gotcha; I'll change it up.

@wilson
Copy link
Author

wilson commented Feb 2, 2023

OK, rebased off main and all looks green.

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Thanks. Couple of questions.

@@ -434,24 +435,25 @@ def __init__(
self._mem = mem
self._gpu = gpu
self._nthreads = nthreads
_command = [
"dask",
"cuda" if self._gpu else None,
Copy link
Member

Choose a reason for hiding this comment

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

I'm apprehensive about having a None in this string. Have you tested that this works as expected?

Copy link
Author

Choose a reason for hiding this comment

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

There's a None but it gets filtered out before we use it.

Copy link
Member

Choose a reason for hiding this comment

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

I found the introduction and filtering of None hard to parse when reviewing this.

I think it would be more readable to have something like

if self._gpu:
    _command = ["dask", "cuda", "worker"]
else:
    _command = ["dask", "worker"]
_command += [OTHERARGS...]

@@ -434,24 +435,25 @@ def __init__(
self._mem = mem
self._gpu = gpu
self._nthreads = nthreads
_command = [
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why create this variable?

e
for e in [
"dask",
"cuda" if self._worker_gpu else None,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Author

Choose a reason for hiding this comment

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

To both the above, just so it was more visible that we're filtering Nones out.
That seemed like the easiest way to have a variable number of arguments in the command.

@jacobtomlinson
Copy link
Member

There's been no activity here for a while so I'm going to close this PR out. @wilson if you have the desire to pick this up I'd happily give it another review.

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