Apply prerequisite changes to spawned tasks, on reload & restart #5334

hjoliver · 2023-01-30T11:47:45Z

On restart we assume that all prerequisites of a spawned task are recorded in the DB. This will not be the case on restarting after adding new dependencies to that task.

Example:

[scheduling]
    [[graph]]
        R1 = """
            a => z
            b => z
            # c => z
        """
[runtime]
    [[a]]
         script = "sleep 20"
    [[b, c, z]]

To reproduce the bug:

install and play
do cylc stop --now when a is still running and z has been spawned by b:succeeded
uncomment # c => z to add a new prerequisite (c) task for z
reinstall and play: scheduler will crash

The fix: if a prerequisite does not exist in the DB, assume that it is unsatisfied.

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).
Applied any dependency changes to both setup.cfg and conda-environment.yml.
Tests are included (or explain why tests are not needed).
CHANGES.md entry included if this is a change that can affect users
Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

hjoliver · 2023-01-30T22:25:31Z

(Rebased to pick up #5333)

* Tests cylc#5334

oliver-sanders · 2023-02-08T16:55:20Z

This fix looks good to me, I've added an integration test for this (restarts are possible, they just require a little fiddling):

hjoliver#30

I haven't looked into removing prereqs or reloads which I think we should also be testing? Or are there other tests for this?

hjoliver · 2023-02-08T21:37:40Z

Thanks for the integration test @oliver-sanders

hjoliver · 2023-02-08T22:08:21Z

I haven't looked into removing prereqs ...

This works already, for restart. Checking for tests ... nope. I've added a tweaked copy of your new integration test to cover this.

hjoliver · 2023-02-08T23:13:43Z

I expect the equivalent reload will be fine. Already-spawned task proxies do have prerequisites copied on reload, and it was just the DB check on restart that caused the problem... NOPE, prerequisites are not copied over intelligently 😬

hjoliver · 2023-02-09T06:23:44Z

(Reload fixed, still working on the integration test for that ...)

hjoliver · 2023-02-10T03:49:25Z

After considerable pfaffing about, this should be good to go now.

hjoliver · 2023-02-10T03:50:21Z

@dwsutherland - could you review at this end? It touches the datastore a bit.

oliver-sanders · 2023-02-16T15:46:18Z

I tried testing with this example:

[scheduler]                    
    allow implicit tasks = True    
                               
[scheduling]                   
    initial cycle point = 1    
    cycling mode = integer    
    [[graph]]               
        P1 = """            
            a => b          
            a => c          
        """

Then added this dependency:

            a[-P1] => a

And reloaded.

Cylc didn't crash, but the workflow became stuck (no stall event detected):

Note 34/a had succeeded and left the pool before the reload happened. I think the missing DB prereq entry results in the dep being marked as unsatisfied incorrectly.

Rather than defaulting to False should we run it through the DB or re-spawn the dependency to run it through the DB somehow?

oliver-sanders · 2023-02-17T10:52:29Z

The issue I mentioned above isn't great, but that said, it is a lot better than crashing. If it's difficult we could bump the DB stuff to a later PR as the pressure on release is quite high due to the GUI bug.

* Accompanies cylc#5334 which fixes traceback for missing prereqs on reload/restart by marking them as unsatisfied.

oliver-sanders · 2023-03-01T13:10:33Z

Gotta fix here: hjoliver#31

However, we still need to work out how to square this with the data store which still defaults to False after my change.

hjoliver · 2023-03-10T00:57:16Z

(Thanks, just back to this now, I'll take a look at the problem you found, and your fix ...)

hjoliver · 2023-03-10T01:18:37Z

OK what's going on is, a is initially parent-less so it gets spawned to the runahead limit. Then after the reload it has a parent (it's own previous instance) and we update the already-spawned instances to give them the new prerequisite. The new prerequisite should be satisfied for the first instance, but it isn't - because under SoD, prerequisite status only gets updated on demand, at the instant the corresponding upstream output gets completed.

hjoliver · 2023-03-10T03:06:39Z

Agreed with your fix @oliver-sanders ; merged it to this branch.

One small issue, I don't think we can provide the prerequsite satisfied message accurately i.e. we can't tell the difference between succeeded naturally and whatever the alternative is. However, this is minor.

Yeah, minor. I think "satisfied naturally" is the only one used at the moment in Cylc 8 anyhow. Although that will change with https://cylc.github.io/cylc-admin/proposal-cylc-set.html

hjoliver · 2023-03-10T03:25:24Z

From the side-PR:

I'm not sure how to square this with the data store without duplicating the database calls.

Presumably I just need to call delta_task_prerequisite(task) in the datastore manager.

This PR marks all satisfied prerequisites as satisfied naturally. I think the information is lost by this point, is there a more suitable value e.g satisfied from DB?

(See above) This isn't used for anything at the moment. For now I'll just make something up to distinguish the two options.

TODO

update datastore
new test for this case
"satisfied by DB" (or similar)

oliver-sanders · 2023-03-10T16:01:33Z

We had someone hit this. Strangely when they reverted the diff and ran vr again, the workflow started up with an empty task pool???

Not sure why that happened, doesn't sound right. The good news is it was recoverable with a bit of triggering. This did remind me of the importance of these two issues:

Both of which hit us whilst trying to recover the workflow.

oliver-sanders · 2023-04-24T15:02:03Z

Gotta solution to the last three bullet points here - hjoliver#32

Sets the prereq message to "satisfied from database".
Updates the data store when prereqs change on reload (the restart case was already handled).
Shims this into the existing tests.

One conflict to shift, then I think this is good to go.

hjoliver · 2023-04-25T22:10:09Z

Gotta solution to the last three bullet points here - hjoliver#32

Great, thanks.

* Tests cylc#5334

* Accompanies cylc#5334 which fixes traceback for missing prereqs on reload/restart by marking them as unsatisfied.

hjoliver · 2023-04-25T23:09:50Z

@oliver-sanders - applied your tweaks and rebased.

_remote_background functional tests failing unrelated infrastructure reasons.

oliver-sanders · 2023-04-26T09:00:47Z

CHANGES.md

@@ -68,6 +68,15 @@ Rose options (`-O`, `-S` & `-D`) with `cylc view`.
 [#5363](https://github.com/cylc/cylc-flow/pull/5363) Improvements and bugfixes
 for `cylc lint`.

+-------------------------------------------------------------------------------
+## __cylc-8.1.2 (<span actions:bind='release-date'>Upcoming</span>)__


Should be 8.1.3.

wxtim · 2023-04-26T10:16:32Z

tests/integration/test_task_pool.py

+    'graph_1, graph_2, '
+    'expected_1, expected_2, expected_3, expected_4',
+    [
+        param(  # Restart after adding a prerequisite to task z


IMO these should have an id= kwarg -

Imagine trying to run

pytest 'tests/i/test_task_pool.py::test_restart_prereqs[a => z\n b => z-a => z\n b => z\n c => z-expected_10-expected_20-expected_30-expected_40]'

wxtim · 2023-04-26T10:22:15Z

tests/integration/test_task_pool.py

+               b => z
+               c => z''',
+            [
+                ('1', 'a', 'running'),


Given that everything is at CP = 1, could we just test output[1:] = expect and not include the '1'?

It's marginal, but I think it cuts down on the amount written. Don't care that much.

wxtim

Approving - It looks good - I've made a couple of stylistic points, but

Reproduced the bug, failed to reproduce it on branch.
Read Code

…_too_soon * upstream/master: (70 commits) Tweak scan CLI help. (cylc#5405) Add support for python 3.11 (cylc#5497) tests/u: add parameter ids build(deps): bump pypa/gh-action-pypi-publish from 1.8.5 to 1.8.6 prerequisite: remove unused interface Fix change log dup section. Fix flaky test Update CONTRIBUTING.md Tidy platforms modify CONTRIBUTING.md Bump dev version (cylc#5501) Prepare release 8.1.3 Ignore off-sequence parents, for datastore. (cylc#5495) Avoid duplicate prerequisites from multiple recurrences. (cylc#5466) Update cylc/flow/prerequisite.py prerequsite: remove target_point_strings attribute tests/f: fix events/11 swarm: convert cylc-dev Docker image to run on ubuntu:latest fix changelog Apply prerequisite changes to spawned tasks, on reload & restart (cylc#5334) ...

…too_soon * upstream/8.1.x: (46 commits) Bump dev version (cylc#5518) Prepare release 8.1.4 cat-log: fix issues with file listing Update changelog Fix `eval_host()` localhost bug & plug testing gap Bump dev version (cylc#5501) Prepare release 8.1.3 Ignore off-sequence parents, for datastore. (cylc#5495) Avoid duplicate prerequisites from multiple recurrences. (cylc#5466) tests/f: fix events/11 swarm: convert cylc-dev Docker image to run on ubuntu:latest fix changelog Apply prerequisite changes to spawned tasks, on reload & restart (cylc#5334) cat-log: don't list directories which don't exist (cylc#5488) task_job_mgr: move compute out of open call manylinux tests: update to ubuntu-20.04 Update changelog Address review Fix.platform is regex remote tidy fail (cylc#5445) parsec: better error messages for section/setting mixups ...

hjoliver added the bug Something is wrong :( label Jan 30, 2023

hjoliver self-assigned this Jan 30, 2023

hjoliver force-pushed the fix-prereq-bug branch 2 times, most recently from a45e438 to 5a8f332 Compare January 30, 2023 22:24

hjoliver force-pushed the fix-prereq-bug branch from 5a8f332 to c154521 Compare January 30, 2023 22:27

wxtim requested review from oliver-sanders and removed request for oliver-sanders January 31, 2023 11:53

oliver-sanders added this to the cylc-8.1.2 milestone Jan 31, 2023

hjoliver changed the base branch from master to 8.1.x February 8, 2023 02:57

hjoliver force-pushed the fix-prereq-bug branch from c154521 to 98c230d Compare February 8, 2023 02:57

oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this pull request Feb 8, 2023

restart: test prereq changes are applied on restart

46de15e

* Tests cylc#5334

oliver-sanders mentioned this pull request Feb 8, 2023

restart: test prereq changes are applied on restart hjoliver/cylc-flow#30

Merged

hjoliver requested a review from oliver-sanders February 8, 2023 21:29

hjoliver marked this pull request as ready for review February 8, 2023 21:57

hjoliver force-pushed the fix-prereq-bug branch 2 times, most recently from 637cee2 to 4273862 Compare February 10, 2023 01:28

hjoliver changed the title ~~Fix restart after adding new deps to spawned task.~~ Apply prerequisite changes to spawned tasks, on reload & restart Feb 10, 2023

hjoliver requested a review from dwsutherland February 10, 2023 03:49

oliver-sanders modified the milestones: cylc-8.1.2, cylc-8.1.3 Feb 20, 2023

oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this pull request Mar 1, 2023

pool: load missing prerequisite status from database

34db233

* Accompanies cylc#5334 which fixes traceback for missing prereqs on reload/restart by marking them as unsatisfied.

oliver-sanders mentioned this pull request Apr 24, 2023

task pool: update data store when prereqs change on reload hjoliver/cylc-flow#32

Merged

hjoliver and others added 10 commits April 26, 2023 10:20

Fix restart after adding new deps to spawned task.

ed4f57a

restart: test prereq changes are applied on restart

cbdd0cd

* Tests cylc#5334

Update change log.

11f9d15

Add an integration test.

8b4db42

Fix reload after prereq change in the graph.

736bf23

Up test coverage.

84524db

pool: load missing prerequisite status from database

985f6e2

* Accompanies cylc#5334 which fixes traceback for missing prereqs on reload/restart by marking them as unsatisfied.

tests/i: replace _make_flow with flow

dd4d975

Remove unused import.

b53b4ad

task pool: update data store when prereqs change on reload

098dee6

hjoliver force-pushed the fix-prereq-bug branch from c671bcd to 098dee6 Compare April 25, 2023 22:22

oliver-sanders reviewed Apr 26, 2023

View reviewed changes

wxtim reviewed Apr 26, 2023

View reviewed changes

wxtim approved these changes Apr 26, 2023

View reviewed changes

wxtim merged commit c8f9c23 into cylc:8.1.x Apr 26, 2023

hjoliver deleted the fix-prereq-bug branch April 26, 2023 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply prerequisite changes to spawned tasks, on reload & restart #5334

Apply prerequisite changes to spawned tasks, on reload & restart #5334

hjoliver commented Jan 30, 2023 •

edited

Loading

hjoliver commented Jan 30, 2023 •

edited

Loading

oliver-sanders commented Feb 8, 2023

hjoliver commented Feb 8, 2023 •

edited

Loading

hjoliver commented Feb 8, 2023 •

edited

Loading

hjoliver commented Feb 8, 2023 •

edited

Loading

hjoliver commented Feb 9, 2023

hjoliver commented Feb 10, 2023

hjoliver commented Feb 10, 2023

oliver-sanders commented Feb 16, 2023 •

edited

Loading

oliver-sanders commented Feb 17, 2023

oliver-sanders commented Mar 1, 2023

hjoliver commented Mar 10, 2023

hjoliver commented Mar 10, 2023 •

edited

Loading

hjoliver commented Mar 10, 2023

hjoliver commented Mar 10, 2023

oliver-sanders commented Mar 10, 2023

oliver-sanders commented Apr 24, 2023 •

edited

Loading

hjoliver commented Apr 25, 2023

hjoliver commented Apr 25, 2023

oliver-sanders Apr 26, 2023

wxtim Apr 26, 2023

wxtim Apr 26, 2023

wxtim left a comment

Apply prerequisite changes to spawned tasks, on reload & restart #5334

Apply prerequisite changes to spawned tasks, on reload & restart #5334

Conversation

hjoliver commented Jan 30, 2023 • edited Loading

hjoliver commented Jan 30, 2023 • edited Loading

oliver-sanders commented Feb 8, 2023

hjoliver commented Feb 8, 2023 • edited Loading

hjoliver commented Feb 8, 2023 • edited Loading

hjoliver commented Feb 8, 2023 • edited Loading

hjoliver commented Feb 9, 2023

hjoliver commented Feb 10, 2023

hjoliver commented Feb 10, 2023

oliver-sanders commented Feb 16, 2023 • edited Loading

oliver-sanders commented Feb 17, 2023

oliver-sanders commented Mar 1, 2023

hjoliver commented Mar 10, 2023

hjoliver commented Mar 10, 2023 • edited Loading

hjoliver commented Mar 10, 2023

hjoliver commented Mar 10, 2023

oliver-sanders commented Mar 10, 2023

oliver-sanders commented Apr 24, 2023 • edited Loading

hjoliver commented Apr 25, 2023

hjoliver commented Apr 25, 2023

oliver-sanders Apr 26, 2023

Choose a reason for hiding this comment

wxtim Apr 26, 2023

Choose a reason for hiding this comment

wxtim Apr 26, 2023

Choose a reason for hiding this comment

wxtim left a comment

Choose a reason for hiding this comment

hjoliver commented Jan 30, 2023 •

edited

Loading

hjoliver commented Jan 30, 2023 •

edited

Loading

hjoliver commented Feb 8, 2023 •

edited

Loading

hjoliver commented Feb 8, 2023 •

edited

Loading

hjoliver commented Feb 8, 2023 •

edited

Loading

oliver-sanders commented Feb 16, 2023 •

edited

Loading

hjoliver commented Mar 10, 2023 •

edited

Loading

oliver-sanders commented Apr 24, 2023 •

edited

Loading