-
-
Notifications
You must be signed in to change notification settings - Fork 727
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
Changes from all commits
3ffe816
48a7854
44d07fb
61b15e2
0f47ac3
1b20f8d
34b2010
0e06669
a54be03
be89019
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
from distributed.utils_test import gen_cluster | ||
from distributed import Worker | ||
|
||
|
||
class MyPlugin: | ||
name = "MyPlugin" | ||
|
||
def __init__(self, data): | ||
self.data = data | ||
|
||
def setup(self, worker): | ||
assert isinstance(worker, Worker) | ||
self.worker = worker | ||
self.worker._my_plugin_status = "setup" | ||
self.worker._my_plugin_data = self.data | ||
|
||
def teardown(self, worker): | ||
assert isinstance(worker, Worker) | ||
self.worker._my_plugin_status = "teardown" | ||
|
||
|
||
@gen_cluster(client=True, ncores=[]) | ||
def test_create_with_client(c, s): | ||
yield c.register_worker_plugin(MyPlugin(123)) | ||
|
||
worker = Worker(s.address, loop=s.loop) | ||
yield worker._start() | ||
assert worker._my_plugin_status == "setup" | ||
assert worker._my_plugin_data == 123 | ||
|
||
yield worker._close() | ||
assert worker._my_plugin_status == "teardown" | ||
|
||
|
||
@gen_cluster(client=True, worker_kwargs={"plugins": [MyPlugin(5)]}) | ||
def test_create_on_construction(c, s, a, b): | ||
assert len(a.plugins) == len(b.plugins) == 1 | ||
assert a._my_plugin_status == "setup" | ||
assert a._my_plugin_data == 5 | ||
|
||
|
||
@gen_cluster(client=True, worker_kwargs={"plugins": [MyPlugin(5)]}) | ||
def test_idempotence_with_name(c, s, a, b): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that we're good here. The In the test below, we change the name intentionally. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, understood. |
||
a._my_plugin_data = 100 | ||
|
||
yield c.register_worker_plugin(MyPlugin(5)) | ||
|
||
assert a._my_plugin_data == 100 # call above has no effect | ||
|
||
|
||
@gen_cluster(client=True, worker_kwargs={"plugins": [MyPlugin(5)]}) | ||
def test_duplicate_with_no_name(c, s, a, b): | ||
assert len(a.plugins) == len(b.plugins) == 1 | ||
|
||
plugin = MyPlugin(10) | ||
plugin.name = "other-name" | ||
|
||
yield c.register_worker_plugin(plugin) | ||
|
||
assert len(a.plugins) == len(b.plugins) == 2 | ||
|
||
assert a._my_plugin_data == 10 | ||
|
||
yield c.register_worker_plugin(plugin) | ||
assert len(a.plugins) == len(b.plugins) == 2 | ||
|
||
yield c.register_worker_plugin(plugin, name="foo") | ||
assert len(a.plugins) == len(b.plugins) == 3 |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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