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

Add CYLC_TASK_SHARE_CYCLE_DIR to task script. #6117

Merged
merged 5 commits into from
Jan 14, 2025

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented May 31, 2024

Closes #6098

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).
  • 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/736.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim wxtim self-assigned this May 31, 2024
@wxtim wxtim marked this pull request as draft May 31, 2024 09:16
@wxtim wxtim added the small label May 31, 2024
@wxtim wxtim added this to the 8.3.1 milestone May 31, 2024
@wxtim wxtim force-pushed the feat.share_cycle_cylce_directory branch from b683ee3 to 02e0c65 Compare May 31, 2024 09:18
@wxtim wxtim requested a review from oliver-sanders May 31, 2024 10:12
@wxtim wxtim marked this pull request as ready for review May 31, 2024 10:12
@ColemanTom
Copy link
Contributor

Just as a note. If people don't use rose task-env, nor the cycle directory, and never have, then this will be creating a folder that they now have to cleanup else risk leaving lots of empty folders on disk, using up potential inode quotas or having too many files on lustre in a single folder, causing performance degredation.

@oliver-sanders
Copy link
Member

Yes, this will create one folder per-cycle. This doesn't seem particularly concerning to me given that we are already creating one folder and four-six files per-job. As a rule of thumb count(jobs) >> count(cycles) so I had not considered that this would be flagged as a performance issue.

However, if you have an existing housekeeping arrangement for log/job/<cycle>, then this might force inclusion of share/cycle/<cycle> in that arrangement (note rose_prune can manage this, one day Cylc may be able to do this itself). WDYT?

@elliotfontaine
Copy link

If I may put my two cents in, this would be really great! I use Rose anyway for the rose-suite.conf file, but this should be Cylc functionality as @oliver-sanders said in May. Right now I have $(eval rose task-env) in my [[root]] task, which is kinda weird.

@hjoliver
Copy link
Member

hjoliver commented Nov 17, 2024

IMO this is good.

  • one additional folder per cycle is not much, compared to the per-task folder/file count as Oliver noted
  • any cycling workflow is very likely to need to a per-cycle dir like this
    • (and for non-cycling workflows this will only create one folder)
  • housekeeping is pretty easy if needed

@oliver-sanders oliver-sanders modified the milestones: 8.4.0, 8.5.0 Nov 29, 2024
@wxtim wxtim requested a review from MetRonnie January 9, 2025 13:48
@MetRonnie
Copy link
Member

Kicking tests as they last ran 8 months ago

@MetRonnie MetRonnie closed this Jan 9, 2025
@MetRonnie MetRonnie reopened this Jan 9, 2025
@MetRonnie
Copy link
Member

Question on the name of the environment variable:

  • CYLC_SHARE_CYCLE_DIR (✔️ short)
  • CYLC_WORKFLOW_SHARE_CYCLE_DIR (✔️ consistent with other env vars)

@MetRonnie
Copy link
Member

This new env var should be added to https://github.com/cylc/cylc-doc/blob/master/src/reference/job-script-vars/var-list.txt

@wxtim
Copy link
Member Author

wxtim commented Jan 10, 2025

This new env var should be added to https://github.com/cylc/cylc-doc/blob/master/src/reference/job-script-vars/var-list.txt

Cylc doc PR already exists, and is linked in the heading - though I didn't change the XXX in the link label, so you might have missed it. (Also PR had the wrong variable name, now fixed)

CYLC_WORKFLOW_SHARE_CYCLE_DIR (✔️ consistent with other env vars)

To my mind that would imply a variable constant across all cycles and tasks in the workflow, which this isn't!

@MetRonnie
Copy link
Member

Yes you're right, CYLC_TASK_SHARE_CYCLE_DIR would be consistent with other vars

@wxtim
Copy link
Member Author

wxtim commented Jan 10, 2025

Yes you're right, CYLC_TASK_SHARE_CYCLE_DIR would be consistent with other vars

No, because that would imply it's unique on a per task basis.

@MetRonnie
Copy link
Member

There are other CYLC_TASK_ vars which apply on a per-task basis without being unique to tasks. Case in point: CYLC_TASK_CYCLE_POINT

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 unless there is a consensus to call it CYLC_TASK_SHARE_CYCLE_DIR

@oliver-sanders
Copy link
Member

Happy unless there is a consensus to call it CYLC_TASK_SHARE_CYCLE_DIR

Yes, I think we should follow the naming pattern of other env vars, clunky as it may be.

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.

LGTM

cylc/flow/etc/job.sh Outdated Show resolved Hide resolved
tests/functional/jobscript/02-share-cycle-cycle/flow.cylc Outdated Show resolved Hide resolved
@MetRonnie MetRonnie changed the title Add CYLC_SHARE_CYCLE_DIR to task script. Add CYLC_TASK_SHARE_CYCLE_DIR to task script. Jan 13, 2025
changes.d/6117.feat.md Outdated Show resolved Hide resolved
Co-authored-by: Oliver Sanders <[email protected]>
@wxtim wxtim requested a review from oliver-sanders January 13, 2025 12:58
changes.d/6117.feat.md Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from oliver-sanders January 13, 2025 14:57
@oliver-sanders oliver-sanders merged commit 690e610 into cylc:master Jan 14, 2025
27 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.

share/cycle/<cycle> directory
6 participants