Polish parsing of worker-saturation from config by crusaderky · Pull Request #7255 · dask/distributed

crusaderky · 2022-11-03T17:52:13Z

Explain and polish support for string inf.
Add check vs. zero or negative values and increase coverage %.

[ORIGINAL COMMENT]
Remove support for string worker-saturation that can be cast to a float.
It's a pattern that is nowhere else to be seen in the package, is unnecessary (not even when reading from env variables), and violates the json schema.

github-actions · 2022-11-03T19:02:36Z

Unit Test Results

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

      15 files       15 suites 6h 28m 37s ⏱️
  3 169 tests   3 082 ✔️   84 💤 3 ❌
23 448 runs 22 542 ✔️ 901 💤 5 ❌

For more details on these failures, see this check.

Results for commit 4248873.

♻️ This comment has been updated with latest results.

gjoseph92 · 2022-11-03T19:18:34Z

is unnecessary (not even when reading from env variables)

Prior to #7064, a user explicitly reported it not working with environment variables:

FWIW I set the env var DASK_DISTRIBUTED__SCHEDULER__WORKER_SATURATION in the docker image that the client/scheduler/workers use, but when I ran, the workers died before doing any computation

confirmed it works when not setting environment variable

| Traceback (most recent call last):                                                                                                                                                   |
|   File "/opt/conda/lib/python3.9/site-packages/distributed/utils.py", line 742, in wrapper                                                                                           |
|     return await func(*args, **kwargs)                                                                                                                                               |
|   File "/opt/conda/lib/python3.9/site-packages/distributed/scheduler.py", line 4051, in add_worker                                                                                   |
|     self.check_idle_saturated(ws)                                                                                                                                                    |
|   File "/opt/conda/lib/python3.9/site-packages/distributed/scheduler.py", line 2896, in check_idle_saturated                                                                         |
|     if math.isinf(self.WORKER_SATURATION)                                                                                                                                            |
| TypeError: must be real number, not str                                                                                                                                              |

gjoseph92 · 2022-11-03T19:23:33Z

I can't reproduce that problem myself, but on this branch you can't set worker-saturation to inf with an env var:

(dask-distributed) gabe dev/distributed ‹worker-saturation-conf› » DASK_DISTRIBUTED__SCHEDULER__WORKER_SATURATION=inf dask-scheduler                                                                                                                                                                                                                                                                                           1 ↵
/Users/gabe/dev/distributed/distributed/cli/dask_scheduler.py:140: FutureWarning: dask-scheduler is deprecated and will be removed in a future release; use `dask scheduler` instead
  warnings.warn(
2022-11-03 13:22:11,047 - distributed.scheduler - INFO - -----------------------------------------------
2022-11-03 13:22:11,492 - distributed.http.proxy - INFO - To route to workers diagnostics web server please install jupyter-server-proxy: python -m pip install jupyter-server-proxy
2022-11-03 13:22:11,527 - distributed.scheduler - INFO - State start
2022-11-03 13:22:11,528 - distributed.scheduler - INFO - End scheduler
Traceback (most recent call last):
  File "/Users/gabe/miniconda3/envs/dask-distributed/bin/dask-scheduler", line 33, in <module>
    sys.exit(load_entry_point('distributed', 'console_scripts', 'dask-scheduler')())
  File "/Users/gabe/miniconda3/envs/dask-distributed/lib/python3.9/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/Users/gabe/miniconda3/envs/dask-distributed/lib/python3.9/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/Users/gabe/miniconda3/envs/dask-distributed/lib/python3.9/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/gabe/miniconda3/envs/dask-distributed/lib/python3.9/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/Users/gabe/dev/distributed/distributed/cli/dask_scheduler.py", line 248, in main
    asyncio.run(run())
  File "/Users/gabe/miniconda3/envs/dask-distributed/lib/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/Users/gabe/miniconda3/envs/dask-distributed/lib/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/Users/gabe/dev/distributed/distributed/cli/dask_scheduler.py", line 208, in run
    scheduler = Scheduler(
  File "/Users/gabe/dev/distributed/distributed/scheduler.py", line 3615, in __init__
    SchedulerState.__init__(
  File "/Users/gabe/dev/distributed/distributed/scheduler.py", line 1656, in __init__
    raise ValueError(  # pragma: nocover
ValueError: `distributed.scheduler.worker-saturation` must be a float > 0; got 'inf'

crusaderky · 2022-11-03T20:43:16Z

This is frustrating:

$ DASK_TEST1=inf python -c 'import dask; x = dask.config.get("test1"); print(x, type(x))'
inf <class 'str'>
$ DASK_TEST1=.inf python -c 'import dask; x = dask.config.get("test1"); print(x, type(x))'
.inf <class 'str'>
$ DASK_TEST1=math.inf python -c 'import dask; x = dask.config.get("test1"); print(x, type(x))'
math.inf <class 'str'>
$ DASK_TEST1=1.2 python -c 'import dask; x = dask.config.get("test1"); print(x, type(x))'
1.2 <class 'float'>

Relevant code:
https://github.com/dask/dask/blob/c013287bbb708ac473f0c7fc00506306bcb8d873/dask/config.py#L229-L235

crusaderky · 2022-11-03T21:00:01Z

@gjoseph92 please review now

gjoseph92 · 2022-11-03T22:44:33Z

LGTM. I'm still slightly concerned that a user reported that the environment variable wasn't parsed correctly before #7064, and then it worked after that was released.

@jgdwyer if it's straightforward for you to install git+https://github.com/crusaderky/distributed.git@worker-saturation-conf and see if setting DASK_DISTRIBUTED__SCHEDULER__WORKER_SATURATION=1.0 still works for you with this branch in your environment, that would be very helpful. If it would take you more than 10min, then don't bother. I have a feeling that maybe when you originally set it, some extra whitespace could have snuck in there?

jgdwyer · 2022-11-04T15:45:16Z

LGTM. I'm still slightly concerned that a user reported that the environment variable wasn't parsed correctly before #7064, and then it worked after that was released.

@jgdwyer if it's straightforward for you to install git+https://github.com/crusaderky/distributed.git@worker-saturation-conf and see if setting DASK_DISTRIBUTED__SCHEDULER__WORKER_SATURATION=1.0 still works for you with this branch in your environment, that would be very helpful. If it would take you more than 10min, then don't bother. I have a feeling that maybe when you originally set it, some extra whitespace could have snuck in there?

@gjoseph92 I just gave this a try and I think it worked okay. I forgot to update the client image to use this version, but I think that is probably okay as the env var only applies to scheduler/workers if I understand correctly. It's certainly possible that I incorporated extra whitespace when I had tried it earlier. (I've been setting this env var in Docker using ENV DASK_DISTRIBUTED__SCHEDULER__WORKER_SATURATION=1.0).

gjoseph92

Thanks for checking @jgdwyer, I'm happy to have this cleaned up then.

crusaderky requested a review from gjoseph92 November 3, 2022 17:52

crusaderky self-assigned this Nov 3, 2022

crusaderky added 2 commits November 3, 2022 20:44

Polish parsing of worker-saturation from config

596ce92

support inf string

4248873

crusaderky force-pushed the worker-saturation-conf branch from 3fec109 to 4248873 Compare November 3, 2022 20:58

gjoseph92 mentioned this pull request Nov 3, 2022

Turn on queuing by default #7213

Closed

11 tasks

gjoseph92 approved these changes Nov 4, 2022

View reviewed changes

gjoseph92 merged commit 4b00be1 into dask:main Nov 4, 2022

crusaderky deleted the worker-saturation-conf branch November 7, 2022 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Polish parsing of worker-saturation from config#7255

Polish parsing of worker-saturation from config#7255
gjoseph92 merged 2 commits intodask:mainfrom
crusaderky:worker-saturation-conf

crusaderky commented Nov 3, 2022 •

edited

Loading

Uh oh!

github-actions bot commented Nov 3, 2022 •

edited

Loading

Uh oh!

gjoseph92 commented Nov 3, 2022

Uh oh!

gjoseph92 commented Nov 3, 2022

Uh oh!

crusaderky commented Nov 3, 2022

Uh oh!

crusaderky commented Nov 3, 2022

Uh oh!

gjoseph92 commented Nov 3, 2022

Uh oh!

jgdwyer commented Nov 4, 2022

Uh oh!

gjoseph92 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

crusaderky commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

gjoseph92 commented Nov 3, 2022

Uh oh!

gjoseph92 commented Nov 3, 2022

Uh oh!

crusaderky commented Nov 3, 2022

Uh oh!

crusaderky commented Nov 3, 2022

Uh oh!

gjoseph92 commented Nov 3, 2022

Uh oh!

jgdwyer commented Nov 4, 2022

Uh oh!

gjoseph92 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

crusaderky commented Nov 3, 2022 •

edited

Loading

github-actions bot commented Nov 3, 2022 •

edited

Loading