Revisit Scheduler.add_plugin / Scheduler.remove_plugin
#5394
Conversation
|
XREF #5120 |
douglasdavis
left a comment
douglasdavis
left a comment
There was a problem hiding this comment.
Everything makes sense to me and I think it looks good; if @jrbourbeau gets a chance to take a look I think that would be a good idea since he recently reviewed the plugin PR I worked on
jrbourbeau
left a comment
jrbourbeau
left a comment
There was a problem hiding this comment.
Thanks @crusaderky!
This seems reasonable to me. Out of curiosity, what motivated this change? Was there something in particular you ran across?
| if idempotent: | ||
| return |
There was a problem hiding this comment.
Thanks for moving this here. It looks like we were emitting a warning when we shouldn't have been before.
distributed/scheduler.py
Outdated
| if not names: | ||
| return |
There was a problem hiding this comment.
I'd prefer to error here instead of silently returning when a user asks to remove a non-registered plugin
There was a problem hiding this comment.
Now both deprecated and new code raise ValueError.
Passing a plugin class to add_plugin did not make sense, as it was creating the instance with the scheduler as the first positional parameter which most plugin classes won't accept. |
|
All review comments have been incorporated |
jrbourbeau
changed the title
Scheduler.add_plugin / Scheduler.remove_plugin
3 participants
Deprecate adding plugins by type: the scheduler positional argument is only accepted by some plugins and it's undocumented.
Harden the logic of add_plugin and remove_plugin.