Pass `stimulus_id` to `SchedulerPlugin.remove_worker` and `SchedulerPlugin.transition` by hendrikmakait · Pull Request #7974 · dask/distributed

hendrikmakait · 2023-07-06T15:33:31Z

Addresses #7973 by implementing the bare minimum to support #7970

Tests added / passed
Passes pre-commit run --all-files

jrbourbeau

Thanks @hendrikmakait. This will break existing plugins that have a transition and/or remove_worker method. Can we make stimulus_id optional such that existing plugins continue to work?

hendrikmakait · 2023-07-06T15:53:18Z

Thanks @hendrikmakait. This will break existing plugins that have a transition and/or remove_worker method. Can we make stimulus_id optional such that existing plugins continue to work?

I'm not sure if we can do this nicely. The problem is that some plugins might accept it as a keyword, others might not, so (I think) we'd have to resort to introspection, which is rather cumbersome.

distributed/distributed/scheduler.py

Lines 1981 to 1993 in 8e92b85

    
           if ( 
        
               "stimulus_id" 
        
               in inspect.signature(plugin.transition).parameters 
        
           ): 
        
               plugin.transition( 
        
                   key, 
        
                   start, 
        
                   actual_finish, 
        
                   stimulus_id=stimulus_id, 
        
                   **kwargs, 
        
               ) 
        
           else: 
        
               plugin.transition(key, start, actual_finish, **kwargs)

implements this. I think I'd be fine with adding such a code for a deprecation cycle and issue a DeprecationWarning if stimulus_id is not supported, but I'd rather want to avoid this long-term.

github-actions · 2023-07-06T17:40:19Z

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

      13 files -         7       13 suites - 7 6h 31m 30s ⏱️ - 4h 48m 26s
  3 718 tests +        2   3 607 ✔️ ±        0   108 💤 +    2 3 ❌ ±0
23 565 runs - 12 377 22 465 ✔️ - 11 723 1 097 💤 - 652 3 ❌ - 2

For more details on these failures, see this check.

Results for commit de43494. ± Comparison against base commit 8e3e0f6.

This pull request skips 2 tests.

distributed.deploy.tests.test_subprocess ‑ test_raise_on_windows
distributed.tests.test_metrics ‑ test_monotonic

♻️ This comment has been updated with latest results.

hendrikmakait · 2023-07-07T10:12:42Z

@jrbourbeau: I've added a FutureWarning for remove_worker. Since transition supports **kwargs, there's no strict need, though I'm wondering if we should still add a warning to alert users that the content of kwargs might change? I might be overthinking this.

fjetter · 2023-07-12T11:02:48Z

The entire purpose of kwargs is to tell people to implement their plugins in a forward compatible way. As long as we specify kwargs in the signature, we can freely add new paramters without affecting old users. Given our fragile plugin-API surface I suggest to keep kwargs and don't worry about any warnings for methods that had this in before.

Alerting about the content of kwargs seems counterintutive to me. If somebody cares aobut this, they should specify it as a proper argument

fjetter · 2023-07-12T11:04:41Z

distributed/diagnostics/plugin.py

    def remove_worker(
-        self, scheduler: Scheduler, worker: str
+        self, scheduler: Scheduler, worker: str, *, stimulus_id: str
    ) -> None | Awaitable[None]:


Suggested change

def remove_worker(

self, scheduler: Scheduler, worker: str

self, scheduler: Scheduler, worker: str, *, stimulus_id: str

) -> None | Awaitable[None]:

def remove_worker(

self, scheduler: Scheduler, worker: str, stimulus_id: str, **kwargs

) -> None | Awaitable[None]:

I suggest to not force the keyword and allow kwargs for future compat reasons, see #7974 (comment)

Forcing the keyword while adding **kwargs would make a method with the signature MySchedulerPlugin.remove_worker(self, scheduler, worker, **kwargs) fully forward-compatible, so I'd suggest sticking with it unless we also want to require *args in the signature.

docs/source/plugins.rst

hendrikmakait · 2023-07-12T11:22:11Z

The entire purpose of kwargs is to tell people to implement their plugins in a forward compatible way. As long as we specify kwargs in the signature, we can freely add new paramters without affecting old users. Given our fragile plugin-API surface I suggest to keep kwargs and don't worry about any warnings for methods that had this in before.

Alerting about the content of kwargs seems counterintutive to me. If somebody cares aobut this, they should specify it as a proper argument

Makes sense, I wasn't sure if the purpose of kwargs was to encourage forwrad-compatible implementations or to mimic the scheduler API. In the case of transition, Scheduler.transition itself takes kwargs because different transitions take different inputs.

I'll adjust this accordingly.

crusaderky

Minor tweak on deprecation

distributed/scheduler.py

Co-authored-by: crusaderky <crusaderky@gmail.com>

hendrikmakait · 2023-07-18T08:40:28Z

@crusaderky: All feedback has been addressed and a test added for **kwargs renaming. This is ready for another review.

…lugin.transition` (dask#7974) Co-authored-by: crusaderky <crusaderky@gmail.com> (cherry picked from commit d9a3457)

stimulus_id in remove_worker and transition

0e7b051

hendrikmakait requested review from fjetter and jacobtomlinson as code owners July 6, 2023 15:33

hendrikmakait changed the title ~~Pass stimulus_id in SchedulerPlugin.remove_worker and SchedulerPlugin.transition`~~ Pass stimulus_id to SchedulerPlugin.remove_worker and SchedulerPlugin.transition Jul 6, 2023

jrbourbeau reviewed Jul 6, 2023

View reviewed changes

hendrikmakait mentioned this pull request Jul 6, 2023

Automatically restart P2P shuffles when output worker leaves #7970

Merged

2 tasks

Tests

3be333e

hendrikmakait marked this pull request as draft July 6, 2023 17:28

hendrikmakait added 8 commits July 6, 2023 19:58

to kwarg

8412d2c

kwarg

ba67405

warning

9d804dc

forgiveness

0fd66cd

forgiveness

4f456e9

forgiveness

8f02f8d

Minor

29405c6

Move warning

476f132

hendrikmakait marked this pull request as ready for review July 7, 2023 10:13

hendrikmakait requested a review from jrbourbeau July 7, 2023 12:19

fjetter reviewed Jul 12, 2023

View reviewed changes

hendrikmakait added 3 commits July 13, 2023 09:38

Code review

bbfa1b9

kwargs in remove_worker

10126ff

Merge branch 'main' into stimuli-in-plugin-hooks

1482153

hendrikmakait requested a review from fjetter July 13, 2023 10:56

crusaderky requested changes Jul 17, 2023

View reviewed changes

distributed/scheduler.py Outdated Show resolved Hide resolved

distributed/scheduler.py Outdated Show resolved Hide resolved

Update distributed/scheduler.py

09d1bc5

Co-authored-by: crusaderky <crusaderky@gmail.com>

hendrikmakait and others added 2 commits July 17, 2023 14:31

Update distributed/scheduler.py

99dcea9

Co-authored-by: crusaderky <crusaderky@gmail.com>

linting

31dd11e

hendrikmakait requested a review from crusaderky July 17, 2023 16:55

Test kwargs renaming

de43494

fjetter approved these changes Jul 18, 2023

View reviewed changes

hendrikmakait merged commit d9a3457 into dask:main Jul 19, 2023

hendrikmakait deleted the stimuli-in-plugin-hooks branch July 19, 2023 10:18

phofl pushed a commit to phofl/distributed that referenced this pull request Jul 24, 2023

Pass stimulus_id to SchedulerPlugin.remove_worker and `SchedulerP…

fa89575

…lugin.transition` (dask#7974) Co-authored-by: crusaderky <crusaderky@gmail.com> (cherry picked from commit d9a3457)

hendrikmakait mentioned this pull request Jul 24, 2023

Pass stimulus_id to SchedulerPlugin hooks #7973

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Pass `stimulus_id` to `SchedulerPlugin.remove_worker` and `SchedulerPlugin.transition`#7974

Pass `stimulus_id` to `SchedulerPlugin.remove_worker` and `SchedulerPlugin.transition`#7974
hendrikmakait merged 17 commits intodask:mainfrom
hendrikmakait:stimuli-in-plugin-hooks

hendrikmakait commented Jul 6, 2023 •

edited

Loading

Uh oh!

jrbourbeau left a comment

Uh oh!

hendrikmakait commented Jul 6, 2023

Uh oh!

github-actions bot commented Jul 6, 2023 •

edited

Loading

Uh oh!

hendrikmakait commented Jul 7, 2023

Uh oh!

fjetter commented Jul 12, 2023

Uh oh!

fjetter Jul 12, 2023

Uh oh!

hendrikmakait Jul 13, 2023

Uh oh!

Uh oh!

hendrikmakait commented Jul 12, 2023

Uh oh!

crusaderky left a comment

Uh oh!

Uh oh!

Uh oh!

hendrikmakait commented Jul 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

hendrikmakait commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

hendrikmakait commented Jul 6, 2023

Uh oh!

github-actions bot commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

hendrikmakait commented Jul 7, 2023

Uh oh!

fjetter commented Jul 12, 2023

Uh oh!

fjetter Jul 12, 2023

Choose a reason for hiding this comment

Uh oh!

hendrikmakait Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hendrikmakait commented Jul 12, 2023

Uh oh!

crusaderky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hendrikmakait commented Jul 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hendrikmakait commented Jul 6, 2023 •

edited

Loading

github-actions bot commented Jul 6, 2023 •

edited

Loading