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.

Sign up for GitHub

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

Jump to bottom

Hide waiting tasks from n=0. #3823

Merged
merged 6 commits into from
Nov 2, 2020
Merged

Hide waiting tasks from n=0. #3823

merged 6 commits into from
Nov 2, 2020

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Sep 17, 2020 •
edited
Loading

(The above two issues are closely related through how we handle partially-satisfied waiting tasks here and/or on master)

Also removes several checkpoint tests rather than adapt them to this branch, because they're about to be deleted anyway.

Requirements 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).
  • Already covered by existing tests.
  • Appropriate change log entry included.
  • No documentation update required. (Will be covered by general Cylc 8 docs on n=windows)
  • No dependency changes.

@hjoliver hjoliver added this to the cylc-8.0.0 milestone Sep 17, 2020
@hjoliver hjoliver self-assigned this Sep 17, 2020
@hjoliver hjoliver marked this pull request as draft September 17, 2020 04:56
@hjoliver hjoliver mentioned this pull request Oct 11, 2020
Closed
13 tasks
@hjoliver hjoliver mentioned this pull request Oct 21, 2020
Merged
11 tasks
@hjoliver hjoliver added POC Proof of Concept sod-follow-up and removed POC Proof of Concept labels Oct 27, 2020
hjoliver
hjoliver commented Oct 27, 2020
View reviewed changes
cylc/flow/scheduler.py Outdated
@@ -1672,6 +1671,7 @@ async def shutdown(self, reason):
self.proc_pool.process()

if self.pool is not None:
self.pool.report_unmet_deps()
Copy link
Member Author

Choose a reason for hiding this comment

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

Log any partially satisfied tasks at shutdown as well as stall.

hjoliver
hjoliver commented Oct 27, 2020
View reviewed changes
cylc/flow/scheduler.py
@@ -1525,7 +1524,7 @@ async def update_data_structure(self):
updated_nodes = set(updated_tasks).union(
self.pool.get_pool_change_tasks())
if (
has_updated or
updated_nodes or
Copy link
Member Author

Choose a reason for hiding this comment

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

The datastore wasn't updating on some changes (noticed via tests that use cylc dump, which now goes via GraphQL to the datastore).

@hjoliver hjoliver force-pushed the hide-waiting-tasks branch 2 times, most recently from 0d105f7 to 00b9f93 Compare October 28, 2020 02:48
@hjoliver hjoliver marked this pull request as ready for review October 28, 2020 10:45
hjoliver
hjoliver commented Oct 28, 2020
View reviewed changes
cylc/flow/task_pool.py
@@ -176,7 +176,6 @@ def __init__(self, config, suite_db_mgr, task_events_mgr, job_pool):

self.is_held = False
self.hold_point = None
self.stuck_future_tasks = []
Copy link
Member Author

Choose a reason for hiding this comment

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

Stuck future tasks are another artifact of the old SoS algorithm: a waiting task gets spawned even though it depends on another task that is beyond the stop point. Under SoD they don't get spawned (or if they have other non-future prereqs as well, they will only be spawned as hidden partially-satisfied tasks).

dwsutherland
dwsutherland approved these changes Oct 29, 2020
View reviewed changes
Copy link
Member

@dwsutherland dwsutherland left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍

It does make the current data-store jump around a bit, but that will be dealt with soon (#3811), although (after testing with this branch), it will also need a little work.

oliver-sanders
oliver-sanders reviewed Oct 29, 2020
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Non-blocking log format improvements:

cylc/flow/task_pool.py Outdated Show resolved Hide resolved
cylc/flow/task_pool.py Outdated Show resolved Hide resolved
cylc/flow/task_pool.py Outdated Show resolved Hide resolved
cylc/flow/task_pool.py Outdated Show resolved Hide resolved
hjoliver and others added 5 commits October 30, 2020 10:08
@hjoliver @oliver-sanders
Update cylc/flow/task_pool.py
7a50e88 
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
@hjoliver @oliver-sanders
Update cylc/flow/task_pool.py
bcbdd12 
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
@hjoliver @oliver-sanders
Update cylc/flow/task_pool.py
faf166a 
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
@hjoliver @oliver-sanders
Update cylc/flow/task_pool.py
2932982 
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
@hjoliver
Fix log message fixes.
131ec15
@hjoliver
Copy link
Member Author

Log message changes applied @oliver-sanders (although they required some tweaking).

oliver-sanders
oliver-sanders approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Works as expected for my examples.

Leaving un-merged incase you want to add a changelog, however, since SoD hasn't made release yet I don't think it's required.

@hjoliver hjoliver mentioned this pull request Nov 2, 2020
Closed
@hjoliver
Copy link
Member Author

hjoliver commented Nov 2, 2020

(I will update the SoD proposal to reflect this change)

@hjoliver hjoliver merged commit 30b0b52 into cylc:master Nov 2, 2020
@hjoliver hjoliver deleted the hide-waiting-tasks branch November 2, 2020 21:29
dwsutherland added a commit to dwsutherland/cylc-flow that referenced this pull request Nov 4, 2020
@dwsutherland
post cylc#3823 fixes
7775878
dwsutherland added a commit to dwsutherland/cylc-flow that referenced this pull request Nov 4, 2020
@dwsutherland
post cylc#3823 fixes
3e749aa
dwsutherland added a commit to dwsutherland/cylc-flow that referenced this pull request Nov 5, 2020
@dwsutherland
post cylc#3823 fixes
0e1e4e4
dwsutherland added a commit to dwsutherland/cylc-flow that referenced this pull request Nov 6, 2020
@dwsutherland
post cylc#3823 fixes
73e59d6
dwsutherland added a commit to dwsutherland/cylc-flow that referenced this pull request Nov 10, 2020
@dwsutherland
post cylc#3823 fixes
e2a411d
@oliver-sanders oliver-sanders modified the milestones: cylc-8.0.0, cylc-8.0a3 Nov 10, 2020
dwsutherland added a commit to dwsutherland/cylc-flow that referenced this pull request Nov 10, 2020
@dwsutherland
post cylc#3823 fixes
c755676
@hjoliver hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliver-sanders oliver-sanders oliver-sanders approved these changes

@dwsutherland dwsutherland dwsutherland approved these changes

Assignees

@hjoliver hjoliver

Labels
None yet
Projects
None yet
Milestone
cylc-8.0b0
Development

Successfully merging this pull request may close these issues.

Correct stall diagnosis Take waiting tasks out of the n=0 window
3 participants
@hjoliver @dwsutherland @oliver-sanders