Check plugin registration status #9118
Conversation
Signed-off-by: Jaya Venkatesh <[email protected]>
Signed-off-by: Jaya Venkatesh <[email protected]>
jayavenkatesh19
changed the title
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 27 files ± 0 27 suites ±0 9h 53m 26s ⏱️ + 16m 50s For more details on these failures, see this check. Results for commit e5cf633. ± Comparison against base commit 8e604a0. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Overall this looks good to me!
If you have a look in distributed/distributed/diagnostics/tests/ you'll find test_scheduler_plugin.py, test_worker_plugin.py and test_nanny_plugin.py. Could you go through those and add/update some tests that exercise the new code here?
There was a problem hiding this comment.
Looks good.
I wonder if it'd be more useful to have separate methods has_worker_plugin, has_scheduler_plugin, has_nanny_plugin. Are people interested whether some plugin is somewhere on the cluster? Or whether it's specifically on the scheduler or specifically on the worker?
In my experience, you typically don't have plugins of different types with the same name, so perhaps it's not worth worrying about.
| elif isinstance(plugin, (WorkerPlugin, SchedulerPlugin, NannyPlugin)): | ||
| plugin_name = getattr(plugin, "name", None) | ||
| if plugin_name is None: |
There was a problem hiding this comment.
Possible simplification: if we detect a single plugin here, we can assign it to
plugin = [plugin]and then fall through to the list case.
There was a problem hiding this comment.
Ah, I see that the return type is different in that case (bool vs. dict[str, bool]). We could add a local unbox variable to handle this, but perhaps not worth it.
distributed/client.py
Outdated
| ) | ||
| return result[plugin_name] | ||
|
|
||
| elif isinstance(plugin, list): |
There was a problem hiding this comment.
Maybe Sequence here (and in the type signature)?
distributed/client.py
Outdated
| f"Plugin {funcname(type(p))} has no 'name' attribute" | ||
| ) | ||
| names_to_check.append(plugin_name) | ||
| return self.sync(self._get_plugin_registration_status, names=names_to_check) |
There was a problem hiding this comment.
We should have an else case that raises rather than silently returning None.
Signed-off-by: Jaya Venkatesh <[email protected]>
Signed-off-by: Jaya Venkatesh <[email protected]>
There was a problem hiding this comment.
Thanks for adding the tests. There's a bit of an AI code smell in here that I want to dig a little into. Totally fine with using AI assistants, but things might need a little more cleaning up before we can merge it.
There is a lot of duplication, but there are also lots of tests that exist already that check registration, has plugin and unregistration. The check for "has plugin" in the existing tests involves poking the objects directly, it feels like a great opportunity to exercise this new method instead. It would also mean we could remove a bunch of these new tests in favour of small tweaks to existing ones.
| class DuckPlugin(NannyPlugin): | ||
| name = "duck-plugin" | ||
|
|
||
| def setup(self, nanny): | ||
| nanny.foo = 123 | ||
|
|
||
| def teardown(self, nanny): | ||
| pass |
There was a problem hiding this comment.
This gets redefined quite a few times in here, can we move it outside the test and define it once to DRY this out a little?
| async def test_has_nanny_plugin_list_check(c, s, a): | ||
| """Test checking multiple nanny plugins at once""" | ||
|
|
||
| class IdempotentPlugin(NannyPlugin): |
There was a problem hiding this comment.
I'm curious about the naming here. Why Idempotent and NonIdempotent?
| self.expected_notifications = expected_notifications | ||
|
|
||
| # Check non-existent plugin | ||
| assert not await c.has_plugin("MyPlugin") # ← await |
There was a problem hiding this comment.
What's with these comments?
| assert not await c.has_plugin("MyPlugin") # ← await | |
| assert not await c.has_plugin("MyPlugin") |
3 participants
Closes #9106
pre-commit run --all-filesAdds a client side function to check if a plugin is registered on either the scheduler, worker or nanny registries. The plugin names seem to be unique enough to ignore the edge case where two different plugin types such as a
WorkerPluginand aSchedulerPluginhave the same name. Theschedulerhas separate functions added for this use case but will need to add extraclientfunctions to separate thehas_pluginfunction intohas_worker_pluginetc.Apart from checking if a plugin is registered by passing its name, also added the functionality of passing the plugin object itself with name extraction handled by the function. This is especially useful while using built-in plugins for users who do not know about the
plugin.nameattribute. Also added an error check for plugins whose name is not set.CC: @jacobtomlinson