Scheduler.reschedule() works only by accident #6339

crusaderky · 2022-05-13T14:45:19Z

Supersedes #6307
Closes #6340

mrocklin · 2022-05-13T19:55:53Z

Looks like a valid failing test

crusaderky · 2022-05-13T20:32:35Z

Hilarious. The test passed when the worker was NOT calling Scheduler.reschedule due to mismatched signature. Now that it does, it breaks the state machine. Which has suddenly bumped up the importance and scope of this PR.

This now closes #6340.
I'm parking it for the time being.

github-actions · 2022-05-14T00:47:16Z

Unit Test Results

      15 files ±0       15 suites ±0 6h 52m 1s ⏱️ + 1m 22s
  2 775 tests ±0   2 695 ✔️ +1   78 💤 - 1 2 ❌ ±0
20 587 runs ±0 19 672 ✔️ - 7 906 💤 ±0 9 ❌ +7

For more details on these failures, see this check.

Results for commit 3f20835. ± Comparison against base commit 50d2911.

github-actions · 2022-06-27T23:40:06Z

Unit Test Results

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

      15 files ±0       15 suites ±0 10h 23m 2s ⏱️ + 1m 37s
  2 898 tests +1   2 812 ✔️ ±0   83 💤 ±0 3 ❌ +1
21 467 runs +9 20 502 ✔️ +2 962 💤 +6 3 ❌ +1

For more details on these failures, see this check.

Results for commit e7bb31b. ± Comparison against base commit a8eb3b2.

♻️ This comment has been updated with latest results.

crusaderky · 2022-06-28T12:57:36Z

distributed/tests/test_scheduler.py


-    for future in x:
-        s.reschedule(key=future.key)


This test remained green if you removed these lines

crusaderky · 2022-06-28T13:33:42Z

distributed/tests/test_scheduler.py

    await f
    s.set_restrictions(worker={f.key: a.address})
    assert s.tasks[f.key].worker_restrictions == {a.address}
-    s.reschedule(f)


This was silently doing nothing (future f is not in s.tasks). The task was executed only once, on b.

crusaderky · 2022-06-28T13:35:01Z

distributed/tests/test_worker.py

-async def test_reschedule(c, s, a, b):
-    await s.extensions["stealing"].stop()
+@pytest.mark.slow
+@pytest.mark.parametrize("long_running", [False, True])


Added test for _transition_long_running_rescheduled

hendrikmakait

Code looks good to me, feel free to ignore the comments.

hendrikmakait · 2022-06-29T15:48:23Z

distributed/scheduler.py

@@ -6568,7 +6568,9 @@ async def get_story(self, keys_or_stimuli: Iterable[str]) -> list[tuple]:

    transition_story = story

-    def reschedule(self, key=None, worker=None):
+    def reschedule(


Out-of-scope comment: Is there a better name that highlights that reschedule does not actually reschedule, i.e., it does not schedule the task somewhere else, it merely cancels the previous scheduling decision? For example, deschedule might be better.

Well no. A transition to released will automatically kick the task back to waiting.
The functional description of the method is accurate. The "released" bit is an implementation detail.
I'm adding a comment to explain.

Makes sense from that perspective, I was thinking that the function doesn't include the rescheduling bit, but in the end you're right, the releasing/descheduling automatically achieves the aim of rescheduling.

distributed/tests/test_scheduler.py

crusaderky · 2022-06-30T12:18:10Z

distributed/tests/test_reschedule.py

+    assert any(isinstance(ev, RescheduleEvent) for ev in a.state.stimulus_log)
+    assert all(f.key in b.data for f in futures)
+
+


the two tests below are new

crusaderky · 2022-06-30T12:19:54Z

distributed/worker_state_machine.py

@@ -2201,6 +2196,7 @@ def _transition_released_forgotten(
        ("cancelled", "missing"): _transition_cancelled_released,
        ("cancelled", "waiting"): _transition_cancelled_waiting,
        ("cancelled", "forgotten"): _transition_cancelled_forgotten,
+        ("cancelled", "rescheduled"): _transition_cancelled_released,


Without this test_cancelled_reschedule would cause a RecursionError

crusaderky · 2022-06-30T12:21:59Z

@hendrikmakait there have been substantial changes from your review; could you give it a second pass please?

hendrikmakait

Good catch with the rescheduling of cancelled tasks!

distributed/tests/test_reschedule.py

Co-authored-by: Hendrik Makait <[email protected]>

crusaderky self-assigned this May 13, 2022

crusaderky mentioned this pull request May 13, 2022

Support receiving stimulus_id's in Scheduler.reschedule #6307

Closed

2 tasks

crusaderky force-pushed the support-stimulus-id-in-reschedule branch from 78b81d8 to 0469601 Compare May 13, 2022 17:02

crusaderky marked this pull request as ready for review May 13, 2022 17:02

crusaderky mentioned this pull request May 13, 2022

Scheduler.reschedule() does not work #6340

Closed

crusaderky removed their assignment May 13, 2022

crusaderky changed the title ~~Support receiving stimulus_id's in Scheduler.reschedule~~ Scheduler.reschedule() works only by accident May 13, 2022

crusaderky marked this pull request as draft May 13, 2022 20:41

crusaderky force-pushed the support-stimulus-id-in-reschedule branch from d3dfa55 to 3f20835 Compare May 13, 2022 20:47

crusaderky force-pushed the support-stimulus-id-in-reschedule branch 2 times, most recently from 2e4522e to 611c85d Compare June 27, 2022 22:38

crusaderky force-pushed the support-stimulus-id-in-reschedule branch from 611c85d to b3ffa87 Compare June 28, 2022 12:52

crusaderky commented Jun 28, 2022

View reviewed changes

crusaderky self-assigned this Jun 28, 2022

crusaderky marked this pull request as ready for review June 28, 2022 13:55

crusaderky requested review from fjetter, gjoseph92 and graingert June 28, 2022 13:55

hendrikmakait self-requested a review June 29, 2022 15:31

hendrikmakait approved these changes Jun 29, 2022

View reviewed changes

crusaderky added 3 commits June 30, 2022 11:36

Fix RescheduleMsg

6999e42

annotations

ddf06f1

doc

c857e92

crusaderky force-pushed the support-stimulus-id-in-reschedule branch from e7bb31b to f0bf899 Compare June 30, 2022 11:15

crusaderky force-pushed the support-stimulus-id-in-reschedule branch from f0bf899 to 950e4bc Compare June 30, 2022 11:27

Break the state machine

2fe8146

crusaderky force-pushed the support-stimulus-id-in-reschedule branch from 950e4bc to 355cc0f Compare June 30, 2022 12:16

crusaderky commented Jun 30, 2022

View reviewed changes

crusaderky requested a review from hendrikmakait June 30, 2022 12:21

Refactor tests

9410072

crusaderky force-pushed the support-stimulus-id-in-reschedule branch from 355cc0f to 9410072 Compare June 30, 2022 12:21

hendrikmakait approved these changes Jun 30, 2022

View reviewed changes

distributed/tests/test_reschedule.py Outdated Show resolved Hide resolved

Update distributed/tests/test_reschedule.py

2e06275

Co-authored-by: Hendrik Makait <[email protected]>

crusaderky merged commit ff250f2 into dask:main Jun 30, 2022

crusaderky deleted the support-stimulus-id-in-reschedule branch June 30, 2022 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduler.reschedule() works only by accident #6339

Scheduler.reschedule() works only by accident #6339

crusaderky commented May 13, 2022 •

edited

Loading

mrocklin commented May 13, 2022

crusaderky commented May 13, 2022

github-actions bot commented May 14, 2022

github-actions bot commented Jun 27, 2022 •

edited

Loading

crusaderky Jun 28, 2022

crusaderky Jun 28, 2022 •

edited

Loading

crusaderky Jun 28, 2022

hendrikmakait left a comment

hendrikmakait Jun 29, 2022

crusaderky Jun 30, 2022

hendrikmakait Jun 30, 2022

crusaderky Jun 30, 2022

crusaderky Jun 30, 2022

crusaderky commented Jun 30, 2022

hendrikmakait left a comment

		assert any(isinstance(ev, RescheduleEvent) for ev in a.state.stimulus_log)
		assert all(f.key in b.data for f in futures)

Scheduler.reschedule() works only by accident #6339

Scheduler.reschedule() works only by accident #6339

Conversation

crusaderky commented May 13, 2022 • edited Loading

mrocklin commented May 13, 2022

crusaderky commented May 13, 2022

github-actions bot commented May 14, 2022

Unit Test Results

github-actions bot commented Jun 27, 2022 • edited Loading

Unit Test Results

crusaderky Jun 28, 2022

Choose a reason for hiding this comment

crusaderky Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

crusaderky Jun 28, 2022

Choose a reason for hiding this comment

hendrikmakait left a comment

Choose a reason for hiding this comment

hendrikmakait Jun 29, 2022

Choose a reason for hiding this comment

crusaderky Jun 30, 2022

Choose a reason for hiding this comment

hendrikmakait Jun 30, 2022

Choose a reason for hiding this comment

crusaderky Jun 30, 2022

Choose a reason for hiding this comment

crusaderky Jun 30, 2022

Choose a reason for hiding this comment

crusaderky commented Jun 30, 2022

hendrikmakait left a comment

Choose a reason for hiding this comment

crusaderky commented May 13, 2022 •

edited

Loading

github-actions bot commented Jun 27, 2022 •

edited

Loading

crusaderky Jun 28, 2022 •

edited

Loading