Fix #4122: Move CALENDAR to env_run.xml to allow keepexe with calenda… - #685
Fix #4122: Move CALENDAR to env_run.xml to allow keepexe with calenda…#685johnpaulalex wants to merge 2 commits into
Conversation
ekluzek
left a comment
There was a problem hiding this comment.
This is the correct change. And it would be helpful to have this come in.
I wondered if that was the right group for it to be in, but that is the best one based on the other choices. It does make sense, since it's the calendar that will be used to defie what the beging, stop and restart dates refer to.
Since, this is a simple change the simple testing done is good enough to me. Once, it comes in we should also try it in CTSM for some of those cases that were failing in the PLUMBER2 PR coming in.
|
Pinging @billsacks or @fischer-ncar to approve and make the merge. |
|
@johnpaulalex be sure to credit any AI that you used to do this work. |
|
Thanks Erik! Gemini is now credited :) |
billsacks
left a comment
There was a problem hiding this comment.
I tend to be nervous about making changes like this without understanding the reason why things were set up as they originally were. I spent some time trying to dig through history for why CALENDAR was put in env_build rather than env_run but gave up. I don't see any problems with this myself.
@mvertens do you know of any reasons why CALENDAR would need to be in env_build? I added you as a reviewer to mark that we should wait to merge until you've had a chance to comment, but if you don't feel a need to look at this, we can merge without your review.
I do see one other minor change needed: In namelist_definition_drv.xml, there is text "set by CALENDAR in env_build.xml", which should be updated to env_run.xml.
Good point. The one thing I wondered about is that if you change it midrun -- maybe it causes problems? And it seems likely it would. I'm not sure it would be easy to disallow changing if CONTINUE_RUN==TRUE either. So that might be why it was added to env_build? Having it in env_build restricts changes more than is needed, but just being in env_run might not restrict it enough? @mvertens and @billsacks what do you think about that? |
|
I fixed the comment Bill mentioned. AI traced the initial placement back through several repos back to 2015: What the Git & Code Data Actually Shows (Empirical Facts)
What is Historical Context / Inference (Domain Knowledge)
SummaryThe placement of |
I was just coming back here to write almost exactly this: this thought also occurred to me after I submitted my initial review - this felt like a possible reason it was put in env_build. I also spent a little more time digging through older history. AI fell short here because it didn't know to look in the older subversion repository. In fact, CALENDAR was moved from env_run to env_build in r36936 of https://svn-ccsm-models.cgd.ucar.edu/scripts/trunk - a change made by @apcraig . There is no comment attached to that commit, and at a glance, I don't see an obvious reason why CALENDAR needed to be moved to env_build in that commit. In all, I still feel okay with this unless @mvertens or @apcraig wants to comment on why CALENDAR belongs as a build-time setting. A comment may be warranted warning users not to change this setting on a CONTINUE_RUN, but maybe that goes without saying, so I don't feel strongly one way or the other. |
|
I'll also say, if the purpose of this is partly to explore the value of AI tools in diagnosing problems without pulling in the relevant human/institutional knowledge: I'd say the AI failed in its diagnosis here: Its focus on the timing of CALENDAR vs --keep-exe is misguided, because there were other reasons driving decisions of placement in env_run vs. env_build before --keep-exe was introduced, namely whether a rebuild was needed for any reason when changing a variable. @johnpaulalex I do appreciate your contribution here, but at the same time, as we experiment with relying more heavily on AI tools, I feel it's important to have open conversations about their shortcomings. |
|
I trust the AI for the timeline above, and find that part helpful. I think it's speculation regarding --keepexe and run_tower is not as helpful though. With it giving the timeline I was able to confirm what it said about the initial checkin. At least as far back as we can go with github.com which is 11 years ago from the svn branch. It looks like svn is closed now so I can't see the history previous to that. I thought we had done more to save history on the move to github, but looks like we didn't for this code. I'm still thinking there could be bad behavior if you try to change this during a run. But, we probably need to test for that. Looking at the code, the calendar is stored on the restart file, but it is NOT used to set the calendar. There doesn't seem to be a read of the calendar in med_phases_restart_read. So it looks to me, like you could mess up time for your simulation if you changed the calendar when CONTINUE_RUN==TRUE. |
Description of changes
Moves the
CALENDARvariable definition incime_config/config_component.xmlfromenv_build.xml(groupbuild_def) toenv_run.xml(grouprun_begin_stop_restart).This resolves an issue where cloning a case with
--keepexeand applying usermods that alter theCALENDARvariable (such as during PLUMBER2 or NEON site spinups) failed with:ERROR: env_build.xml cannot be changed via usermods if keepexe is an optionSpecific notes
CALENDARvariable is strictly a runtime configuration parameter used by Python setup and namelist-building scripts (buildnml). It is never referenced in preprocessor macros or build system logic.CALENDARtoenv_run.xmlaccurately reflects its runtime nature and allows CIME'scase_clone.pyscript to accept calendar modifications under--keepexewithout requiring a re-compilation.Contributors other than yourself, if any: Antigravity using Gemini LLMs came up with the fix, looked for any other CALENDAR references, ran the tests below, and wrote this PR (except this one line :))
CMEPS Issues Fixed (include github issue #): Fixes ESCOMP/CTSM#4122
Are changes expected to change answers? (specify if bfb, different at roundoff, more substantial)
BFB (Bit-for-bit). This change only alters the XML file location (
env_run.xmlinstead ofenv_build.xml) where theCALENDARvariable is stored during case creation.Any User Interface Changes (namelist or namelist defaults changes)?
No namelist changes. The XML variable
CALENDARis now stored inenv_run.xmlinstead ofenv_build.xml.Testing performed
Please describe the tests along with the target model and machine(s)
If possible, please also added hashes that were used in the testing
create_newcase(--compset IHistClm50BgcCrop,--mach ubuntu-latest) and ran./case.setup../xmlchange CALENDAR=GREGORIANinshell_commands.create_clone --keepexe --user-mods-dir <usermod_path>against the base case.create_clonefailed with anenv_build.xmlvalidation error. With this change,create_clonecompletes successfully with--keepexe.