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

Fcm make fix for remote and subshell platforms #120

Merged
merged 8 commits into from
Mar 21, 2022

Conversation

datamel
Copy link
Contributor

@datamel datamel commented Mar 16, 2022

Sibling to: metomi/rose#2557

Again credit to @wxtim for pair programming this.
These changes close metomi/rose#2553

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Applied any dependency changes to setup.cfg - None to apply.
  • Appropriate tests are included (unit and/or functional).
  • No change log entry required (why? invisible to users).
  • No documentation update required.

@wxtim wxtim mentioned this pull request Mar 17, 2022
28 tasks
def get_platform_from_task_def(
flow: str, task: str
) -> Dict[str, Any]:
def get_platform_from_task_def(flow: str, task: str) -> Dict[str, Any]:
"""Return the platform dictionary for a particular task.

Uses the flow definition - designed to be used with tasks
Copy link
Member

Choose a reason for hiding this comment

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

I think the docstring could be updated to mention that it evaluates any subshell defined platform/hosts

cylc/rose/platform_utils.py Outdated Show resolved Hide resolved
cylc/rose/platform_utils.py Outdated Show resolved Hide resolved
cylc/rose/platform_utils.py Outdated Show resolved Hide resolved
from cylc.flow.id_cli import parse_id
from cylc.flow.platforms import (
HOST_REC_COMMAND,
Copy link
Member

Choose a reason for hiding this comment

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

This allows backticks, was that desired?

Copy link
Member

Choose a reason for hiding this comment

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

This is only for the old [remote]host syntax which I think we supported at Cylc 7.

datamel and others added 3 commits March 18, 2022 15:18
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Looks good, going to try to get the test passing locally.

cylc/rose/platform_utils.py Outdated Show resolved Hide resolved
cylc/rose/platform_utils.py Outdated Show resolved Hide resolved
cylc/rose/platform_utils.py Show resolved Hide resolved
datamel and others added 3 commits March 21, 2022 09:11
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
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.

Happy, although I haven't tested. Is there an example workflow/Rose suite I can test manually? (The subshell bit, irrespective of FCM Make?)

@wxtim
Copy link
Member

wxtim commented Mar 21, 2022

Happy, although I haven't tested. Is there an example workflow/Rose suite I can test manually? (The subshell bit, irrespective of FCM Make?)

t/rose-task-run/08-app-fcm-make/

From a quick glance add the following to the rose-suite.conf

[template variables]
HOST="name your remote host"

You will also need an fcm wrapper in remoteHost:~/bin - have a look in my or Mel's bin dir on the remote test host.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Ran against the tests in metomi-rose, all good.

@oliver-sanders oliver-sanders merged commit ec02168 into cylc:master Mar 21, 2022
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.

fcm_make: app fails when run remotely
4 participants