Skip to content

cat-log: infer --force-remote for --mode=list-dir if job.out not present#6933

Merged
oliver-sanders merged 14 commits intocylc:masterfrom
ChrisPaulBennett:6596-force-remote
Oct 23, 2025
Merged

cat-log: infer --force-remote for --mode=list-dir if job.out not present#6933
oliver-sanders merged 14 commits intocylc:masterfrom
ChrisPaulBennett:6596-force-remote

Conversation

@ChrisPaulBennett
Copy link
Copy Markdown
Contributor

@ChrisPaulBennett ChrisPaulBennett commented Aug 20, 2025

This PR closes #6596.
There was an issue where if cat-log is ran in a small window of time between when a task finishes, but before the logs are copied locally, cat-log could display logs incorrectly. This change looks to see if the job.out log is present locally and if not, it defaults to remote retrieval.

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 if present).
  • Tests are included (or explain why tests are not needed).
  • Changelog 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.

@oliver-sanders oliver-sanders changed the title 6596 force remote retrieval cat-log: infer --force-remote for --mode=list-dir if job.out not present Aug 20, 2025
@oliver-sanders
Copy link
Copy Markdown
Member

This PR addresses the issue #6596.

FYI, if you change this message to closes #6596, then then GitHub magic will automatically link this PR to the issue and close it once the PR is merged.

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

ChrisPaulBennett and others added 3 commits August 20, 2025 14:57
Fix small logical flaw

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
@ChrisPaulBennett
Copy link
Copy Markdown
Contributor Author

Unless I've missed something, this code also closes #6597. Double check this though please.
I've also noticed that --mode=list-dir argument was missing from the test, so this has been added

@oliver-sanders
Copy link
Copy Markdown
Member

oliver-sanders commented Aug 27, 2025

FYI: The two issues are similar, but subtly different:

@oliver-sanders
Copy link
Copy Markdown
Member

I'm not sure this is actually going to the remote platform?

E.g, if I apply this diff:

diff --git a/cylc/flow/scripts/cat_log.py b/cylc/flow/scripts/cat_log.py
index e2ca4ad32..4af5e8536 100755
--- a/cylc/flow/scripts/cat_log.py
+++ b/cylc/flow/scripts/cat_log.py
@@ -611,6 +611,7 @@ def _main(
                 # (Ctrl-C while tailing)
                 # NOTE: This will raise NoHostsError if the platform is not
                 # contactable
+                LOG.critical("Goin' remote")
                 proc = remote_cylc_cmd(
                     cmd,
                     platform,

It says it is going to the remote platform, but it doesn't log the newly added message, suggesting that it doesn't actually go remote?

$ cylc cat remote//1/remote -f e --debug
DEBUG - Loading site/user config files
DEBUG - Reading file /home/users/metomi/apps/metomi-site/etc/cylc/flow/8/global.cylc
DEBUG - Processing with Jinja2
DEBUG - Setting Jinja2 template variables:
    + CYLC_TEMPLATE_VARS={'CYLC_VERSION': '8.6.0.dev', 'CYLC_TEMPLATE_VARS': {...}}
    + CYLC_VERSION=8.6.0.dev
DEBUG - Reading file /home/users/oliver.sanders/.cylc/flow/global.cylc
DEBUG - Processing with Jinja2
DEBUG - Setting Jinja2 template variables:
    + CYLC_TEMPLATE_VARS={'CYLC_VERSION': '8.6.0.dev', 'CYLC_TEMPLATE_VARS': {...}}
    + CYLC_VERSION=8.6.0.dev
DEBUG - Reading file /home/users/oliver.sanders/.cylc/flow/8/global.cylc
DEBUG - job.out not present, getting job log remotely
File not found: /home/users/oliver.sanders/cylc-run/remote/run1/log/job/1/remote/NN/job.err

@ChrisPaulBennett
Copy link
Copy Markdown
Contributor Author

ChrisPaulBennett commented Aug 28, 2025

I'm not sure this is actually going to the remote platform?

E.g, if I apply this diff:

diff --git a/cylc/flow/scripts/cat_log.py b/cylc/flow/scripts/cat_log.py
index e2ca4ad32..4af5e8536 100755
--- a/cylc/flow/scripts/cat_log.py
+++ b/cylc/flow/scripts/cat_log.py
@@ -611,6 +611,7 @@ def _main(
                 # (Ctrl-C while tailing)
                 # NOTE: This will raise NoHostsError if the platform is not
                 # contactable
+                LOG.critical("Goin' remote")
                 proc = remote_cylc_cmd(
                     cmd,
                     platform,

It says it is going to the remote platform, but it doesn't log the newly added message, suggesting that it doesn't actually go remote?

$ cylc cat remote//1/remote -f e --debug
DEBUG - Loading site/user config files
DEBUG - Reading file /home/users/metomi/apps/metomi-site/etc/cylc/flow/8/global.cylc
DEBUG - Processing with Jinja2
DEBUG - Setting Jinja2 template variables:
    + CYLC_TEMPLATE_VARS={'CYLC_VERSION': '8.6.0.dev', 'CYLC_TEMPLATE_VARS': {...}}
    + CYLC_VERSION=8.6.0.dev
DEBUG - Reading file /home/users/oliver.sanders/.cylc/flow/global.cylc
DEBUG - Processing with Jinja2
DEBUG - Setting Jinja2 template variables:
    + CYLC_TEMPLATE_VARS={'CYLC_VERSION': '8.6.0.dev', 'CYLC_TEMPLATE_VARS': {...}}
    + CYLC_VERSION=8.6.0.dev
DEBUG - Reading file /home/users/oliver.sanders/.cylc/flow/8/global.cylc
DEBUG - job.out not present, getting job log remotely
File not found: /home/users/oliver.sanders/cylc-run/remote/run1/log/job/1/remote/NN/job.err

This is caused by this commit
a7d9fca

log_is_remote is always coming up as false and causing remote to not happen
Can you remind me what the logic flaw was?

@oliver-sanders
Copy link
Copy Markdown
Member

Can you remind me what the logic flaw was?

There's a logical flaw?

I think the remote functionality is still working on master right?

@oliver-sanders oliver-sanders modified the milestones: 8.6.0, 8.7.0 Sep 1, 2025
Copy link
Copy Markdown
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

  • Needs a changelog entry.
  • I have questions about the logic
  • I've left some style thoughts. None of them should block this PR>

@oliver-sanders oliver-sanders marked this pull request as draft September 19, 2025 10:51
@oliver-sanders
Copy link
Copy Markdown
Member

Have converted to "draft" to assist with review prioritisation whilst Tim's comment above is addressed.

ChrisPaulBennett and others added 3 commits October 6, 2025 09:30
Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
@MetRonnie MetRonnie removed their request for review October 14, 2025 11:38
Copy link
Copy Markdown
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

You should undraft.

@ChrisPaulBennett ChrisPaulBennett marked this pull request as ready for review October 15, 2025 10:27
@oliver-sanders oliver-sanders merged commit 41df1eb into cylc:master Oct 23, 2025
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cat-log: infer --force-remote for --mode=list-dir if job.out not present

5 participants