Remove the requirements to add comm to every handler #5820

fjetter · 2022-02-16T14:00:40Z

Most of our handlers are specifying comm as its first argument because the handle_comm interface currently requires so. IMO this does not only create very poor function signatures but also is potentially confusing to new developers. Dropping this requirement is pretty easy by simply checking for the argument names.

Of course, this check for the name of the argument so there is no implicit comm passing possible anymore but I'd be fine with that.

I simplified a few signatures already but would leave the bulk work to follow up PRs

distributed/core.py

distributed/node.py

distributed/scheduler.py

crusaderky · 2022-02-16T18:08:54Z

distributed/scheduler.py

        task_stream = self.get_task_stream(start=start)
        total_tasks = len(task_stream)
-        timespent = defaultdict(int)
+        timespent: dict[int, float] = defaultdict(int)


Suggested change

timespent: dict[int, float] = defaultdict(int)

timespent: "defaultdict[str, float]" = defaultdict(int)

looks like we're abusing defaultdict while we should really use a Counter?

sorry, I think my annotation is just wrong and it shouldn't be initialized to int but rather float, if we're 100% accurate

timespent: defaultdict[str, float] = defaultdict(float)

Does mypy recognize defaultdict?

mypy recognizes everything in collections and collections.abc.
Note that you must have either from __future__ import annotations in your module or wrap it in quotes. In scheduler.py you don't have the former option due to Cython.

still, I think you should use a Counter?

still, I think you should use a Counter?

Am I missing something obvious? How is this a counter? start, stop are floats / time.time(), we're not counting
timespent[x["action"]] += x["stop"] - x["start"]

Counter and defaultdict(int) are pretty much the same thing, they only differ in how you initialise them. Nevermind, it's really a philosophical difference.

distributed/tests/test_core.py

github-actions · 2022-02-17T04:06:15Z

Unit Test Results

      12 files ±0       12 suites ±0 7h 13m 19s ⏱️ + 1m 48s
  2 608 tests +1   2 528 ✔️ +  2     79 💤 ±  0 1 ❌ ±0
15 572 runs +6 14 503 ✔️ +38 1 068 💤 - 30 1 ❌ - 1

For more details on these failures, see this check.

Results for commit e3b1f0c. ± Comparison against base commit 43dfb61.

♻️ This comment has been updated with latest results.

crusaderky · 2022-02-19T12:14:35Z

distributed/core.py


+def _expects_comm(func):
+    sig = inspect.signature(func)
+    return "comm" in sig.parameters or "stream" in sig.parameters


Where is stream used? Is it for potential third party scheduler extensions? If so, I think there should be a deprecation cycle.

I could not find any internal use of stream, besides this one which you forgot to remove:
https://github.com/fjetter/distributed/blob/a16c7e713a787b52573c31664db98d2fb275dab7/distributed/publish.py#L50-L52

How about we raise a deprecation warning here if we hit stream? I noticed there is another place and didn't clean it up since I wasn't sure about the actual usage.

Note: The change I am proposing here is technically already a breaking change to user plugins. If they are not using any of the two names, their plugin might break.

I'm wondering if this warrants a deprecation cycle as well.

Thoughts?

I think the deprecation system you put in place is already plenty

distributed/scheduler.py

distributed/tests/test_core.py

crusaderky · 2022-02-21T10:38:20Z

distributed/core.py

 LOG_PDB = dask.config.get("distributed.admin.pdb-on-err")


+def _expects_comm(func):


Suggested change

def _expects_comm(func):

def _expects_comm(func: Callable) -> bool:

crusaderky · 2022-02-21T10:42:10Z

distributed/core.py

+    sig = inspect.signature(func)
+    if "comm" in sig.parameters or "stream" in sig.parameters:
+        params = list(sig.parameters)
+        if len(params) > 1 and params[1] in ["comm", "stream"]:
+            if "stream" in sig.parameters:
+                try:
+                    fname = func.__name__
+                except AttributeError:
+                    # e.g. partials don't have __name__ We don't want to crash
+                    # just because we're trying to raise a warning
+                    fname = "unknown"
+                warnings.warn(
+                    f"Calling the first arugment of a RPC handler `stream` is deprecated. Defining this argument is optional. Either remove the arugment or rename it to `comm`. Instead got {fname}{sig}",
+                    FutureWarning,
+                )
+            return True


Suggested change

sig = inspect.signature(func)

if "comm" in sig.parameters or "stream" in sig.parameters:

params = list(sig.parameters)

if len(params) > 1 and params[1] in ["comm", "stream"]:

if "stream" in sig.parameters:

try:

fname = func.__name__

except AttributeError:

# e.g. partials don't have __name__ We don't want to crash

# just because we're trying to raise a warning

fname = "unknown"

warnings.warn(

f"Calling the first arugment of a RPC handler `stream` is deprecated. Defining this argument is optional. Either remove the arugment or rename it to `comm`. Instead got {fname}{sig}",

FutureWarning,

)

return True

params = list(inspect.signature(func).parameters)

if params[:1] == ["comm"]:

return True

if params[:1] == ["stream"]:

warnings.warn(

f"Calling the first arugment of a RPC handler `stream` is deprecated. Defining this argument is optional. Either remove the argument or rename it to `comm` in {func}",

FutureWarning,

)

return True

crusaderky · 2022-02-21T10:42:58Z

distributed/tests/test_core.py

+        def comm_kwarg_only(self, *, comm, other):
+            ...
+
+        def comm_not_leading_position(self, *, other, comm):
+            ...
+
+        def stream_not_leading_position(self, *, other, stream):
+            ...
+
+        stream_not_leading_partialmethod = partialmethod(
+            stream_not_leading_position, other="foo"
+        )


Suggested change

def comm_kwarg_only(self, *, comm, other):

...

def comm_not_leading_position(self, *, other, comm):

...

def stream_not_leading_position(self, *, other, stream):

...

stream_not_leading_partialmethod = partialmethod(

stream_not_leading_position, other="foo"

)

I actually think the "not leading position" thing is important since I don't want to block any names from signatures. If a user wants to define a stream kwarg or a comm kwarg without using our comm mechanism that's fine imo.

I generally dislike this API quite a lot. I don't think we should silently expect the first argument to be positional-only comm.

distributed/tests/test_core.py

fjetter · 2022-02-21T12:03:33Z

I'm quite open to moving on to a more stable interface overall. I figured this may be a nice transition to something that is cleaner.
For instance, we might want to choose to pass comm as a keyword argument instead of a positional. I believe this would already provide a clearer API. I'm sure there are even better options.

crusaderky · 2022-02-21T12:05:53Z

I'm happy to support comm as a keyword argument; handle_comm should be changed accordingly (with added unit tests)

fjetter · 2022-02-22T12:32:38Z

I'm happy to support comm as a keyword argument; handle_comm should be changed accordingly (with added unit tests)

We'd still need to go through a deprecation cycle for the stream argument. I think we should do this in another release

fjetter requested a review from crusaderky February 16, 2022 14:00

crusaderky requested changes Feb 16, 2022

View reviewed changes

crusaderky mentioned this pull request Feb 17, 2022

Improve type annotations in worker.py #5814

Merged

crusaderky assigned fjetter Feb 17, 2022

Remove the requirements to add comm to every handler

0416d33

fjetter force-pushed the remove_comm_requ_from_handlers branch from f3d555b to 69cb2e1 Compare February 18, 2022 15:01

fjetter requested a review from crusaderky February 18, 2022 15:02

review comments

44077d3

fjetter force-pushed the remove_comm_requ_from_handlers branch from 69cb2e1 to 44077d3 Compare February 18, 2022 15:02

fjetter mentioned this pull request Feb 18, 2022

Decrease timeout-minutes for GHA jobs #5837

Merged

Remove remove_worker type annotations due to cythonization error

a16c7e7

crusaderky requested changes Feb 19, 2022

View reviewed changes

fjetter added 3 commits February 21, 2022 10:12

Include deprecation cycle and allow usage beyond first position

fc864d8

add partialmethod

5030cd7

use getitem instead of get

4205e15

crusaderky reviewed Feb 21, 2022

View reviewed changes

distributed/tests/test_core.py Outdated Show resolved Hide resolved

crusaderky reviewed Feb 21, 2022

View reviewed changes

distributed/tests/test_core.py Show resolved Hide resolved

crusaderky reviewed Feb 21, 2022

View reviewed changes

distributed/tests/test_core.py Outdated Show resolved Hide resolved

crusaderky reviewed Feb 21, 2022

View reviewed changes

distributed/tests/test_core.py Outdated Show resolved Hide resolved

fjetter force-pushed the remove_comm_requ_from_handlers branch 2 times, most recently from e49775e to 537750c Compare February 21, 2022 11:58

simplify

e3b1f0c

fjetter force-pushed the remove_comm_requ_from_handlers branch from 537750c to e3b1f0c Compare February 21, 2022 11:58

crusaderky approved these changes Feb 22, 2022

View reviewed changes

crusaderky merged commit 9a266a0 into dask:main Feb 22, 2022

	timespent: dict[int, float] = defaultdict(int)
	timespent: "defaultdict[str, float]" = defaultdict(int)

		LOG_PDB = dask.config.get("distributed.admin.pdb-on-err")


		def _expects_comm(func):

	def _expects_comm(func):
	def _expects_comm(func: Callable) -> bool:

Uh oh!

Remove the requirements to add comm to every handler #5820

Remove the requirements to add comm to every handler #5820

Uh oh!

Conversation

fjetter commented Feb 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

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!

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

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!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Unit Test Results

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Uh oh!

Uh oh!

fjetter commented Feb 21, 2022

Uh oh!

crusaderky commented Feb 21, 2022

Uh oh!

fjetter commented Feb 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

crusaderky Feb 16, 2022 •

edited

Loading

crusaderky Feb 17, 2022 •

edited

Loading

github-actions bot commented Feb 17, 2022 •

edited

Loading

crusaderky Feb 21, 2022 •

edited

Loading