Conversation
Contributor
|
You can preview documentation at https://esmci.github.io/cime/branch/fix/flux-walltime-minutes/html/index.html |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a Flux-specific walltime rendering path so CIME emits a Flux-compatible --time-limit value (minutes or Flux Standard Duration) without changing the existing format_time() / positional walltime_format behavior for other batch systems. Also updates documentation and adds unit tests to validate conversion behavior and error handling.
Changes:
- Add
EnvBatch._flux_walltime()and use it fromEnvBatch.set_job_defaults()whenbatch_systemisflux. - Document Flux-specific
walltime_formatsemantics and accepted formats. - Add unit tests for Flux walltime conversion and the
set_job_defaults()Flux branch.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| doc/source/ccs/model-configuration/variables/batch.rst | Documents walltime_format, including Flux-specific total-duration semantics and accepted formats. |
| CIME/XML/env_batch.py | Implements Flux-only walltime conversion/validation during job default setup. |
| CIME/tests/test_unit_xml_env_batch.py | Adds pytest coverage for _flux_walltime() and Flux behavior in set_job_defaults(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Author
|
Needs testing on actual machine still. |
Flux's -t/--time-limit accepts minutes or Flux Standard Duration (RFC 23), not HH:MM:SS. On the flux path %H/%M/%S in walltime_format expand to the total duration in that unit so a matching FSD suffix yields a valid value (e.g. %Mm on 01:10:00 gives 70m). Formats that would produce values flux rejects or misreads are rejected at setup. Closes #5017
jasonb5
force-pushed
the
fix/flux-walltime-minutes
branch
2 times, most recently
from
August 25, 2026 19:57
54907b5 to
51150d2
Compare
jasonb5
force-pushed
the
fix/flux-walltime-minutes
branch
from
September 15, 2026 19:42
51150d2 to
bc2c4b0
Compare
The cmake install step places cprnc at builds/cprnc/bin/cprnc (cwd is
builds/cprnc when CMAKE_INSTALL_PREFIX=${PWD} is set), but the
comparison step invoked ./builds/bin/cprnc, one directory level off.
This mismatch was introduced in d6a3da2 when the build switched from
a flat make build to a cmake install.
download_input_data() only checked -f $dest before skipping a download, and wget wrote directly to the final path. Any interruption (container kill, CI timeout, network drop) could leave a zero-byte or truncated file at the destination, which every subsequent run would then treat as already present forever, silently feeding corrupt input data (e.g. the oQU240 mapping file) to consumers like gen_domain and causing it to abort. Now each download goes to a per-attempt temp file and is only renamed into place after wget succeeds and the result is non-empty, so a partial download can never land at the final path. Existing zero-byte or otherwise-empty cached files are detected and removed so they get retried instead of being treated as valid forever. --continue is dropped since it no longer applies once every attempt starts from a fresh temp file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
EnvBatch.set_job_defaults(): Flux's-t/--time-limitaccepts minutes or Flux Standard Duration (FSD, RFC 23), notHH:MM:SS%H/%M/%S(/%D) inwalltime_formatexpand to the total duration in that unit, so a matching FSD suffix yields a valid value (e.g.%Mmon01:10:00→70m,%Hh→1.17h); no format defaults to FSD minutes (70m)%M,%Ss,%Mm,%Hh,%Dd); anything else (e.g.%H— a bare number flux would misread as minutes — or%H:%M:%S) fails loudly at case setup instead of submitting a wrong time limitformat_time()/walltime_formatpath for all other batch systems is untouched<walltime_format>%Mm</walltime_format>on Tuolumne) works unchangedValidation
_flux_walltimecovering all accepted formats, rounding edge cases, and rejected formats, plus mock-basedset_job_defaultstests for the flux branch (01:10:00→70m/70/1.17h,00:10:30→11m)test_unit_xml_env_batch.pytests pass, confirming non-flux behavior is unchangedCloses #5017