Fix add_plugin warnings #5267
Fix add_plugin warnings #5267
Conversation
douglasdavis
force-pushed
the
add-plugin-warnings
branch
from
abfd4dc to
2afcfa5
Compare
jrbourbeau
left a comment
jrbourbeau
left a comment
There was a problem hiding this comment.
Thanks @douglasdavis! It looks like we're still getting
distributed/dashboard/tests/test_scheduler_bokeh.py::test_simple
distributed/dashboard/tests/test_scheduler_bokeh.py::test_https_support
/home/runner/work/distributed/distributed/distributed/scheduler.py:5483: UserWarning: Scheduler already contains a plugin with name graph-layout; overwriting.
| if GraphLayout.name in scheduler.plugins: | ||
| self.layout = scheduler.plugins[GraphLayout.name] | ||
| else: | ||
| self.layout = GraphLayout(scheduler) | ||
| scheduler.add_plugin(self.layout) |
There was a problem hiding this comment.
It turns out that using a unique name here breaks the task graph dashboard plot even though tests pass. Instead of seeing the rendered task graph, the plot is blank.
Locally I tested out assigning a unique name and this seems to fixes things (both tests pass and when I manually look at the task graph plot, everything is as expected). I suspect this is an unexpected side-effect which has always been here, but previously went unnoticed, and the recent switch around how we store scheduler plugins made this more obvious.
Ideally we'd only have a single GraphLayout plugin running on the scheduler and multiple dashboard plots can then access the state stored on that plugin. I'll spend a few minutes seeing if I can get things to work in this manner. If I'm not successful after a bit, let's just go with generating a unique name for each GraphLayout plugin and open an issue to track follow-up work.
There was a problem hiding this comment.
Going with using a unique name for each GraphLayout plugin instance. This is inefficient, but consistent what our previous behavior was. I'll open an issue to track future work
There was a problem hiding this comment.
Hey @jrbourbeau thanks for picking this up! I was wrestling this earlier in the week and and also went the uuid route (which I also thought was a bit of a band-aid, so I wanted to come back and try to think of something else). I also agree with your take on the unnoticed previous behavior. I'll be happy to follow up on a new issue after getting this taken care of for the release.
There was a problem hiding this comment.
That sounds great. It sounds like you did some of the same debugging I did and have a good understanding of the issue we want to address here. Could I ask you to open up an issue which documents this? No worries if you're busy with other things
2 participants
black distributed/flake8 distributed/isort distributedthe
Scheduler.get_task_streammethod creates aTaskStreamPluginand adds it to the scheduler plugins viaScheduler.add_plugin; butTaskStreamPlugin.__init__was also callingScheduler.add_plugin, hence the warning. I removed theScheduler.add_plugincall fromTaskStreamPlugin.__init__, so I updated otherTaskStreamPlugintests to explicitly useScheduler.add_plugininstead of relying on initialization to add the plugin.