Skip to content

Conversation

@douglasdavis
Copy link
Member

@douglasdavis douglasdavis commented Jul 26, 2021

  • Closes #xxxx
  • Tests added / passed
  • Passes black distributed / flake8 distributed / isort distributed

Following up on some discussion in #4808, this converts scheduler plugin storage from a list to a dictionary (key: name, value: plugin instance)

@douglasdavis
Copy link
Member Author

douglasdavis commented Jul 27, 2021

OK this is ready for a first review-- the main thing that I'm not sure about is the introduction of names for the builtin plugins. I've implemented name discovery similar to worker plugins: that is, if a name attribute is present on the plugin instance, it is taken as the name to be used as the key in the plugin dictionary. Are there any scheduler plugin classes that are designed/capable of having multiple instances used?

An alternative would be to use the name= argument in the Scheduler.add_plugin method, or add name= to SchedulerPlugin.start(scheduler=..., name=...)

"""Maintain a copy of worker events"""

def __init__(self, scheduler=None):
self.name = "EventStream"
Copy link
Member Author

Choose a reason for hiding this comment

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

w.r.t. #5120 (comment): Here's an example of giving a name to a builtin plugin

@douglasdavis
Copy link
Member Author

douglasdavis commented Jul 27, 2021

After seeing #5127, perhaps something similar should be done for SchedulerPlugins (a name property which uses tokenize?)

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @douglasdavis!

@douglasdavis
Copy link
Member Author

Thanks for the feedback @jrbourbeau! I think I covered everything so far, happy to make further adjustments

@jrbourbeau
Copy link
Member

Thanks for the updates @douglasdavis! FWIW there's a discussion over in #5127 around how worker and nanny plugins treat repeats different which you may find interesting

@mrocklin
Copy link
Member

I need something similar to this for some ongoing work. My apologies for taking over. I hope that you don't mind.

@mrocklin mrocklin merged commit 9c92a61 into dask:main Aug 22, 2021
@mrocklin
Copy link
Member

This is in. Thank you @douglasdavis

@douglasdavis
Copy link
Member Author

Don't mind at all, thanks for getting it over the finish line

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.

3 participants