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

Replace register_worker_callbacks with worker plugins #2453

Merged
merged 10 commits into from May 22, 2019

Conversation

@mrocklin
Copy link
Member

@mrocklin mrocklin commented Jan 8, 2019

These provide space for a bit more complexity, including teardown and optional idempotence. This is still a work in progress. Some things I'd like to do:

  • deprecate register_worker_callbacks
  • support simple setup functions in register_worker_plugins
  • improve error handling in broadcast

cc @rainwoodman and @guillaumeeb . Possible replacement for #2391

distributed/client.py Outdated Show resolved Hide resolved
Loading
distributed/worker.py Show resolved Hide resolved
Loading
@rainwoodman
Copy link
Contributor

@rainwoodman rainwoodman commented Jan 8, 2019

I like this better than #2391.

Loading

Copy link
Member

@guillaumeeb guillaumeeb left a comment

I agree with @rainwoodman, this design looks better!

Loading

distributed/client.py Outdated Show resolved Hide resolved
Loading
distributed/client.py Outdated Show resolved Hide resolved
Loading
distributed/client.py Outdated Show resolved Hide resolved
Loading
distributed/client.py Outdated Show resolved Hide resolved
Loading
distributed/worker.py Show resolved Hide resolved
Loading
@mrocklin
Copy link
Member Author

@mrocklin mrocklin commented May 21, 2019

@jcrist should you find yourself with a moment this could also use your review

Loading

@mrocklin
Copy link
Member Author

@mrocklin mrocklin commented May 22, 2019

Docs added in dask/dask#4833

Loading

@mrocklin
Copy link
Member Author

@mrocklin mrocklin commented May 22, 2019

I plan to merge this this afternoon if there are no further comments

Loading

def register_worker_plugin(self, plugin=None, name=None):
"""
Registers a lifecycle worker plugin for all current and future workers.
Copy link
Member

@TomAugspurger TomAugspurger May 22, 2019

Choose a reason for hiding this comment

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

General question: do you have thoughts on using ..versionadded:: <version> directives? Don't need to do it here necessarily, but I ask since this is being referenced from the dask documentation, which may be on a different release cycle.

Loading

Copy link
Member Author

@mrocklin mrocklin May 22, 2019

Choose a reason for hiding this comment

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

No strong thoughts. I've never used them much, either as an author or reader.

My plan was just to merge the doc PR after this gets released.

Happy with whatever though.

Loading

Copy link
Member Author

@mrocklin mrocklin May 22, 2019

Choose a reason for hiding this comment

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

Ah, I see that you've since merged the doc PR :) Shouldn't be a big deal either way

Loading

Copy link
Member

@TomAugspurger TomAugspurger left a comment

Small question on a test name. Since Github / travis have been having issues with web hooks today, you may just want to merge.

Loading



@gen_cluster(client=True, worker_kwargs={"plugins": [MyPlugin(5)]})
def test_idempotence_with_name(c, s, a, b):
Copy link
Member

@TomAugspurger TomAugspurger May 22, 2019

Choose a reason for hiding this comment

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

Are the names of these two tests flipped (this one doesn't use plugin names, the other does?)

Loading

Copy link
Member Author

@mrocklin mrocklin May 22, 2019

Choose a reason for hiding this comment

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

I think that we're good here. The MyPlugin class has a name attribute, which is what causes the idempotence here.

In the test below, we change the name intentionally.

Loading

Copy link
Member

@TomAugspurger TomAugspurger May 22, 2019

Choose a reason for hiding this comment

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

Ah, understood.

Loading

@TomAugspurger TomAugspurger merged commit 6134c75 into dask:master May 22, 2019
1 of 2 checks passed
Loading
@mrocklin mrocklin deleted the worker-plugin branch May 22, 2019
@mrocklin
Copy link
Member Author

@mrocklin mrocklin commented May 22, 2019

Woot. Thanks for merging @TomAugspurger ! I think that some version of this PR has existed for years now :)

Loading

calebho added a commit to calebho/distributed that referenced this issue May 29, 2019
* Add worker plugins

* add docstring

* Replace legacy worker_callbacks with worker_plugins

* add and test name keyword

* fix missing import

* black

* respond to feedback

* Handle errors again

* Expand docstring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants