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

cylc-rose: cylc list compatibility #4293

Merged
merged 14 commits into from
Jul 15, 2021
Merged

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jul 6, 2021

These changes address #4288
These changes are twinned with cylc/cylc-rose#65

Make the following scripts work with cylc-rose:

Internal Todo List

  • Cylc list
  • Cylc graph
  • Cylc config
  • Cylc validate

Standard checklist

  • 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.py and
    conda-environment.yml.
  • Does not need tests (tests are in cylc-rose).
  • Appropriate change log entry included: update change log wxtim/cylc#30 (Can be merged before merging this PR, avoids conflict)
  • Documented changes in cylc list docstring: Auto documents using the Cylc Rose args.

How to review:

Create workflows with a rose-suite.conf in

  • ~/cylc-src/
  • ~/cylc-run/ (using cylc install)
  • some other folder somewhere else entirely

You might use jinja2 or empy in the workflow to carry out tests like these.

Try each of the four cylc CLI commands with and without the extra rose options provided by this function/

Ideally these commands and cylc install should behave in a consistent way.

@wxtim wxtim self-assigned this Jul 6, 2021
@wxtim wxtim marked this pull request as draft July 6, 2021 19:08
@wxtim wxtim changed the title Cylc rose.add list cylc-rose: cylc list compatibility Jul 8, 2021
@wxtim wxtim marked this pull request as ready for review July 9, 2021 11:58
@wxtim

This comment has been minimized.

@wxtim wxtim requested a review from datamel July 12, 2021 09:27
@wxtim wxtim requested a review from kinow July 12, 2021 13:25
@kinow
Copy link
Member

kinow commented Jul 12, 2021

File containing a list of tests which need to be run locally with cylc/cylc-rose#65 installed: These will fail on GH Actions even if this PR is OK.

fail_online.txt

I think you fixed this right? (In GH UI, we can mark a comment like this as "Outdated" and hide it using the three dots at the top right corner of the comment area)

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Code and tests look good! Installing everything (flow + rose from both new branches) to test it now.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

@wxtim I used @oliver-sanders ' example from the linked issue.

  1. Created ~/cylc-src/rose1/flow.cylc with the following content:
[scheduling]
    initial cycle point = {{ ICP }}
    [[graph]]
        R1 = foo

Then created a ~/cylc-src/rose1/rose-suite.conf (not using myopt as in his example for now, will after this simpler example works).

[template variables]

ICP = "20000101T0000Z"
  1. Installed the workflow
(venv) kinow@ranma:~/cylc-src/rose1$ cylc install
INSTALLED rose1/run1 from /home/kinow/cylc-src/rose1
  1. Verified the files were installed/copied correctly
(venv) kinow@ranma:~/cylc-src/rose1$ ls ~/cylc-run/rose1/run1/
flow.cylc  log  opt  rose-suite.conf
  1. Tried cylc validate rose1/run1
(venv) kinow@ranma:~/cylc-src/rose1$ cylc validate rose1/run1
IllegalValueError: (type=cycle point) [scheduling]initial cycle point = {{ ICP }}

Any idea what step I missed? I am trying @oliver-sanders ' example first as that was in the ticket, and looks like a simple way to test this PR. Will try more Jinja2/empy once I get this one working :)

Thanks!!!
Bruno

cylc/flow/scripts/install.py Outdated Show resolved Hide resolved
@wxtim
Copy link
Member Author

wxtim commented Jul 13, 2021

@kinow

WF not working / Logging bug?

Any idea what step I missed? I am trying @oliver-sanders ' example first as that was in the ticket, and looks like a simple way to test this PR. Will try more Jinja2/empy once I get this one working :)

You might have not explicitly stated because it seems obvious, but I don't thing you have put a #!jinja2 line in your flow.cylc: This is fine if you are using the Rose 1 legacy rose-suite.conf:[jinja2:suite.rc] section heading, but with [template variables] there is no way way Cylc-Rose can deduce the templating language.

Created an issue: cylc/cylc-rose#67. I believe that this is a logging bug.

Outdated comments

I think you fixed this right? (In GH UI, we can mark a comment like this as "Outdated" and hide it using the three dots at the top right corner of the comment area)

So done

@wxtim wxtim requested a review from kinow July 13, 2021 07:50
@kinow
Copy link
Member

kinow commented Jul 13, 2021

Ah! You are right @wxtim ! Added the shebang, and also the runtime section, now it's working fine.

#!jinja2

[scheduling]
    initial cycle point = {{ ICP }}
    [[graph]]
        R1 = foo

[runtime]
[[foo]]

(venv) kinow@ranma:~/cylc-src/rose1$ cylc install 
INSTALLED rose1/run1 from /home/kinow/cylc-src/rose1
(venv) kinow@ranma:~/cylc-src/rose1$ cylc validate rose1/run1
Valid for cylc-8.0b2.dev

Will finish the review tomorrow morning. Thanks again!! 👍

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Tested the other commands, list, config, and graph. Everything worked fine. Changed the workflow to be:

#!empy

[scheduling]
    initial cycle point = @ICP
    [[graph]]
        R1 = foo

[runtime]
[[foo]]

And confirmed I had empty installed. Everything worked fine too :-) Great job @wxtim !

p.s.: we probably have a ticket somewhere to rename rose-suite.conf to rose-workflow.conf I think?

@wxtim
Copy link
Member Author

wxtim commented Jul 13, 2021

p.s.: we probably have a ticket somewhere to rename rose-suite.conf to rose-workflow.conf I think?

No. A rose suite is a config for a Cylc workflow. No immediate plans to change the term.

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.

Thanks @wxtim
Looks great. I have read the code, checked out the branch (with its sibling), ran the tests and the manual one as detailed above. I have also locally run the functional tests which were affected. No issues found for me.

@datamel datamel merged commit 327a8fa into cylc:master Jul 15, 2021
@oliver-sanders
Copy link
Member

@wxtim, just noticed this PR was marked as "partially addresses #4288", could you add a note to #4288 covering what's left for future work.

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.

4 participants