Store task prerequisites in their own DB table #3863

MetRonnie · 2020-10-13T11:15:59Z

These changes close #3752

Instead of storing task prerequisites as a JSON string inside the task_pool table, remove the satisfied column from task_pool and introduce a task_prerequisites table.

Note: This PR removes the ability to restart a Cylc 7 suite.

For the flow (where foo.1 fails)

P1 = foo & ipsum:custom1 => bar

the task_prerequisites table has the form:

cycle* TEXT	name* TEXT	prereq_name* TEXT	prereq_cycle* TEXT	prereq_output* TEXT	satisfied TEXT
1	bar	foo	1	succeeded	0
1	bar	ipsum	1	I'm a custom task message	satisfied naturally

where asterisk in the column header denotes primary key.

A task's prerequisites are inserted into the task_prerequisites table when the task is added to the runahead pool (if a task has no prerequisites, no entry is inserted to the task_prerequisites table). The task_prerequisites table is updated at the same time as before when the prerequisites used to be updated in the task_pool table.

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).

Appropriate tests are included (functional).

Change log entry included

No documentation update required.

No dependency changes.

cylc/flow/rundb.py

cylc/flow/unicode_rules.py

oliver-sanders · 2020-10-13T11:26:52Z

Looks good to me, note to others, this change does not have an upgrader for the database. We have abandoned the idea of maintaining restart compatibility with Cylc7 due to installation changes amongst other things so this is fine. Just be aware that cylc restart will not work with previously run workflows on this branch.

MetRonnie · 2020-10-13T12:33:11Z

Does that mean that the following test:

cylc-flow/tests/functional/retries/03-upgrade.t

Lines 18 to 27 in 96f5603

    
           # Compat: Cylc<8 
        
           # Test the upgrade of the old *retrying states to the new xtrigger based 
        
           # retry mechanism. 
        
           . "$(dirname "$0")/test_header" 
        
           set_test_number 10 
        
           install_suite 
        
           # install the cylc7 restart database 
        
           cp "${TEST_SOURCE_DIR}/${TEST_NAME_BASE}/.service/db" \ 
        
               "${HOME}/cylc-run/${SUITE_NAME}/.service/db"

which is failing due to

sqlite3.OperationalError: table task_pool_checkpoints has 7 columns but 6 values were supplied

can be removed?

oliver-sanders · 2020-10-13T12:39:18Z

Sure, but if removing, remove all upgraders and their associated tests.

MetRonnie · 2020-10-13T17:53:22Z

Still need to detect when trying to restart a Cylc 7 suite and give a useful error message

MetRonnie · 2020-10-14T10:23:02Z

Would it be worth renaming the db file in order to detect when the user is trying to restart a Cylc < 8.0a3 suite?

oliver-sanders · 2020-10-14T12:16:17Z

Would it be worth renaming the db file in order to detect when the user is trying to restart a Cylc < 8.0a3 suite?

The Cylc version of the current run (even if the suite is stopped it can still be thought of as the current "run") has been stored in the database since Cylc 7.something:

$ sqlite3 <path-to-suite>/.service/db 'SELECT value FROM suite_params WHERE key = "cylc_version";'

Wasn't expecting this as part of this PR but if you're happy to take a crack at it that would be great. If the SQL execution fails then assume that the Cylc version is less than 8.

MetRonnie · 2020-10-14T15:17:55Z

Is there any reason not to rename the db file, though? It would make the logic simpler

oliver-sanders · 2020-10-14T15:24:06Z

Yes, people may be using it, if an SQL call is too much work why not use the presence of the flow.cylc file?

Note we don't really need to worry about inter-version compatibility between alpha releases.

tests/functional/restart/54-incompatible-db.t

cylc/flow/suite_db_mgr.py

oliver-sanders

Looks good, works ok.

One thing left to do, the task_pool table is housekept so as to keep it in sync with the task pool. The same logic will need to apply to the task_prereqs table otherwise it will continue to grow indefinitely.

Had a quick fish about in the code and it looks like the suite_db_mgr wipes and rebuilds the task_pool and it's related tables for each batch of changes 🤮. The rundb performs all queued insertions and deletions in the same transition so that's probably not as bad as it sounds (depending on the implementation of the database), presumably just means sqlite has to do a bit more legwork to compute what's actually changed?

Probs best to turn a blind eye to this and just fit the task_prerequsites table into the existing global-update pattern. Perhaps in a later Cylc8 version as we try to make the main loop more async we will make DB updates more event-driven and remove this per-loop global-update logic in the process.

cylc/flow/suite_db_mgr.py

MetRonnie · 2020-10-16T12:37:04Z

Probs best to turn a blind eye to this and just fit the task_prerequsites table into the existing global-update pattern.

Like this: 01e0798 ?

MetRonnie · 2020-10-16T15:18:25Z

@oliver-sanders Looks like that will need a task_prerequisites_checkpoints table, the absence of which seems to be causing tests/k/restart/19-checkpoint.t to fail now

oliver-sanders · 2020-10-16T15:34:33Z

Ah, perhaps disable that test for now and add a comment to the checkpoints issue.

oliver-sanders · 2020-10-21T08:07:20Z

To checkpoint or not to checkpoint?!

hjoliver · 2020-10-22T21:57:47Z

BLOCKED by #3864

oliver-sanders · 2020-10-23T12:03:41Z

Can we just disable these tests for now and add a note to #3864 for now, then reinstate checkpoint functionality later if required?

hjoliver · 2020-10-24T02:11:48Z

Can we just disable these tests for now and add a note to #3864 for now, then reinstate checkpoint functionality later if required?

We could, but is there any reason we can't make a quick decision on removing named checkpointing. All the facts are "on the table" already.

MetRonnie · 2020-10-26T15:01:20Z

If named checkpoints are removed does that mean automatic checkpoints are also removed?

oliver-sanders · 2020-10-26T15:14:32Z

We could, but is there any reason we can't make a quick decision on removing named checkpointing

Perhaps but it's a bit off topic for this PR to go removing all of the named checkpointing logic.

If named checkpoints are removed does that mean automatic checkpoints are also removed?

Any logic relating to the checkpoint tables or cylc *checkpoint commands would be up for removal. (Any use of the word "checkpoint" referring to a non-checkpoint table would stay).

hjoliver · 2020-10-26T20:03:37Z

We could, but is there any reason we can't make a quick decision on removing named checkpointing

Perhaps but it's a bit off topic for this PR to go removing all of the named checkpointing logic.

Arguable, but fair enough - my feeling was, this PR breaks checkpointing as-is, so let's wait on the checkpointing decision to know if this PR should be amended or not.

Moot point now, as @dpmatthews has agreed on removal on Element chat - I'll remove the blocked label...

Previously we were abusing the task_pool table by storing prereqs as a JSON string in a column there. Now, the prereqs are stored in the new task_prerequisites table instead.

Because we are droppping the idea of being able to restart a Cylc 7 suite in Cylc 8

If incompatible or missing db, raise meaningful exception

method for updating the table

To be fully removed - cylc#3891

oliver-sanders

👍

hjoliver

Great, thanks @MetRonnie 🎉

MetRonnie added the sod-follow-up label Oct 13, 2020

MetRonnie added this to the cylc-8.0a3 milestone Oct 13, 2020

MetRonnie requested a review from oliver-sanders October 13, 2020 11:15

MetRonnie self-assigned this Oct 13, 2020

MetRonnie commented Oct 13, 2020

View reviewed changes

cylc/flow/rundb.py Show resolved Hide resolved

oliver-sanders reviewed Oct 13, 2020

View reviewed changes

cylc/flow/unicode_rules.py Outdated Show resolved Hide resolved

MetRonnie force-pushed the prereq-db branch from a00bf8a to abba5e5 Compare October 13, 2020 13:55

oliver-sanders mentioned this pull request Oct 13, 2020

checkpoint: re-evaluate purpose of checkpointing #3864

Closed

MetRonnie marked this pull request as draft October 13, 2020 15:19

MetRonnie force-pushed the prereq-db branch from abba5e5 to 5d7b555 Compare October 13, 2020 15:49

oliver-sanders mentioned this pull request Oct 15, 2020

database refactor #3872

Open

11 tasks

MetRonnie force-pushed the prereq-db branch from 294d425 to 04661d4 Compare October 15, 2020 17:54

MetRonnie marked this pull request as ready for review October 15, 2020 17:55

MetRonnie commented Oct 15, 2020

View reviewed changes

tests/functional/restart/54-incompatible-db.t Show resolved Hide resolved

oliver-sanders reviewed Oct 15, 2020

View reviewed changes

cylc/flow/suite_db_mgr.py Outdated Show resolved Hide resolved

MetRonnie force-pushed the prereq-db branch from 04661d4 to 4202700 Compare October 16, 2020 09:58

oliver-sanders reviewed Oct 16, 2020

View reviewed changes

cylc/flow/suite_db_mgr.py Outdated Show resolved Hide resolved

MetRonnie force-pushed the prereq-db branch from 4202700 to 01e0798 Compare October 16, 2020 12:34

MetRonnie marked this pull request as draft October 16, 2020 15:18

hjoliver added the BLOCKED This can't happen until something else does label Oct 22, 2020

hjoliver removed the BLOCKED This can't happen until something else does label Oct 26, 2020

hjoliver mentioned this pull request Oct 26, 2020

Remove named checkpointing #3891

Closed

MetRonnie marked this pull request as ready for review October 27, 2020 10:06

MetRonnie added 9 commits October 28, 2020 17:13

Sort out storage of task prerequisites in db

1ffb6f8

Previously we were abusing the task_pool table by storing prereqs as a JSON string in a column there. Now, the prereqs are stored in the new task_prerequisites table instead.

Tidy

b4289f5

Write functional test for task_prerequisites db table, in restart

b2ccf5f

Fix tests

ac7021b

Remove DB upgrader functions

bdf191e

Because we are droppping the idea of being able to restart a Cylc 7 suite in Cylc 8

Update changelog

c2f3350

Check for db incompatibility on restart

9f8433c

If incompatible or missing db, raise meaningful exception

DB: task_prerequisites table: change from INSERT/UPDATE to DELETE/INSERT

7daa6b9

method for updating the table

Disable part of checkpoint test

8b1f295

To be fully removed - cylc#3891

MetRonnie force-pushed the prereq-db branch from eccaf72 to 8b1f295 Compare October 28, 2020 17:13

MetRonnie mentioned this pull request Oct 30, 2020

Remove checkpointing #3906

Merged

8 tasks

oliver-sanders approved these changes Nov 4, 2020

View reviewed changes

hjoliver approved these changes Nov 4, 2020

View reviewed changes

hjoliver merged commit 06455a2 into cylc:master Nov 4, 2020

MetRonnie deleted the prereq-db branch November 4, 2020 11:08

MetRonnie mentioned this pull request Nov 4, 2020

Integration tests: restart with faked task pool #3668

Closed

6 tasks

hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021

MetRonnie added the db change Change to the workflow database structure label Jan 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store task prerequisites in their own DB table #3863

Store task prerequisites in their own DB table #3863

MetRonnie commented Oct 13, 2020 •

edited

Loading

oliver-sanders commented Oct 13, 2020

MetRonnie commented Oct 13, 2020

oliver-sanders commented Oct 13, 2020

MetRonnie commented Oct 13, 2020

MetRonnie commented Oct 14, 2020

oliver-sanders commented Oct 14, 2020 •

edited

Loading

MetRonnie commented Oct 14, 2020

oliver-sanders commented Oct 14, 2020 •

edited

Loading

oliver-sanders left a comment

MetRonnie commented Oct 16, 2020

MetRonnie commented Oct 16, 2020

oliver-sanders commented Oct 16, 2020

oliver-sanders commented Oct 21, 2020

hjoliver commented Oct 22, 2020

oliver-sanders commented Oct 23, 2020

hjoliver commented Oct 24, 2020

MetRonnie commented Oct 26, 2020

oliver-sanders commented Oct 26, 2020 •

edited

Loading

hjoliver commented Oct 26, 2020 •

edited

Loading

oliver-sanders left a comment

hjoliver left a comment

Store task prerequisites in their own DB table #3863

Store task prerequisites in their own DB table #3863

Conversation

MetRonnie commented Oct 13, 2020 • edited Loading

oliver-sanders commented Oct 13, 2020

MetRonnie commented Oct 13, 2020

oliver-sanders commented Oct 13, 2020

MetRonnie commented Oct 13, 2020

MetRonnie commented Oct 14, 2020

oliver-sanders commented Oct 14, 2020 • edited Loading

MetRonnie commented Oct 14, 2020

oliver-sanders commented Oct 14, 2020 • edited Loading

oliver-sanders left a comment

Choose a reason for hiding this comment

MetRonnie commented Oct 16, 2020

MetRonnie commented Oct 16, 2020

oliver-sanders commented Oct 16, 2020

oliver-sanders commented Oct 21, 2020

hjoliver commented Oct 22, 2020

oliver-sanders commented Oct 23, 2020

hjoliver commented Oct 24, 2020

MetRonnie commented Oct 26, 2020

oliver-sanders commented Oct 26, 2020 • edited Loading

hjoliver commented Oct 26, 2020 • edited Loading

oliver-sanders left a comment

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

MetRonnie commented Oct 13, 2020 •

edited

Loading

oliver-sanders commented Oct 14, 2020 •

edited

Loading

oliver-sanders commented Oct 14, 2020 •

edited

Loading

oliver-sanders commented Oct 26, 2020 •

edited

Loading

hjoliver commented Oct 26, 2020 •

edited

Loading