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 Validation using old tvars fails for Cylc 7 workflows #5313

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jan 19, 2023

Allow parsec to fail gracefully when trying to retrieve template variables from a workflow database if it's a Cylc 7 DB, by doing the same DB compatibility check that's done on restart.

Fixes two small issues not recorded elsewhere

  • I had failed to use the is_public kwarg on CylcWorkflowDAO.__init__ , without which the function tries to write to the database, which we don't want to do for the parsec.fileparse use of database access. (Now sorted out by DB interface & safety improvements #5330)
  • Pre Cylc 8 workflows don't have the database field under the same name - have made cylc play and cylc vr fail gracefully when trying to run on such workflows.

Also closes #5317

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.

@wxtim wxtim self-assigned this Jan 19, 2023
@wxtim wxtim added this to the 8.1.1 milestone Jan 19, 2023
@wxtim wxtim force-pushed the fix_get_tvars_from_db_breaks_with_Cylc_7_validation branch from 3980928 to 3ce6562 Compare January 19, 2023 16:55
@oliver-sanders
Copy link
Member

oliver-sanders commented Jan 19, 2023

The check which prevents Cylc 8 from restarting Cylc 7 workflows is this:

Traceback (most recent call last):
  ...
  File "~/cylc-flow/cylc/flow/workflow_db_mgr.py", line 799, in check_workflow_db_compatibility
cylc.flow.exceptions.ServiceFileError: Workflow database is incompatible with Cylc 8.2.0.dev, or is corrupted.

So I think all that's needed is to add the check_workflow_db_compatibility check at the start of cylc vr.

BUT

Only do this if the workflow is NOT running (because if it is running, we know it's Cylc 8 compatible). Note it doesn't matter that check_workflow_db_compatibility uses the private database provided we know the workflow is not running.

@wxtim wxtim requested a review from oliver-sanders January 19, 2023 20:00
@MetRonnie
Copy link
Member

MetRonnie commented Jan 20, 2023

if not self.is_public:
self.create_tables()

IMO whether to create the tables or not should be handled by a different, new arg, called create_tables instead of is_public, because that really isn't clear otherwise

@MetRonnie MetRonnie added the bug Something is wrong :( label Jan 20, 2023
Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

I have checked out this branch. I have run workflows under cylc7 and then tried validate with this branch and master.
I get a nice error on this branch when the workflow has run and validating at cylc 8, master will state the workflow is valid at cylc 8.
If I validate an actively running (with cylc 7) workflow on this branch, it still pass validation. This is probably a niche case? EDIT: I see Oliver has this covered and it it ok in this case.
When I play an actively running cylc 7 workflow, I get traceback with KeyError: 'CYLC_WORKFLOW_PID' at cylc8, again probs niche and maybe a separate issue?

cylc/flow/workflow_files.py Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from datamel January 20, 2023 11:05
cylc/flow/templatevars.py Outdated Show resolved Hide resolved
@wxtim
Copy link
Member Author

wxtim commented Jan 20, 2023

IMO whether to create the tables or not should be handled by a different, new arg, called create_tables instead of is_public, because that really isn't clear otherwise

It might well have saved me from this error, but unless you insist I think it's out of scope.

@wxtim wxtim requested a review from datamel January 20, 2023 11:07
cylc/flow/workflow_files.py Outdated Show resolved Hide resolved
@oliver-sanders
Copy link
Member

Could you rebase onto 8.1.x please.

@wxtim wxtim force-pushed the fix_get_tvars_from_db_breaks_with_Cylc_7_validation branch 2 times, most recently from 800dc9a to 9f5af69 Compare January 24, 2023 09:42
@wxtim wxtim requested a review from oliver-sanders January 24, 2023 09:42
@oliver-sanders oliver-sanders changed the base branch from master to 8.1.x January 24, 2023 09:55
@wxtim wxtim force-pushed the fix_get_tvars_from_db_breaks_with_Cylc_7_validation branch from 9f5af69 to 79ee9c3 Compare January 24, 2023 09:59
@oliver-sanders
Copy link
Member

oliver-sanders commented Jan 24, 2023

Ran through the methodology from here - #5313 (comment)

2023-01-24T10:10:23Z CRITICAL - Cannot tell if the workflow is running
    Note, Cylc 8 cannot restart Cylc 7 workflows.
Traceback (most recent call last):
  File "/home/h06/osanders/mambaforge/envs/cylc.dev/bin/cylc", line 33, in <module>
    sys.exit(load_entry_point('cylc-flow', 'console_scripts', 'cylc')())
  File "/net/home/h06/osanders/cylc-flow/cylc/flow/scripts/cylc.py", line 653, in main
    execute_cmd(command, *cmd_args)
  File "/net/home/h06/osanders/cylc-flow/cylc/flow/scripts/cylc.py", line 286, in execute_cmd
    entry_point.resolve()(*args)
  File "/net/home/h06/osanders/cylc-flow/cylc/flow/terminal.py", line 232, in wrapper
    wrapped_function(*wrapped_args, **wrapped_kwargs)
  File "/net/home/h06/osanders/cylc-flow/cylc/flow/scripts/validate_reinstall.py", line 120, in main
    sys.exit(vro_cli(parser, options, workflow_id))
  File "/net/home/h06/osanders/cylc-flow/cylc/flow/scripts/validate_reinstall.py", line 152, in vro_cli
    workflow_running, options.templatevars, options.templatevars_file
UnboundLocalError: local variable 'workflow_running' referenced before assignment

Need to halt the execution either by raising the original exception as suggested, or by return 1 or sys.exit(1) or whatnot. So long as the commands exit code is non-zero.

@wxtim wxtim force-pushed the fix_get_tvars_from_db_breaks_with_Cylc_7_validation branch from 230c5f4 to 2a3a93f Compare January 27, 2023 13:03
cylc/flow/scripts/report_timings.py Outdated Show resolved Hide resolved
tests/unit/test_templatevars.py Outdated Show resolved Hide resolved
@wxtim wxtim force-pushed the fix_get_tvars_from_db_breaks_with_Cylc_7_validation branch from 6e55161 to 30d2e6d Compare January 27, 2023 13:40
@wxtim wxtim force-pushed the fix_get_tvars_from_db_breaks_with_Cylc_7_validation branch from 51810cf to 30d2e6d Compare January 27, 2023 13:52
@oliver-sanders oliver-sanders linked an issue Jan 27, 2023 that may be closed by this pull request
@MetRonnie
Copy link
Member

Added some tests in wxtim#52

@wxtim wxtim requested a review from MetRonnie January 30, 2023 11:13
@wxtim wxtim force-pushed the fix_get_tvars_from_db_breaks_with_Cylc_7_validation branch from 050d352 to c0d7b1a Compare January 30, 2023 15:32
@oliver-sanders
Copy link
Member

Re-reviewed code, LGTM, moving onto functional testing...

@MetRonnie MetRonnie marked this pull request as draft January 30, 2023 17:36
@MetRonnie
Copy link
Member

MetRonnie commented Jan 30, 2023

Regression in latest rebase. Would not recommend rebasing at this point due to how out of date the earlier commits are, just whack squash 'n merge at the end

Update: I have force pushed back to f26b78c

@MetRonnie MetRonnie force-pushed the fix_get_tvars_from_db_breaks_with_Cylc_7_validation branch from 3641358 to f26b78c Compare January 30, 2023 18:07
@MetRonnie MetRonnie marked this pull request as ready for review January 30, 2023 18:07
@MetRonnie
Copy link
Member

Have tested for both a running and stopped Cylc 7 workflow:

  • cylc play
  • cylc validate
  • cylc vr (Produces traceback for a running Cylc 7 workflow but the correct error message is now printed at the top so who cares)
  • cylc vip

And ensured no tables are created by these commands

Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

Tested as working. Thanks @wxtim

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.

Raise Error when playing a running Cylc 7 workflow
4 participants