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

suite-state command forward compat. #5238

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Nov 23, 2022

Address #5236
Sibling of #5237

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 hjoliver added this to the cylc-7.8.x milestone Nov 23, 2022
@hjoliver hjoliver self-assigned this Nov 23, 2022
@hjoliver
Copy link
Member Author

Not sure if we really need this one (it'd be nice to avoid yet another final Cylc 7 release).

#5237 is the critical one, for Cylc 8 research users triggering tasks off of Cylc 7 operational suites that haven't been migrated yet.

@dwsutherland
Copy link
Member

For operations, this is important for migration, otherwise we'll be forced to upgrade downstream first.. Which we could do I suppose...

@hjoliver hjoliver marked this pull request as ready for review October 29, 2023 23:51
@hjoliver
Copy link
Member Author

Can you give this a quick review and test @dwsutherland ?

(Since we need it soon!)

@hjoliver
Copy link
Member Author

(New tests are not needed at this point, for Cylc 7 IMO)

@MetRonnie MetRonnie self-requested a review October 30, 2023 14:12
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Missing the xtrigger part here?

fmt = checker.get_remote_point_format()

@hjoliver
Copy link
Member Author

hjoliver commented Oct 31, 2023

Good spot @MetRonnie, thanks.

Turns out I hadn't quite finished this off - another change was need to handle the change in task_outputs table format. (Well, 11 months is a long time...)

@hjoliver
Copy link
Member Author

hjoliver commented Oct 31, 2023

Testing:

Run with Cylc 8:

[scheduler]
    cycle point format = %Y
[scheduling]
    initial cycle point = 2010
    final cycle point = 2012
   [[graph]]
      P1Y = "foo"
[runtime]
   [[foo]]
      script = "cylc message x"
      [[[outputs]]]
          x = x

Now with Cylc 7, both of these work:

cylc-7 $ cylc suite-state c8/runN --task=foo --point=2011 --message=x
polling for 'output: x': satisfied

cylc-7 $ cylc suite-state c8/runN --task=foo --point=2011 --status=succeeded
polling for 'succeeded': satisfied

And to test xtriggers polling the above Cylc 8 workflow:

# suite.rc
[cylc]
  cycle point format = %Y
[scheduling]
    initial cycle point = 2010
    final cycle point = 2012
    [[xtriggers]]
         x1 = suite_state(suite=c8/runN, task=foo, point=%(point)s, message=x)
         x2 = suite_state(suite=c8/runN, task=foo, point=%(point)s, status=succeeded)
   [[dependencies]]
        [[[P1Y]]]
           graph = """
                @x1=> foo
                @x2 => foo
           """
[runtime]
    [[foo]]

@hjoliver
Copy link
Member Author

Should be good to go now.

Comment on lines 134 to 139
# This works for Cylc 7 and Cylc 8 outputs table format:
return any(
message == value
for outputs_str, in res
for value in json.loads(outputs_str)
)
Copy link
Member

Choose a reason for hiding this comment

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

Are you saying removing the .values() makes it work for Cylc 8 DBs? Surely that breaks Cylc 7?

Copy link
Member

Choose a reason for hiding this comment

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

Or was that just a mistake?

Suggested change
# This works for Cylc 7 and Cylc 8 outputs table format:
return any(
message == value
for outputs_str, in res
for value in json.loads(outputs_str)
)
# This works for Cylc 7 and Cylc 8 outputs table format:
return any(
message == value
for outputs_str, in res
for value in json.loads(outputs_str).values()
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn it. I owe you a chocolate fish 🐟

It wasn't a mistake - I took it from master, and I tested it.
However, I tested it with our classic shortcut x = x outputs - i.e. with message and label the same 🤦

So this is a bug on master too. Patch coming...

Copy link
Member

Choose a reason for hiding this comment

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

So this is a bug on master too.

You mean, only a bug on master/8.x? (Though nearly introduced to Cylc 7 in this PR)

Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

Works for me:
image

[sutherlanddw@w-ec-admin01 ~]$ cylc --version
7.9.2
[sutherlanddw@w-ec-admin01 ~]$ cylc suite-state download/run1 --run-dir=/home/sutherlanddw/cylc-run --status=succeeded --task=ext_check_globalfields_ukmet --point=20231031T00
no such table: suite_params
[sutherlanddw@w-ec-admin01 ~]$ cylc --version
7.8.11-30-gf9537
[sutherlanddw@w-ec-admin01 ~]$ cylc suite-state download/run1 --run-dir=/home/sutherlanddw/cylc-run --status=succeeded --task=ext_check_globalfields_ukmet --point=20231031T00
polling for 'succeeded': satisfied

@hjoliver
Copy link
Member Author

@MetRonnie - now it's done!!

Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Tested

@MetRonnie MetRonnie merged commit 715d002 into cylc:7.8.x Jan 11, 2024
@hjoliver hjoliver deleted the suite-state-forward-compat branch January 11, 2024 21:17
@hjoliver hjoliver modified the milestones: cylc-7.8.x, 7.8.14 and 7.9.9 Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants