Skip to content

Allow platform setting to have prefix/suffix around subshell#6848

Merged
MetRonnie merged 1 commit intocylc:8.5.xfrom
MetRonnie:platform-subshell
Aug 26, 2025
Merged

Allow platform setting to have prefix/suffix around subshell#6848
MetRonnie merged 1 commit intocylc:8.5.xfrom
MetRonnie:platform-subshell

Conversation

@MetRonnie
Copy link
Copy Markdown
Member

@MetRonnie MetRonnie commented Jul 11, 2025

This so that we can do e.g.

platform = $(rose config platform hpc_live)-bg

Currently we have to resort to

platform = $(echo "$(rose config platform hpc_live)-bg")

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).
  • No dependency changes
  • Tests are included
  • Changelog entry included if this is a change that can affect users
  • No docs PR applicable
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@MetRonnie MetRonnie added this to the 8.6.0 milestone Jul 11, 2025
@MetRonnie MetRonnie self-assigned this Jul 11, 2025
@oliver-sanders
Copy link
Copy Markdown
Member

The change makes sense, however, platform subshell is really Cylc 7 hangover, [platforms] were intended to mitigate the need for this sort of thing.

It looks like this approach is using Rose to configure Cylc with the Rose config edited when hpc_live updates?

Could this be done the other way around, editing the Cylc config instead?

# global.cylc

[platform groups]
  [[hpc-live]]
    platforms = aaa
  [[hpc-live-bg]]
    platforms = aaa-bg

Or even pull in the value from an external source of truth:

#!Jinja2

{% import "os" as os %}

[platform groups]
  [[hpc-live]]
    platforms = {{ os.fdopen("hpc_live_file").read() }}
  [[hpc-live-bg]]
    platforms = {{ os.fdopen("hpc_live_file").read() }}-bg

@MetRonnie
Copy link
Copy Markdown
Member Author

It looks like this approach is using Rose to configure Cylc with the Rose config edited when hpc_live updates?

Only using Rose for the simple parsing of the config file (~/.metomi/rose.conf) to pick out the hpc_live setting. Could use something like jq with a JSON config file, but jq is not generally installed. The config file itself has a few other settings used for switching HPCs, e.g.

[platform]
hpc_live = zone1
hpc_mirror = zone2
hpc_switch = zone3

Could this be done the other way around, editing the Cylc config instead?

This would not pick up changes to hpc_live without a reload, which would rather be avoided for an operational suite of dozens of workflows.

Brainstorming here, would it be worth implementing dynamically updatable platforms in some other way, as an 8.6.0 feature?

@MetRonnie MetRonnie modified the milestones: 8.6.0, 8.5.x Jul 16, 2025
@MetRonnie MetRonnie added the could be better Not exactly a bug, but not ideal. label Jul 16, 2025
@MetRonnie
Copy link
Copy Markdown
Member Author

Tentatively putting this against 8.5.x as it's equally a fix as it is a feature?

@oliver-sanders
Copy link
Copy Markdown
Member

oliver-sanders commented Jul 16, 2025

Brainstorming here, would it be worth implementing dynamically updatable platforms in some other way, as an 8.6.0 feature?

I think we need a better understanding of the use case and a quick think.

I suspect that the only reason we are defining the platform at all is to ensure that it is defined when the workflow is started up. Beyond this broadcasts take over? If so, then this doesn't need to be dynamic at all.


This would not pick up changes to hpc_live without a reload, which would rather be avoided for an operational suite of dozens of workflows.

Yes, we would need to run a command to do this:

$ cylc reload -g '*'

I thought we're using broadcasts for this anyway?


Lets discuss when we've gathered a bit more insight.

@MetRonnie MetRonnie changed the base branch from master to 8.5.x July 29, 2025 11:48
@MetRonnie MetRonnie marked this pull request as ready for review August 18, 2025 16:05
Copy link
Copy Markdown
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.

LGTM.

Maybe consider adding an example that uses a prefix/suffix into the config docs.

There is one, very minor flaw in that the regex will match something like $(foo)-$(bar) and attempt to run it leaving a cryptic error. But doesn't really matter.

# Regex to check whether a string is a command
HOST_REC_COMMAND = re.compile(r'(`|\$\()\s*(.*)\s*([`)])$')
PLATFORM_REC_COMMAND = re.compile(r'(\$\()\s*(.*)\s*([)])$')
PLATFORM_REC_COMMAND = re.compile(r'(\$\()\s*(.*)\s*(\))')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[minor] can prevent this first group from being captured using ?::

Suggested change
PLATFORM_REC_COMMAND = re.compile(r'(\$\()\s*(.*)\s*(\))')
PLATFORM_REC_COMMAND = re.compile(r'(?:\$\()\s*(.*)\s*(\))')

@oliver-sanders oliver-sanders requested a review from wxtim August 19, 2025 10:27
@wxtim
Copy link
Copy Markdown
Member

wxtim commented Aug 26, 2025

Seems fine. Will approve once I've seen matching Docs PR.

@MetRonnie
Copy link
Copy Markdown
Member Author

Added 1 line of documentation in this PR

@wxtim
Copy link
Copy Markdown
Member

wxtim commented Aug 26, 2025

Feel free to merge after tests pass

@MetRonnie MetRonnie merged commit 03b2dde into cylc:8.5.x Aug 26, 2025
28 checks passed
@MetRonnie MetRonnie deleted the platform-subshell branch August 26, 2025 15:41
@oliver-sanders oliver-sanders modified the milestones: 8.5.x, 8.5.2 Sep 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

could be better Not exactly a bug, but not ideal. small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants