Skip to content

Edge and impossible transitions to memory #7205

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

Merged
merged 13 commits into from
Nov 2, 2022

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Oct 27, 2022

await ev.wait()
s = next(si for si in Scheduler._instances if id(si) == sched_id)
# Note that the task is async, so it runs in the same thread as the scheduler
s.reschedule("x-0", stimulus_id="steal")
Copy link
Member

Choose a reason for hiding this comment

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

As pointed out in #7200 (comment)
this is not how stealing works and using Scheduler.reschedule in this way is not intended use. If this is the only way to provoke this transition, we should remove Scheduler.reschedule (or rather make it private, users should raise the Reschedule exception.

@fjetter
Copy link
Member

fjetter commented Oct 27, 2022

I'd like us to add a more realistic reproducer to show this can actually occur before adding this transition

@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2022

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   6h 14m 13s ⏱️ + 2m 57s
  3 168 tests +    1    3 083 ✔️ +  13    84 💤  - 11  1  - 1 
23 440 runs  +643  22 535 ✔️ +689  902 💤  - 44  3  - 2 

For more details on these failures, see this check.

Results for commit 8fb3dab. ± Comparison against base commit c137ac0.

♻️ This comment has been updated with latest results.

@crusaderky
Copy link
Collaborator Author

Discussion on #7200: I'd like to remove the test altogether, merge the change untested, and open a follow-up for a proper investigation and coverage of all use cases of unexpected transitions to memory.

@gjoseph92
Copy link
Collaborator

@crusaderky crusaderky changed the title queued->memory transition Edge and impossible transitions to memory Nov 1, 2022
@crusaderky crusaderky self-assigned this Nov 1, 2022
@crusaderky
Copy link
Collaborator Author

All test failures are unrelated. Ready for review and merge.

@crusaderky crusaderky marked this pull request as ready for review November 1, 2022 14:20
("queued", "released"): transition_queued_released,
("queued", "processing"): transition_queued_processing,
("processing", "released"): transition_processing_released,
("processing", "memory"): transition_processing_memory,
("processing", "erred"): transition_processing_erred,
("no-worker", "released"): transition_no_worker_released,
("no-worker", "processing"): transition_no_worker_processing,
("no-worker", "memory"): transition_no_worker_memory,
("no-worker", "memory"): partial(impossible_transition, "memory"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the value of having a transition function for an impossible transition, versus updating

raise RuntimeError(f"Impossible transition from {start} to {finish}")

to raise a more detailed error like you have here (including the story)?

Copy link
Collaborator Author

@crusaderky crusaderky Nov 2, 2022

Choose a reason for hiding this comment

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

The idea was to avoid the error from being silenced in the future if anybody adds a (released, memory) transition. But on second thought I'll just overhaul the above.

Copy link
Collaborator

@gjoseph92 gjoseph92 left a comment

Choose a reason for hiding this comment

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

Nice, thanks for figuring all this out

@gjoseph92
Copy link
Collaborator

Only failing:

@crusaderky crusaderky merged commit 02b9430 into dask:main Nov 2, 2022
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.

Investigate and remove unusual scheduler transitions to memory Transition queued->memory causes AssertionError
3 participants