Introduce `Client.register_plugin()` #8169

hendrikmakait · 2023-09-06T15:18:14Z

Follow-up to Deprecate duck-typed plugins #8150

This PR introduces Client.register_plugin() as the new method to use for registering plugins. Following #8150, it does not allow duck-typed plugins but dispatches based on the inherited plugin class.

It also deprecates Client.register_scheduler_plugin() and Client.register_worker_plugin() in favor of Client.register_plugin(). Within Client.register_worker_plugin(), it also adjusts the dispatching logic. With this PR, typing takes precedence, which means that the plugin will get registered based on its inherited plugin class, regardless of the nanny keyword and - most notably - if a SchedulerPlugin is used, it will be registered as a scheduler plugin. This fixes the migration problem for #8142.

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

Co-authored-by: crusaderky <crusaderky@gmail.com>

github-actions · 2023-09-06T17:57:29Z

Unit Test Results

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

      21 files +      19       21 suites +19 10h 39m 18s ⏱️ + 10h 26m 24s
  3 813 tests +  1 973   3 702 ✔️ +  2 057   107 💤 -     88 4 ❌ +4
36 859 runs +34 984 35 054 ✔️ +33 409 1 800 💤 +1 570 5 ❌ +5

For more details on these failures, see this check.

Results for commit 696ab82. ± Comparison against base commit b5729b0.

This pull request removes 4 and adds 1977 tests. Note that renamed tests count towards both.

distributed.diagnostics.tests.test_nanny_plugin ‑ test_duck_typed_nanny_plugin_is_deprecated
distributed.diagnostics.tests.test_scheduler_plugin ‑ test_register_scheduler_plugin
distributed.diagnostics.tests.test_scheduler_plugin ‑ test_register_scheduler_plugin_pickle_disabled
distributed.diagnostics.tests.test_worker_plugin ‑ test_duck_typed_worker_plugin_is_deprecated

distributed.cli.tests.test_dask_scheduler ‑ test_dashboard
distributed.cli.tests.test_dask_scheduler ‑ test_dashboard_allowlist
distributed.cli.tests.test_dask_scheduler ‑ test_dashboard_non_standard_ports
distributed.cli.tests.test_dask_scheduler ‑ test_dashboard_port_zero
distributed.cli.tests.test_dask_scheduler ‑ test_defaults
distributed.cli.tests.test_dask_scheduler ‑ test_hostport
distributed.cli.tests.test_dask_scheduler ‑ test_idle_timeout
distributed.cli.tests.test_dask_scheduler ‑ test_interface
distributed.cli.tests.test_dask_scheduler ‑ test_multiple_protocols
distributed.cli.tests.test_dask_scheduler ‑ test_multiple_workers
…

♻️ This comment has been updated with latest results.

hendrikmakait · 2023-09-06T19:53:45Z

distributed/scheduler.py

@@ -7483,17 +7491,24 @@ def stop_task_metadata(self, name: str | None = None) -> dict:
        self.remove_plugin(name=plugin.name)
        return {"metadata": plugin.metadata, "state": plugin.state}

-    async def register_worker_plugin(self, comm, plugin, name=None):


Can the comm keyword be removed safely?

no, it can't #8189

hendrikmakait · 2023-09-06T19:54:41Z

distributed/client.py

-            self._register_worker_plugin, plugin=plugin, name=name, nanny=nanny
-        )
+        method: Callable
+        if isinstance(plugin, WorkerPlugin):


As opposed to #8150, this PR changes the precedence between the nanny keyword and typing.

hendrikmakait · 2023-09-06T19:55:13Z

distributed/client.py

+                    UserWarning,
+                    stacklevel=2,
+                )
+        elif isinstance(plugin, SchedulerPlugin):  # type: ignore[unreachable]


It also takes the decision to override registration in case a scheduler plugin is used.

hendrikmakait · 2023-09-07T13:35:17Z

distributed/client.py

        """
        Registers a lifecycle worker plugin for all current and future workers.

+        .. deprecated:: 2023.9.2


I'm assuming we do another release in September.

hendrikmakait · 2023-09-08T08:21:12Z

Note that this PR will likely fail if someone uses multiple inheritance, e.g. to subclass both SchedulerPlugin and WorkerPlugin. I don't think this is a pattern we'd want to encourage, but I've seen it being used at least once.

fjetter · 2023-09-08T09:52:02Z

distributed/diagnostics/tests/test_nanny_plugin.py

+            pass
+
+    n_existing_plugins = len(a.plugins)
+    assert not hasattr(a, "foo")
    with pytest.warns(DeprecationWarning, match="duck-typed.*NannyPlugin"):


for the future, there is a specialized API in pytest for deprecated stuff, https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest-deprecated-call

distributed/client.py

fjetter · 2023-09-08T09:56:59Z

distributed/client.py

                    SchedulerUploadFile(filename, load=load), name=name
                ),
-                self.register_worker_plugin(UploadFile(filename, load=load), name=name),
+                # FIXME: Make scheduler plugin responsible for (de)registering worker plugin


ticket would be nice

fjetter · 2023-09-08T10:02:16Z

changes look good. I could see use cases for multiple inheritance but what I can come up with is a "server plugin". Maybe thi is something to add later.
The deprecated paths are still available. Let's keep the deprecation warnings in for a bit to see where and if this causes issues

Co-authored-by: Florian Jetter <fjetter@users.noreply.github.com>

hendrikmakait · 2023-09-08T10:14:21Z

changes look good. I could see use cases for multiple inheritance but what I can come up with is a "server plugin". Maybe thi is something to add later.

+1 on a ServerPlugin that supports a subset of the existing APIs.

hendrikmakait and others added 15 commits September 1, 2023 12:18

Deprecate

0246655

Add tests

673aa4c

Fix tests

06cb627

Update distributed/diagnostics/tests/test_nanny_plugin.py

1239468

Co-authored-by: crusaderky <crusaderky@gmail.com>

Update distributed/diagnostics/tests/test_worker_plugin.py

2606b18

Co-authored-by: crusaderky <crusaderky@gmail.com>

typing

8d0ad56

Merge branch 'main' into client-register-plugin

7e9d178

Add idempotent KW

d366dae

Typing

fb87899

Idempotent messages

f4a2d35

Refactor

2ed284f

Refactor

ab63a2b

Overwrite

c478d8e

Replace register_scheduler_plugin

08642e4

register_plugin

bd8868a

hendrikmakait added 11 commits September 6, 2023 21:52

Fixes

ba5135c

Tests + msg

f1d1685

Tests

20a3a20

Add tests

c9bf64e

Merge branch 'main' into client-register-plugin

bd48619

Minor

0cdbb93

Docstring

718ba32

docs

ec5b700

deprecated

5c2f19e

deprecated version

3888bbe

wording

fafff80

hendrikmakait commented Sep 7, 2023

View reviewed changes

hendrikmakait added 2 commits September 7, 2023 16:00

fix

e728d56

Idempotency

382616f

test idempotency

d59f148

hendrikmakait marked this pull request as ready for review September 7, 2023 16:38

hendrikmakait requested review from jacobtomlinson and fjetter as code owners September 7, 2023 16:38

hendrikmakait added 2 commits September 8, 2023 08:29

msgs

4808ef5

tests

9a94d9c

fjetter approved these changes Sep 8, 2023

View reviewed changes

Update distributed/client.py

696ab82

Co-authored-by: Florian Jetter <fjetter@users.noreply.github.com>

hendrikmakait merged commit 4e90a0a into dask:main Sep 8, 2023
21 of 28 checks passed

hendrikmakait deleted the client-register-plugin branch September 8, 2023 11:17

hendrikmakait mentioned this pull request Sep 8, 2023

Fix PackageInstall by making it a scheduler plugin #8142

Merged

2 tasks

jrbourbeau mentioned this pull request Sep 8, 2023

CI failing due to plugin deprecations dask/dask#10502

Closed

fjetter mentioned this pull request Sep 15, 2023

Allow comm for (un)register_*_plugin #8189

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `Client.register_plugin()` #8169

Introduce `Client.register_plugin()` #8169

hendrikmakait commented Sep 6, 2023 •

edited

github-actions bot commented Sep 6, 2023 •

edited

hendrikmakait Sep 6, 2023

fjetter Sep 15, 2023

hendrikmakait Sep 6, 2023

hendrikmakait Sep 6, 2023

hendrikmakait Sep 7, 2023

hendrikmakait commented Sep 8, 2023

fjetter Sep 8, 2023

fjetter Sep 8, 2023

hendrikmakait Sep 8, 2023

fjetter commented Sep 8, 2023

hendrikmakait commented Sep 8, 2023 •

edited

Introduce Client.register_plugin() #8169

Introduce Client.register_plugin() #8169

Conversation

hendrikmakait commented Sep 6, 2023 • edited

github-actions bot commented Sep 6, 2023 • edited

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hendrikmakait commented Sep 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjetter commented Sep 8, 2023

hendrikmakait commented Sep 8, 2023 • edited

Introduce `Client.register_plugin()` #8169

Introduce `Client.register_plugin()` #8169

hendrikmakait commented Sep 6, 2023 •

edited

github-actions bot commented Sep 6, 2023 •

edited

hendrikmakait commented Sep 8, 2023 •

edited