Deprecate `--nprocs` option for `dask-worker` CLI by bryanwweber · Pull Request #5641 · dask/distributed

bryanwweber · 2022-01-06T17:20:12Z

Per #2471, nprocs is potentially confusing and should be replaced by num-workers.

Closes Remove --nprocs keyword from dask-worker command? #2471
Tests added / passed
Passes pre-commit run --all-files

Per #2471, nprocs is confusing and should be replaced by num-workers.

GPUtester · 2022-01-06T17:20:14Z

Can one of the admins verify this patch?

jrbourbeau · 2022-01-06T17:21:02Z

add to allowlist

jrbourbeau · 2022-01-06T17:23:56Z

cc @guillaumeeb for visibility (as this change will impact dask-jobqueue)

jrbourbeau

Thanks @bryanwweber! I know this is still a draft PR, but I still wanted to leave some early feedback.

Also, unfortunately distributed has several known flaky tests at the moment. The test failures popping up here are all unrelated to the changes in this PR.

distributed/cli/dask_worker.py

distributed/cli/tests/test_dask_worker.py

distributed/cli/dask_worker.py

bryanwweber · 2022-01-06T22:07:12Z

@jrbourbeau Sure, thanks for the review! I'll take a look tomorrow. I also noticed that edge case, but wanted to follow the pre-existing convention for the bokeh/dashboard options. I'll be happy to update.

I just pushed 3 more commits to catch the few other places where nprocs was used. I'm not sure if I should also update the old_ssh interface, so I put those changes into a separate commit to make it easy to drop.

guillaumeeb · 2022-01-11T13:46:13Z

cc @guillaumeeb for visibility (as this change will impact dask-jobqueue)

Thanks @jrbourbeau, I'll try to follow the evolution here!

jrbourbeau · 2022-01-12T15:58:21Z

@jacobtomlinson if you get a moment, would you mind taking a look at the SSHCluster-related changes here?

bryanwweber · 2022-01-12T16:14:14Z

@jrbourbeau I can't reproduce this test failure locally: https://github.com/dask/distributed/runs/4791743011?check_suite_focus=true#step:12:182 possibly because I'm on a mac and this is a Windows failure?

jacobtomlinson

The SSH changes look reasonable to me.

This will be breaking for old_ssh but perhaps we should think about removing that now anyway.

Further information

jacobtomlinson · 2022-01-12T16:39:40Z

Sorry for the confusion with the review dismissal. I'm happy with this change.

We should remove old_ssh at some point, but that requires migrating the CLI over to use it.

distributed/deploy/ssh.py

distributed/cli/tests/test_dask_ssh.py

distributed/deploy/tests/test_old_ssh.py

bryanwweber · 2022-01-18T20:13:24Z

@jrbourbeau I tried to use Client as we discussed to test dask_ssh but the test always times out. I wonder if this is the cause:

distributed/distributed/cli/dask_ssh.py

Lines 216 to 217 in f4451b6

    
           # Monitor the output of remote processes.  This blocks until the user issues a KeyboardInterrupt. 
        
           c.monitor_remote_processes()

I suspect this might be why I needed to issue the SIGINT to get any output, if that function is blocking until SIGINT is sent?

graingert · 2022-01-19T10:49:32Z

@jrbourbeau I tried to use Client as we discussed to test dask_ssh but the test always times out. I wonder if this is the cause:

distributed/distributed/cli/dask_ssh.py

Lines 216 to 217 in f4451b6

# Monitor the output of remote processes. This blocks until the user issues a KeyboardInterrupt.

c.monitor_remote_processes()

I suspect this might be why I needed to issue the SIGINT to get any output, if that function is blocking until SIGINT is sent?

I suspect this is because the stdout stream is block-buffered by default (when non-interactive) you could set PYTHONUNBUFFERED if you want the output as soon as possible.

https://docs.python.org/3/library/sys.html#sys.stderr

bryanwweber · 2022-01-19T15:12:29Z

@graingert Thanks for the thought! I don't think that's the problem here, for a few reasons. The modified test code with Client looks like:

def test_ssh_cli_nprocs_renamed_to_num_workers(loop):
    num_workers = 2
    with popen(
        ["dask-ssh", f"--nprocs={num_workers}", "--nohost", "localhost"]
    ) as cluster:
        with Client("tcp://127.0.0.1:8786", loop=loop) as c:
            c.wait_for_workers(num_workers, timeout="15 seconds")

The timeout failure happens due to pytest-timeout, not because the Client has timed out. I'm running pytest as pytest -s distributed/cli/tests/test_dask_ssh.py::test_ssh_cli_nprocs_renamed_to_num_workers --timeout=30, and the timeout only happens after 30 seconds, not 15.
The -s flag to pytest should result in unbuffered output, if I understand correctly
I also tried modifying the bufsize kwarg to popen to set it to 0, which also did not result in any output
The modified test is not actually testing or waiting on any output to stdout or stderr, but it still times out

jsignell · 2022-01-19T15:29:55Z

Sorry to be super late to the conversation, but is there a good reason to use num_ rather than n or n_ as the prefix? And if there is, can we consolidate nthreads and n_workers to this prefix?

bryanwweber · 2022-01-19T16:57:36Z

Sorry to be super late to the conversation, but is there a good reason to use num_ rather than n or n_ as the prefix? And if there is, can we consolidate nthreads and n_workers to this prefix?

No problem @jsignell! num_workers was suggested in the linked issue, #2471 (comment). n_workers would be fine with me 😄

I believe nthreads and num_workers are orthogonal. nthreads sets the number of threads per process, and num_workers sets the number of processes. But I'm not totally sure about that last part, specifically whether num_workers sets the number of processes 😃

jsignell · 2022-01-19T18:36:49Z

Yeah, I just meant consolidate on a prefix :)

bryanwweber · 2022-01-20T13:44:50Z

Thanks @jsignell! Do you prefer nworkers and nthreads or with the underscore? The advantage of nworkers is that it won't require a deprecation cycle with nthreads, but my personal preference is for the underscores.

jsignell · 2022-01-20T22:13:44Z

Thanks @jsignell! Do you prefer nworkers and nthreads or with the underscore? The advantage of nworkers is that it won't require a deprecation cycle with nthreads, but my personal preference is for the underscores.

No preference. There is an n_workers already elsewhere (at least in LocalCluster, but possibly other places). Not sure if we should match or be different for greppability (probably match).

bryanwweber · 2022-01-26T21:52:10Z

Whoops, sorry @jacobtomlinson I hit the wrong button there. Feel free to dismiss the review request (or do a review if you want to 😄 )

bryanwweber · 2022-01-26T22:18:44Z

@jsignell Based on a conversation with @jrbourbeau we agreed that using --nworkers for CLI flags and n_workers in Python code would be the most self-consistent approach.

The client only has 5 seconds to connect to the scheduler by default, so it seems to sometimes time out on CI.

bryanwweber · 2022-01-27T16:06:33Z

@jrbourbeau I think I tracked down the SSH failures to the Client not having enough time on the CI machines to connect to the scheduler. I bumped that timeout, hopefully it will fix it 🤞

bryanwweber · 2022-01-27T17:11:21Z

@jrbourbeau The test failing on Ubuntu/3.9/no ci1 doesn't look like it's due to my change, but I can't quite tell. Do you have any insight? https://github.com/dask/distributed/runs/4968972824?check_suite_focus=true#step:12:1434 Otherwise, waiting on macOS like some other PRs

jrbourbeau

Thanks for all your efforts on this @bryanwweber. Agreed those test failures are unrelated to the changes here. I left a few final review comments, but this looks good overall

jrbourbeau · 2022-02-01T16:16:37Z

distributed/deploy/old_ssh.py

        nanny_port=None,
        remote_dask_worker="distributed.cli.dask_worker",
        local_directory=None,
+        **kwargs,


Adding **kwargs here will cause invalid keyword arguments to go unnoticed. For example, today a typo of memory_limt instead of memory_limit will raise an error, while with this change as is such a typo will be silently ignored.

Instead I'd rather us either keep an explicit nprocs= keyword around, but still raise a ValueError / FutureWarning accordingly, or do something like (below is pseudocode)

nprocs = kwargs.pop("nprocs") if kwargs: raise TypeError("...") <existing ValueError / FutureWarning logic>

jrbourbeau · 2022-02-01T16:17:58Z

distributed/deploy/old_ssh.py

+        elif n_workers is None:
+            n_workers = 1
+
+        self.n_workers = n_workers


Can we add a small self.nprocs property which redirects users to self.n_workers instead?

I'm happy to add this. However, setting nprocs or n_workers as an attribute (after __init__() runs) doesn't actually change anything about the cluster, unless the user subsequently runs add_worker(). For that matter, it's not all that clear to me what the behavior should be... do all the worker nodes need to be updated with the number of worker processes? Is there a method to do that (there doesn't seem to be from my quick read of the code)?

After an offline discussion, we decided that although these attributes are not necessarily meant to be set, a belt-and-suspenders approach to backward compatibility means we should add a property to warn instead of an AttributeError if nprocs is not defined. This is particularly true for the getting case

jrbourbeau · 2022-02-01T16:18:36Z

distributed/deploy/ssh.py

+            )
+            self.n_workers = self.kwargs.pop("nprocs")
+        else:
+            self.n_workers = self.kwargs.pop("n_workers", 1)


Similar comment here about adding a self.nprocs property redirecting to self.n_workers

Similar response as above 😄

jrbourbeau · 2022-02-01T16:19:41Z

distributed/deploy/ssh.py

+                "renamed to n_workers.",
+                FutureWarning,
+            )
+            self.n_workers = self.kwargs.pop("nprocs")


Not sure if this is needed or not

Suggested change

self.n_workers = self.kwargs.pop("nprocs")

self.n_workers = self.kwargs.pop("nprocs", 1)

It shouldn't be needed, but wouldn't hurt to add it.

github-actions · 2022-02-02T03:43:49Z

Unit Test Results

      18 files +      1       18 suites +1 9h 48m 14s ⏱️ + 20m 49s
  2 572 tests +    11   2 494 ✔️ +    15     78 💤 ±    0 0 ❌ - 4
23 014 runs +1 432 21 428 ✔️ +1 316 1 586 💤 +120 0 ❌ - 4

Results for commit 353d5ce. ± Comparison against base commit 713204b.

jrbourbeau

Thanks @bryanwweber! This is in

Deprecate nprocs keyword from dask-worker

42cbfd3

Per #2471, nprocs is confusing and should be replaced by num-workers.

bryanwweber marked this pull request as draft January 6, 2022 17:21

jrbourbeau reviewed Jan 6, 2022

View reviewed changes

distributed/cli/dask_worker.py Outdated Show resolved Hide resolved

distributed/cli/tests/test_dask_worker.py Outdated Show resolved Hide resolved

distributed/cli/tests/test_dask_worker.py Outdated Show resolved Hide resolved

distributed/cli/dask_worker.py Outdated Show resolved Hide resolved

Bryan Weber added 3 commits January 6, 2022 16:57

Remove nprocs from SSHCluster

585d272

Remove nprocs from old SSHCluster

99eafc4

Update documentation to remove nprocs

0e478b4

Address review comments

013e289

bryanwweber marked this pull request as ready for review January 7, 2022 17:39

jrbourbeau assigned bryanwweber Jan 11, 2022

Test edge cases and update error messages

014975f

jacobtomlinson previously approved these changes Jan 12, 2022

View reviewed changes

bryanwweber commented Jan 12, 2022

View reviewed changes

distributed/deploy/ssh.py Outdated Show resolved Hide resolved

Fix test failures and add a few more checks

64bf8ea

bryanwweber commented Jan 12, 2022

View reviewed changes

distributed/cli/tests/test_dask_ssh.py Outdated Show resolved Hide resolved

bryanwweber commented Jan 12, 2022

View reviewed changes

distributed/deploy/tests/test_old_ssh.py Outdated Show resolved Hide resolved

Bryan Weber added 4 commits January 13, 2022 15:00

Attempt fix at SSH test failures

12cf1a1

Fix timeout for dask-ssh test

6201e15

Add more async stuff to test_ssh

6093826

Clarify test names

f4451b6

bryanwweber requested a review from jrbourbeau January 14, 2022 21:21

Use Client to test SSH CLI num_workers

ac4693e

Bryan Weber added 2 commits January 26, 2022 14:30

Debug Python 3.9

79496c1

Disable test debugging

6df4d9a

bryanwweber requested a review from jacobtomlinson January 26, 2022 21:51

Rename num-workers to nworkers

203557d

Increase Client timeout

c62ac37

The client only has 5 seconds to connect to the scheduler by default, so it seems to sometimes time out on CI.

jrbourbeau approved these changes Feb 1, 2022

View reviewed changes

Bryan Weber added 2 commits February 1, 2022 13:35

Respond to review comments

cb31967

Merge branch 'main' into deprecate-nprocs

353d5ce

jrbourbeau changed the title ~~Deprecate nprocs keyword from dask-worker~~ Deprecate --nprocs option for dask-worker CLI Feb 2, 2022

jrbourbeau approved these changes Feb 2, 2022

View reviewed changes

jrbourbeau merged commit 27e467e into dask:main Feb 2, 2022

quasiben mentioned this pull request Feb 2, 2022

nprocs deprecated in dask-worker rapidsai/dask-cuda#851

Closed

bryanwweber deleted the deprecate-nprocs branch February 2, 2022 21:43

guillaumeeb mentioned this pull request Jul 19, 2022

Replace --nprocs by --nworkers in the generated job script dask/dask-jobqueue#559

Closed

	self.n_workers = self.kwargs.pop("nprocs")
	self.n_workers = self.kwargs.pop("nprocs", 1)

Uh oh!

Conversation

bryanwweber commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GPUtester commented Jan 6, 2022

Uh oh!

jrbourbeau commented Jan 6, 2022

Uh oh!

jrbourbeau commented Jan 6, 2022

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryanwweber commented Jan 6, 2022

Uh oh!

guillaumeeb commented Jan 11, 2022

Uh oh!

jrbourbeau commented Jan 12, 2022

Uh oh!

bryanwweber commented Jan 12, 2022

Uh oh!

jacobtomlinson left a comment

Choose a reason for hiding this comment

Uh oh!

jacobtomlinson commented Jan 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryanwweber commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graingert commented Jan 19, 2022

Uh oh!

bryanwweber commented Jan 19, 2022

Uh oh!

jsignell commented Jan 19, 2022

Uh oh!

bryanwweber commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsignell commented Jan 19, 2022

Uh oh!

bryanwweber commented Jan 20, 2022

Uh oh!

jsignell commented Jan 20, 2022

Uh oh!

bryanwweber commented Jan 26, 2022

Uh oh!

bryanwweber commented Jan 26, 2022

Uh oh!

bryanwweber commented Jan 27, 2022

Uh oh!

bryanwweber commented Jan 27, 2022

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 2, 2022

bryanwweber commented Jan 6, 2022 •

edited

Loading

bryanwweber commented Jan 18, 2022 •

edited

Loading

bryanwweber commented Jan 19, 2022 •

edited

Loading