adds start/stop options and fixes restart issues - #524
Conversation
|
Passes Ctests on Chrysalis. I also tested running 5 days and restarting for another 5 days and verified that the append option works to correctly write daily time slices to a single file (to verify fixing #482) |
|
omega-buildnml PR tests are failing due to changes in the config file needed for this PR. |
|
I will be mostly out of contact for about two weeks starting 8/25. I might be able to briefly respond to any issues and point to fixes but will not be able to access/modify code during that time. |
|
@philipwjones, just a quick note to say that I have seen this work is up and I really appreciate it! I'll get to reviewing it as soon as I can. |
xylar
left a comment
xylar
left a comment
There was a problem hiding this comment.
Thanks for this — the restart clock handling is a real improvement, and I think the approach to #482 is correct. I found three bugs that keep the coupled path from working at all, plus a CI failure. All four are small, and the fix for each is described inline below. The rest is one downstream note about Polaris and a couple of small things.
Bugs
getTimeStepperStartTypeFromE3SM has no break statements
In TimeStepper.cpp, every case falls through to default:, so the function aborts with Invalid E3SM start type value no matter what the coupler passes. Every coupled run dies in ocnInit1.
The same function's signature doesn't match its declaration
TimeStepper.h declares getTimeStepperStartTypeFromE3SM(const int &E3SMOpt) while TimeStepper.cpp defines getTimeStepperStartTypeFromE3SM(const int E3SMOption). Top-level const is stripped from a by-value parameter, so these are two distinct overloads: the one omega_cxx2f_interface.cpp calls is declared but never defined, and the coupled build fails to link. This is also why the missing breaks got through — the standalone CTests don't link the coupled driver, so neither problem is reachable from ctest.
getDuration() is declared but never defined
TimeStepper.h declares TimeInterval getDuration() const; and there is no definition. Nothing calls it yet, so it's latent, but the first caller would fail to link.
CI
The omega-buildnml job fails because the TimeIntegration options were renamed but the buildnml side still refers to the old ones:
ValueError: Invalid Omega configuration:
- Unknown option(s) TimeIntegration.StopTime, TimeIntegration.RunDuration in coupled overrides.
config_overrides.yaml still sets StartTime, StopTime and RunDuration under coupled, and validate.py's BLOCKED_OPTIONS still names StopTime and RunDuration. Worth noting that coupled runs also need StopType: OnSignal — this isn't cosmetic. Without it the merged config falls through to Default.yml's StopType: AtTime with StopCriterion: 0001-01-01_02:00:00, which is not what a coupled run wants. Since blocked options can't appear in config_overrides.yaml, that setting belongs in build_runtime_overrides alongside CalendarType.
#482 looks genuinely fixed
Recording the reasoning since it took me a while to convince myself. The history time axis is SimTime - ModelClock->getStartTime() in IOStream.cpp, and Clock::setCurrentTime moves only CurrTime/PrevTime/NextTime, leaving the clock's StartTime at the full-simulation start. On the coupled side ocn_comp_mct.F90 passes case_start_ymd, which is constant across restart segments rather than the segment start. So elapsed time is monotonic across segments and the frame-matching loop appends instead of overwriting. That's the right fix, and separating StartType from the stream-level UseStartEnd hack makes it much harder to get wrong.
Downstream: this breaks Polaris in three ways
Flagging this because it needs a paired Polaris PR rather than anything in this one, polaris#731, whose omega_pr suite is green against this PR. So this is a description of what had to change rather than an open problem. I list all three because I only spotted the first by reading, and the other two turned up when the suite actually ran.
Through the option map. polaris/ocean/model/mpaso_to_omega.yaml maps config_stop_time to StopTime and config_run_duration to RunDuration, both of which this PR deletes, and every ocean forward task in Polaris sets config_run_duration. This is not a rename either: config_run_duration now has to produce two keys, StopType: AfterDuration plus StopCriterion, which a one-to-one option map cannot express, so it needs a change in the translation code rather than in the yaml.
Around the option map. A task can also write Omega's options straight into an Omega: block, where the map never sees them. horiz_press_grad sets StopTime that way, and setup fails with Attempting to modify a nonexistent config options: TimeIntegration: StopTime. Nothing warns about this class until the one task that does it happens to be set up.
The UseStartEnd sentinel. Both existing Polaris restart tasks suppressed the restart read by setting UseStartEnd: true with a 99999-12-31 start time the run never reaches, and switched InitialState to FreqUnits: never for the restarting run. Both halves break here. Dropping StartTime and EndTime from RestartRead in Default.yml means UseStartEnd: true now inherits no end time, so cosine_bell/restart aborts with Stream RestartRead requests UseStartEnd but no end time provided. And because ocnInit now follows StartType strictly instead of trying both streams and accepting whichever succeeds, baroclinic_channel/restart aborts with Stream InitialState not found after its own never dropped the stream. StartType is the right replacement for that sentinel and is much clearer, but it is a required migration, not an optional cleanup.
That last one is worth a line in the PR description, since anyone else carrying Omega restart configuration outside Polaris will hit exactly the same two aborts.
Minor
OnSignal builds an EndAlarm but never attaches it to the StepClock, unlike the AtTime and AfterDuration cases. That's harmless in coupled mode, where ocnRun loops on the coupling alarm, but standalone ocnRun loops on EndAlarm->isRinging(), so a standalone run configured with StopType: OnSignal would never terminate. Might be worth rejecting OnSignal in the standalone init1() path, or at least saying in the config comment that it's coupled-only.
Alarm::reset walking a periodic alarm backwards is a nice generalisation, and RingTimePrev is initialised in the periodic constructor so the backward walk has a valid starting point. No issue, just noting I checked it since Clock::setCurrentTime now depends on it for every periodic alarm.
Testing
All on chrysalis with intel/openmpi. C++ linting used the omega_dev conda environment (clang-format 18.1.8); Omega itself was built by Polaris from this branch.
The CI failure, reproduced and confirmed fixed. cime_config/validate_config.py fails on this branch with Unknown option(s) TimeIntegration.StopTime, TimeIntegration.RunDuration in coupled overrides, and passes once the buildnml side is updated as described above. The 133 omega_buildnml unit tests pass. One environment note while I was there: omega_dev has no pytest even though dev-conda.txt lists it, so the environment looks stale relative to that file; I borrowed pytest from another environment to run them.
Standalone build. Omega builds clean from this branch via polaris setup --build --branch, and pre-commit (clang-format included) is clean on every file I touched.
Issue #482, tested directly. I added a realistic_global restart task to Polaris that runs QU.240km for two hours three ways: once straight through, and once as two one-hour segments where the second continues from the first's restart and appends to its history. It compares the frame count and time axis as well as the state, since #482 produced a well-formed file that was simply missing its first half. On this branch the chain writes all four frames at 1800, 3600, 5400 and 7200 s, matching the uninterrupted run, and the state matches too. The whole task runs in 46 s.
The negative control matters more than the pass. To check the test can actually fail, I re-ran the continuing segment with StartTime set to the segment start rather than the simulation start, which is what the pre-#524 restart pattern amounted to. Omega then rewrote the elapsed-time axis from zero, the continuation's frames landed back on 1800 and 3600, and they overwrote the two frames already in the file, leaving two instead of four. That is #482 reproduced exactly, and the validation step fails on it. So the test has teeth and this branch is what makes it pass.
Not tested: the coupled path. The two fixes above are in code the standalone build does not link, so nothing I ran exercises them. Confirming them needs a coupled E3SM case, which is worth doing before this merges, since the missing breaks mean every coupled run aborts in ocnInit1.
Suite comparison. Polaris omega_pr run twice, a baseline on Polaris main with the pre-#524 Omega, and then polaris#731 against this PR. The three fixes above were applied locally to build and test with, but none of them can affect this result: the buildnml one touches only CIME-generated config, which Polaris does not use, and the other two are in code the standalone build never calls. The comparison therefore stands for this PR as it is. The baseline passes 22 tasks; the branch passes 23, with all 108 baseline comparisons clean and no differences. So this PR is answer-preserving for standalone runs once Polaris speaks the new config. The three breakages described above are what had to be fixed to get there; the two restart-task aborts in particular only surfaced by running the suite, not by reading the diff.
d6e1abb to
51765cf
Compare
andrewdnolan
left a comment
andrewdnolan
left a comment
There was a problem hiding this comment.
@philipwjones Sorry about the delay!
Overall this looks great. Very sensible improvements to the start/stop logic. I have a few substantive comments/question, most of them are just cosmetic.
If you could resolve conflicts in components/omega/src/ocn/OceanDriver.h that seem to have snuck in I can start testing the coupled mode paths/.
|
@andrewdnolan Thanks, but you picked a bad time to review - I committed some fixes to the issues Xylar found and did a rebase earlier today and the rebase broke some things which I hadn't checked before I pushed the changes. I should have it fixed tomorrow and I'll respond to your comments after I get that done. |
|
@andrewdnolan I've pushed various fixes for bugs I introduced on rebase and I have implemented most of your suggested changes. I still need to fix the latest conflict with the recent doc changes for Split Explicit. Also, although I'm not too concerned on the use of time interval for setting a time in the future (the internal representation in time manager uses long long for most things), I will probably modify that to a fixed large time per your suggestion. But you should be able to test this version. Note that the current HEAD fails a few of the ctests, at least on chrysalis, so this branch fails the same ones - I will create an issue for that (and a lack of omega.log file from ctest). |
c97f732 to
95b3808
Compare
|
@xylar and @andrewdnolan This should all be cleaned up and ready to go again. Since the SplitExplicit was merged after this PR was originally submitted, the rebase was a bit involved. I've incorporated all review suggestions and all tests pass on chrysalis with the exception of those that are currently failing for the HEAD of develop and unrelated to this PR. |
Polaris
|
| Omega | -p |
|
|---|---|---|
| baseline | develop 5dfd2ae971 |
/lcrc/group/e3sm/ac.xylar/polaris_1.1/chrysalis/test_20260921/build_omega_develop_5dfd2ae971 |
| PR | 9034abef18 |
/lcrc/group/e3sm/ac.xylar/polaris_1.1/chrysalis/test_20260921/build_omega_stop_time_changes_9034abef18 |
19 of 24 tasks run on both sides and all 108 baseline comparisons are bit-for-bit. Work directories:
/lcrc/group/e3sm/ac.xylar/polaris_1.1/chrysalis/test_20260921/pr731_baseline
/lcrc/group/e3sm/ac.xylar/polaris_1.1/chrysalis/test_20260921/pr731_branch
The other five fail identically on both sides with #447's HorzTracerFluxOrder must be greater than 2 check, which polaris#790 fixes on the Polaris side; unrelated to this PR.
The new realistic_global/QU.240km/restart task (the #482 regression test) inherits the same order-2 setting, so it was run from polaris#731 merged with polaris#790 against this PR's build. It passes, with the continued segment ending with all four history frames (1800 to 7200 s), matching the uninterrupted run. It has no baseline; that it fails on the pre-fix behavior was checked in August.
Posted by Claude Code on @xylar's behalf. The testing, analysis and wording above are AI-authored; please check them accordingly.
xylar
left a comment
xylar
left a comment
There was a problem hiding this comment.
Approved based on my Omega and Polaris testing with help from Claude and Phil's testing.
Thanks so much, @philipwjones!
|
Thanks @xylar Did you want to coordinate a merge with downstream polaris mods once @andrewdnolan approves or just merge this and deal with it after? |
|
I'll take care of the Polaris side when the time comes. So no need to wait here. |
andrewdnolan
left a comment
andrewdnolan
left a comment
There was a problem hiding this comment.
Testing:
I ran the e3sm_omega_developer suite on chrysalis with the gnu compiler and all tests pass.
ERS_Vmct_Ln5.TL319_EC30to60E2r2.COMEGA-JRA1p5.chrysalis_gnu.omega-jra_1958 (Overall: PASS)
ERS_Vmct.T62_oQU240.COMEGA-IAF.chrysalis_gnu (Overall: PASS)
PEM_Vmct.T62_oQU240.COMEGA-IAF.chrysalis_gnu (Overall: PASS)
SMS_Vmct_Ln5.TL319_EC30to60E2r2.COMEGA-JRA1p5.chrysalis_gnu.omega-jra_1958 (Overall: PASS)
SMS_Vmct.T62_oQU240.COMEGA-IAF.chrysalis_gnu (Overall: PASS)
@philipwjones thanks for addressing the previous comments! Sorry about the delay, I was out of the office yesterday and Friday.
In testing I'm realizing there might an inconsistency between the meaning of a Branch StartType as implemented here and a branch run as determined by CIME. What you've implemented here as Branch is closer to CIME's hybrid RUN_TYPE value. I'll look into this and comment on the relevant code. Either way we'll probably want to guard the user from setting StartType in the user_nl_omega because this is something that comes from the coupled driver.
|
@andrewdnolan thanks - that does appear to be the case looking at CIME documentation, so for the purposes of this pr, branch acts just like continue and hybrid changes the start time. I'll add the hybrid option and make those changes to code and documentation to be consistent with E3SM. And I will add StartType to the variables protected in the build nml files. |
|
Thanks @philipwjones! Sorry for not catching that in my initial review. Matching Omega's I guess one way to address this would be set I'm not exactly sure what this additional |
|
@andrewdnolan Well, I can use FromCoupler (and maybe also change the StopType to FromCoupler for consistency?). And the start_type and RUN_TYPE are not equivalent anyway - there is a translation in CIME from the input RUN_TYPE to the cpl driver and the integer sent to components: <value run_type="startup" continue_run=".false.">startup</value>
<value run_type="hybrid" continue_run=".false.">startup</value>
<value run_type="branch" continue_run=".false.">branch</value>
<value run_type="startup" continue_run=".true.">continue</value>
<value run_type="hybrid" continue_run=".true.">continue</value>
<value run_type="branch" continue_run=".true.">continue</value>so the standalone options will have to be treated differently from the FromCoupler option anyway. |
|
Gotcha, okay thanks for pointing that out. I agree making the Thanks @philipwjones. |
| // Simulation will stop on an external signal so no StopTime | ||
| // or Duration are needed. | ||
| std::string StopTimeStr = "9999-12-31_00:00:00"; | ||
| // Set Duration and StopTime with long future values for a dummy | ||
| // EndAlarm | ||
| Duration = TimeInterval(1.e16, TimeUnits::Seconds); | ||
| StopTime = TimeInstant(StopTimeStr); |
There was a problem hiding this comment.
I was seeing a fail for the TimeStepper CTest with a local merge of this PR into develop. Claude believes the reason is because parsing the hardcoded StopTimeStr fails when using the NoCalendar option. The suggested fix is:
| // Simulation will stop on an external signal so no StopTime | |
| // or Duration are needed. | |
| std::string StopTimeStr = "9999-12-31_00:00:00"; | |
| // Set Duration and StopTime with long future values for a dummy | |
| // EndAlarm | |
| Duration = TimeInterval(1.e16, TimeUnits::Seconds); | |
| StopTime = TimeInstant(StopTimeStr); | |
| // Simulation will stop on an external signal so no StopTime | |
| // or Duration are needed. | |
| // Set Duration and StopTime with a long future value for a dummy | |
| // EndAlarm. The dummy stop time is computed by adding a large | |
| // non-calendar interval to the current time (rather than parsing a | |
| // hardcoded calendar date string) so this works under any Calendar | |
| // type, including NoCalendar. | |
| TimeInstant CurrentTime = StepClock->getCurrentTime(); | |
| Duration = TimeInterval(1.e16, TimeUnits::Seconds); | |
| StopTime = CurrentTime + Duration; |
There was a problem hiding this comment.
This effectively undoes what @andrewdnolan asked for in #524 (comment).
I think that's fine because I don't think @andrewdnolan's rollover worry hold up: 1e16 s is well under the time manager's 1e18 s input limit and the 64-bit integer range. Here's another option but the complexity is likely not warranted:
Duration = TimeInterval(1.e16, TimeUnits::Seconds);
if (Calendar::getKind() == CalendarNoCalendar) {
StopTime = TimeInstant(0, 0, 0, 0, 0, 1.e16);
} else {
std::string StopTimeStr = "9999-12-31_00:00:00";
StopTime = TimeInstant(StopTimeStr);
}There was a problem hiding this comment.
Yeah, it's possible that's why I did it this way the first time - have a vague memory of dealing with No Calendar options. I'll revert back to the interval add with the next commit once I've modified the Coupled options.
- adds a config stop option to support stopping
at a time, after an interval, or on a signal from coupler
- adds a start option to support start-up, continuation and
branch runs
- fixes some related clock and clock reset issues on restart that
were impacting a number of situations, especially in streams
for appended files
- modifies buildnml for new start/stop options
- also changes calculation of stop time for coupled sims
9034abe to
5d52959
Compare
|
@xylar and @andrewdnolan I've just committed changes that change both the start and stop options to coupled for coupled configurations. And reverts the calculation of end time for coupling. It has also been rebased again. It passes all CTests on Chrysalis but could probably use another round of testing otherwise. And you should also check my changes to the cime config utilities since I'm less familiar with that. Hopefully this satisfies all the review comments/requests. |
|
@philipwjones, I'll try to do a full round of testing tomorrow and hopefully merge if @andrewdnolan has approved. Thanks so much! |
|
Thanks @philipwjones! I will re-test, in coupled mode, this afternoon. |
Testing: chrysalis, oneapi-ifx, openmpi (Polaris
|
Testing: frontier, craygnu, mpichCTests: 50 of 50 passed. Tested the merge of #524 ( Polaris
|
Testing: aurora, oneapi-ifx, mpichCTests: 50 of 50 passed. Tested the merge of #524 ( Polaris
|
Testing: pm-cpu, gnu, mpichCTests: 50 of 50 passed. Tested the merge of #524 ( Polaris
|
Testing: pm-gpu, gnugpu, mpichCTests: 50 of 50 passed. Tested the merge of #524 ( Polaris
|
|
I'm just waiting on the final frontier craygnu-mphipcc test, and @andrewdnolan's coupled testing before I'll merge. |
Testing: frontier, craygnu-mphipcc, mpichCTests: 50 of 50 passed. Tested the merge of #524 ( Polaris
|
andrewdnolan
left a comment
andrewdnolan
left a comment
There was a problem hiding this comment.
Testing: Chrysalis, intel
Ran the e3sm_omega_developer suite and all tests pass!
ERS_Vmct_Ld3.ne4_oQU240.WCYCL1850NS-OMEGA.chrysalis_intel (Overall: PASS)
ERS_Vmct_Ln9.TL319_EC30to60E2r2.GOMEGA-JRA1p5.chrysalis_intel.omega-jra_1958 (Overall: PASS)
ERS_Vmct.T62_oQU240.COMEGA-IAF.chrysalis_intel (Overall: PASS)
PEM_Vmct_Ln9.TL319_EC30to60E2r2.GOMEGA-JRA1p5.chrysalis_intel.omega-jra_1958 (Overall: PASS)
PEM_Vmct.T62_oQU240.COMEGA-IAF.chrysalis_intel (Overall: PASS)
PEM_Vmct.T62_oQU240.GOMEGA-IAF.chrysalis_intel (Overall: PASS)
SMS_Vmct_Ld3.TL319_EC30to60E2r2.COMEGA-JRA1p5.chrysalis_intel.omega-jra_1958 (Overall: PASS)
SMS_Vmct.ne4_oQU240.WCYCL1850NS-OMEGA.chrysalis_intel (Overall: PASS)
SMS_Vmct.T62_oQU240.GOMEGA-IAF.chrysalis_intel (Overall: PASS)
output in in: /lcrc/group/e3sm/ac.anolan/scratch/chrys if anyone is interested.
TestingI also confirmed the guarding in to my That's for all the work @philipwjones, this will great to have in! |
|
Thanks @andrewdnolan! |
This brings in E3SM-Project/Omega#524, which changes how Omega's stop time is specified. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
4 participants
In an attempt to fix various restart issues, the methods for starting and stopping a run have been refactored. These are more fully described in the user and developer guides in the time stepping sections, but include:
These changes required changes to the input config so after this PR, users/developers will need to update their config files accordingly.
Checklist
Documentation:
Linting
Building
Testing
aurora, oneapi-ifx, mpich
chrysalis, oneapi-ifx, openmpi
frontier, craygnu, mpich
frontier, craygnu-mphipcc, mpich
pm-cpu, gnu, mpich
pm-gpu, gnugpu, mpich
Provide relevant details in a comment to the PR titled
Testingwith the following:have been run on and indicate that are all passing.
has passed, using the Polaris
e3sm_submodules/Omegabaseline-pfor both the baseline (Polarise3sm_submodules/Omega) and the PR buildFixes #482