feat(config): migrate mizuRoute configuration parsing to native TOML format - #646
feat(config): migrate mizuRoute configuration parsing to native TOML format#646johnpaulalex wants to merge 6 commits into
Conversation
032278b to
c0fea92
Compare
c0fea92 to
ecd153b
Compare
ecd153b to
8989fe6
Compare
ekluzek
left a comment
There was a problem hiding this comment.
It looks like this removes the old control files, but also has logic to keep them and just add the new TOML file format. As I understand the transition plan this first step needs the old control files to be kept around until they can be removed when the TOML file reading is brought in.
So I think the way this should go is that at this step it should handle both the control file format and the TOML format. So both are kept around. But, at this step it actually reads in the control file format. But, there can be testing/checking that the python is handling the TOML format correctly.
| user_ctl_file = os.path.join(caseroot, file_src + inst_string) | ||
| if ( not os.path.exists( user_ctl_file ) ): | ||
| safe_copy( os.path.join( srcroot, "cime_config", file_src), user_ctl_file ) | ||
| file_src_toml = "user_nl_mizuroute.toml" |
There was a problem hiding this comment.
Use a underscore rather than a dot to make it match how it's done for the control file.
There was a problem hiding this comment.
| file_src_toml = "user_nl_mizuroute.toml" | |
| file_src_toml = "user_nl_mizuroute_toml" |
There was a problem hiding this comment.
Although there are also advantages to the .toml extension, so it really only matters if others have an opinion on this.
There was a problem hiding this comment.
Actually thinking about it though. For the user_nl_* files in CESM we never use an extension we only use underscores, so we should probably keep that convention until CESM changes it for the user_nl_* files.
| @@ -2,8 +2,7 @@ | |||
| ! This is for changes to the mizuRoute control file only | |||
| ! Changes to the namelist file need to go in the user_nl_mizuRoute file | |||
| ! | |||
There was a problem hiding this comment.
This should stay the same if the control file is going to be left around.
There was a problem hiding this comment.
I thought we'd have users specify their overrides in a toml file going forward, and merge that with the case's toml
There was a problem hiding this comment.
I talk about this below. We thought keeping the ability to read either format would be good for a transition time. But, this was also from my understanding that this PR doesn't change the code to read TOML format -- but a future PR will do that step. So this can't change until the Fortran code is updated to read TOML format anyway.
I do think it's a good plan to do this in a few steps. And it makes sense to me to make the first step be to just add the toml library to the build -- but don't use it. And then the next step is to actually use it.
There was a problem hiding this comment.
I wonder if this file should be left alone, and a new file mizuRute_toml.py added to handle toml format? Then the control one can be removed once toml format takes over.
| integer, public :: rtmhist_mfilt = 30 ! namelist: number of time samples per tape | ||
| integer, public :: rtmhist_nhtfrq = 0 ! namelist: history write freq(0=monthly) | ||
| character(len=256),public :: cfile_name = 'mizuRoute.control' | ||
| character(len=256),public :: cfile_name = 'mizuroute.toml' |
There was a problem hiding this comment.
This can't really be done, until the Fortran can actually read in the TOML format file. So this change should be removed until then.
There was a problem hiding this comment.
meaning ESMCI/cime#5024? How should that work - get the CIME PR and this one in minus this line, and then flip it on in a later PR?
There was a problem hiding this comment.
My understanding of this PR is that it brings in the ability to build the toml-f library -- but doesn't change the code to read TOML format (until a fture PR is done). As such the old control file format needs to remain, until the ability to read TOML format is in the Fortran code.
The cime PR just allows cime to do differences on TOML format files. So it's nicely independent of this one. And cime isn't a submodule of mizuRoute, so it doesn't apply here anyway.
|
@johnpaulalex thanks for your work on this. I brought it up with a few people and they agree this is a good thing to work on. One of those is @martynpclark and the library he's using to read TOML file format is the same as the library you are using here. So that's a good sign! This is also something that's too low of a priority for us to work on. But, at the same time it's something that makes a good improvement in usability. I talked with @nmizukami about this a bit and we do see several things that need to be done. It might be good to have a meeting to go over it. The most important thing missing is that the standalone build was updated to use the toml-f library. But, the CESM build will also be modified. For that, ccs_config might need changes. And also buildlib under cime_config will need to be updated. The buildlib is currently really simple and it just compiles code in a list of directories. That possibly could be done for toml-f -- but it seems like the toml-f build is really what should be called. We'd also like to have a path forward that has a transition from control file format to TOML format. So both allowed initially, and then later the control file format removed. A tool to help facilitate that would be good to have is a python tool to convert between the formats. I think that shouldn't be hard to write. But, that would allow users with existing control files to convert the format to TOML and show that the results are identical. They may have a bunch of control files they aleady use, so be able to convert to the new format would help facilitate adoption. And later they start a collection of files in TOML format, that they might need to run with an older version and they'll need it converted backwards. This tool could be built on top of the existing mizuRoute_control.py file in route/settings and hopefully wouldn't need many additions to it. So let me know what you think about this. I'll put a summary of checkboxes in a comment below to list the things I see. |
|
These are the list of things that I see that need to go in here:
|
|
Workign on a transition plan with @johnpaulalex and @nmizukami
|
406e743 to
fa3134e
Compare
eed74ec to
0d6c394
Compare
0d6c394 to
272c768
Compare
Description of changes
Migrates mizuRoute's configuration file format from legacy space-delimited control files (
.control) to native TOML (.toml). Integrates thetoml-fFortran library (externals/toml-f) as a git submodule to parse TOML configuration files natively.This PR is split into two logical commits:
feat(deps): add toml-f Fortran library submodule and build system integration(build system & submodule setup).feat(config): migrate mizuRoute configuration parsing to native TOML format(TOML parsing,buildnmlupdates, andmizuRoute_controlrefactoring usingOrderedDict).Updates
cime_configscripts (buildnml,buildlib,mizuRoute_control.py) to generate and processmizuroute.tomlwhile maintaining dual generation of legacymizuRoute.controlduring the transition phase. Extendsbuildnmlto check for$CASEROOT/user_nl_mizuroute.tomlfirst, falling back to$CASEROOT/user_nl_mizuroute_controlif missing.Specific notes
Contributors other than yourself, if any:
Linked issues addressed, if any:
Description of generative AI usage:
Answer Changes & Scientific Impact
User Interface & Namelist Changes
mizuroute.toml)cime_config/buildlib,cime_config/buildnml,.gitmodules)Testing planned or performed, if any:
CTSM / CESM baseline hash-tag:
28514dea3fd28a2f820c7ecaa9014f820c1bb220PR branch hash-tag:
8989fe6Requirements before merge: