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

Raise helpful error when using the wrong plugin base classes #8893

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

jacobtomlinson
Copy link
Member

@jacobtomlinson jacobtomlinson commented Oct 15, 2024

The Client.register_plugin() method uses @functools.singledispatchmethod to dispatch to the correct plugin registration method. However singledispatchmethod doesn't like the dask.distributed import shim and can't determine that dask.distributed.diagnostics.plugins.WorkerPlugin is the same as distributed.diagnostics.plugins.WorkerPlugin.

This means that plugins based on classes like dask.distributed.diagnostics.plugins.WorkerPlugin raise the TypeError("Registering duck-typed plugins is not allowed. Please inherit from NannyPlugin, WorkerPlugin, or SchedulerPlugin to create a plugin.") exception.

I explored importing the base classes from the dask.distributed path and registering those for dispatch too, but it's not possible to import them from dask.distributed.diagnostics.plugins while distributed is being imported. And it's not possible to register them at runtime, only at import time.

Therefore the best option here appears to be to improve the error message. So this PR checks for plugins based on classes from dask.distributed.diagnostics.plugins and makes the error message more helpful.

In [1]: from dask.distributed.diagnostics.plugin import WorkerPlugin
   ...: import dask.distributed as dd
   ...: 
   ...: class PolitePlugin(WorkerPlugin):
   ...:     def setup(self, worker):
   ...:         print("hi", worker)
   ...:         self.worker = worker
   ...: 
   ...:     def teardown(self, worker):
   ...:         print("bye", worker)
   ...: 
   ...: cluster = dd.LocalCluster()
   ...: client = dd.Client(cluster)
   ...: client.register_plugin(PolitePlugin())
---------------------------------------------------------------------------
TypeError: Importing plugin base classes from `dask.distributed.diagnostics.plugin` is not supported. Please import directly from `distributed.diagnostics.plugin` instead.

Closes #8778

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

Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

Thanks, @jacobtomlinson! It's a pity the shim isn't handled properly.

Copy link
Contributor

Unit Test Results

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

    25 files  ±0      25 suites  ±0   10h 18m 23s ⏱️ - 5m 4s
 4 130 tests ±0   4 016 ✅ +1    110 💤 ±0  4 ❌  - 1 
47 708 runs  ±0  45 607 ✅  - 1  2 096 💤 +1  5 ❌ ±0 

For more details on these failures, see this check.

Results for commit 9be2d8d. ± Comparison against base commit 9bec841.

@hendrikmakait hendrikmakait merged commit c3482ee into dask:main Oct 15, 2024
24 of 31 checks passed
@jacobtomlinson jacobtomlinson deleted the plugin-base-shims branch October 16, 2024 08:20
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.

Can't register WorkerPlugin subclass with Client.register_plugin
2 participants