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

Fix KeyError when using cylc remove after a reload changed the graph #6516

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Dec 5, 2024

Closes #6502

Unreleased bug. To reproduce:

  1. Start a workflow a & a:started => b where a is either a long running task or is set to fail
  2. Change the graph to just b and reload the workflow
  3. Do cylc remove a
  4. This results in a KeyError which crashes the scheduler

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • No dependency changes
  • Tests are included
  • No changelog entry needed as unreleased bug
  • No docs needed
  • Master branch is correct

@MetRonnie MetRonnie added the bug Something is wrong :( label Dec 5, 2024
@MetRonnie MetRonnie added this to the 8.4.0 milestone Dec 5, 2024
@MetRonnie MetRonnie self-assigned this Dec 5, 2024
cylc/flow/scheduler.py Outdated Show resolved Hide resolved
@MetRonnie MetRonnie force-pushed the cylc-remove branch 2 times, most recently from f6b5e35 to 231f7ee Compare December 5, 2024 12:43
@@ -2327,7 +2327,7 @@ def filter_task_proxies(
ids: Iterable[str],
warn_no_active: bool = True,
inactive: bool = False,
) -> 'Tuple[List[TaskProxy], Set[Tuple[str, PointBase]], List[str]]':
) -> 'Tuple[List[TaskProxy], Set[Tuple[TaskDef, PointBase]], List[str]]':
Copy link
Member Author

Choose a reason for hiding this comment

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

One thing to note with this change is that the hashability/equality of TaskDefs is purely reference based (i.e. __hash__() and __eq__() fall back to object default). This should be fine as the TaskDefs are always coming from the scheduler.config.taskdefs dictionary, I believe

Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

:)

  • Reproduced bug
  • Checked that it fixed bug
  • Read code
  • Read tests
  • Got irked that "read" is pronounced differently dependent on tense without changing spelling

@hjoliver
Copy link
Member

hjoliver commented Dec 10, 2024

Got irked that "read" is pronounced differently dependent on tense without changing spelling

🤣 fair play

@wxtim wxtim merged commit d21c9e5 into cylc:master Dec 11, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove: handle reload
3 participants