[uwtools_integration] Integrate run_fcst #271
Conversation
…-srweather-app into integrate-run-fcst
Model configure to yaml
…niNOAA/ufs-srweather-app into integrate-run-fcst
christinaholtNOAA
requested review from
NaureenBharwaniNOAA,
elcarpenterNOAA and
maddenp-noaa
as code owners
| # the corresponding surface climatology fields (the second column of the | ||
| # array). | ||
| # These are the names of the fields generated by the make_sfc_climo | ||
| # task. |
There was a problem hiding this comment.
This file was a lot of shell-based stuff that didn't work with "flattening" the YAML in the run scripts. The relevant information has been moved to the FV3 YAML.
Two sections remain here. The first describes the list of variables that come out of make_sfc_climo, which the UW Driver could absorb once the output method is instantiated on that class. The second describes the set of required fix files the Thompson mp scheme needs. Perhaps those belong over in ccpp_suites_defaults.yaml? I'll take another peek at that.
There was a problem hiding this comment.
Are those Thompson mp input files eventually involved in a uw fs copy or link action? Mostly just curious how the list is used w.r.t. uwtools.
There was a problem hiding this comment.
Yes. They do eventually get added to the files_to_link section of the fv3 UW YAML. Instead of listing the names of CCPP Suites that use Thompson MP, though, SRW calls a function that loads the XML to find out if Thompson is used. I could definitely clean up this section and move it over to CCPP suites to put it closer to its other relevant settings.
|
Switching to in-progress. I forgot to check the test files for relevant changes. |
There was a problem hiding this comment.
I made a couple comments and asked a couple questions, and will follow-up on answers and/or do another pass if it's helpful, though I don't see any reason this couldn't be merged. Some of my suggestions might apply to past and future PRs on this branch, too, and might be better addressed in a separate PR once everything is ready -- for example, suggestions to DRY things out across scripts.
| # the corresponding surface climatology fields (the second column of the | ||
| # array). | ||
| # These are the names of the fields generated by the make_sfc_climo | ||
| # task. |
There was a problem hiding this comment.
Are those Thompson mp input files eventually involved in a uw fs copy or link action? Mostly just curious how the list is used w.r.t. uwtools.
| logging.error(f"Value at {pathstr} must be a dictionary") | ||
| sys.exit(1) | ||
| config = subconfig | ||
| return config |
There was a problem hiding this comment.
Do we maybe have multiple copies of this in SRW now? I think I see at least one more in chgres_cube.py. Should we think about a support.py or common.py module to factor out repetition between the scripts?
There was a problem hiding this comment.
Historically, we've had to put those types of files in the ush directory. If we did this, I'd want it in the same scripts directory so that we don't have to manage a PYTHONPATH.
Unless, of course, you are talking about opening it to the uwtools API. Then, yes, I think that would be great. I've essentially copied this directly from uwtools anyway.
I'd like to hear your thoughts on both options.
There was a problem hiding this comment.
I hadn't thought of exposing the uwtools version. Hmm... We could consider something like uwtools.api.support or uwtools.api.utils where we could expose otherwise-internal functions that we determine are more widely useful. Of course, that increases the API surface area and the risk of breaking changes when we need to change previously-internal functionality to suite still-internal needs. I think maybe we should keep this in mind but maybe wait until we have some kind of critical mass -- like 3 or more functions uwtools functions that we find ourselves reimplementing in apps -- before going this route.
I was thinking of just something alongside the other scripts that they could import.
The common module should not have executable bits set... :)
| if linkname.is_symlink(): | ||
| linkname.unlink() | ||
| logging.info(f"Linking {linkname} -> {path}") | ||
| linkname.symlink_to(path) |
There was a problem hiding this comment.
Any reason not to use uw fs link or its API equivalent for this? Maybe one reason: This unlinks files first and re-does work vs the uwtools idempotent behavior. Should we think about adding an option to the fs actions to force them to remove and then recreate their targets?
There was a problem hiding this comment.
The unlinking was the main reason, I think. It could be nice to stay consistent. This solution, for example, doesn't do any logging on it's own, so I had to add it in.
I wouldn't hate an "unlink" option for our fs tool.
There was a problem hiding this comment.
This might be related to this, which would require reliable output() methods to know what to remove, but this is simpler, as there's only a file or symlink to rm -- or, maybe, a rm -rf for a directory -- for fs targets. Implementing --reset for fs might be a good start.
On the other hand, I do wonder about mixing behaviors: In some parts of an app's automation, logic (like this) that repeatedly clobbers existing on-disk state; and in other parts (like invocation of drivers) logic that is idempotent. Maybe that's ok, but I wonder if users will understand which part has which behavior.
There was a problem hiding this comment.
This came in handy during the debugging step when we were trying to get the linking correct. We could add back the idempotence here by just saying if it exists, don't try to re-link. It is external to the rundir though, so this behavior could send users on a wild goose chase through Timbuktu to unlink everything a given SRW script might try to link.
There was a problem hiding this comment.
We could add back the idempotence here by just saying if it exists, don't try to re-link.
Would that then be equivalent to using uw fs link (CLI or API)? I'm just angling for using uwtools functionality where possible, of course. I'll leave it at that -- perfect is not the enemy of good! (And maybe this is even better, I don't know.)
| default="000", | ||
| help="The 3-digit ensemble member number.", | ||
| ) | ||
| return parser.parse_args(argv) |
There was a problem hiding this comment.
In the parse_args() functions of the new scripts we're adding, we could also DRY thinks out in a support module, e.g. have
def add_argument_cycle(parser):
parser.add_argument(
"--cycle",
...
and then here just call support.add_argument_cycle(parser). It seems like we're going to have a lot of repeated arguments across scripts where they really should all be the same. In some cases we might want
def add_argument_foo(parser, required: bool = True):
parser.add_argument(
...
required=required,
...
if there are arguments that are required for some scripts and not for others.
There was a problem hiding this comment.
I'm not entirely sure that the SRW devs would use that consistently. I can see a future where SRW would have some scripts that we contribute that would use that kind of tool, and others that use argparse directly. I doubt this would be an enforceable construct to the community.
I'm stuck here in the middle of explicit (in the script) vs abstracting out the commonalities and which would be more friendly toward SRW developers.
There was a problem hiding this comment.
I'd imagine that developers would mostly copy existing patterns. If the pattern they observe is copy-paste, we'll certainly get more of that -- and then subtle drift over time as ad hoc changes are made without regard to the larger design. If they see much shorter code that uses something imported, I suppose they'll copy that. I feel like we can at least try to set a good example and establish a useful pattern. Whether or not what I'm suggesting is "good" or "useful" -- and I agree that there's a balance between explicit and DRY -- is your call.
I doubt this would be an enforceable construct to the community.
That's what PRs are for, but if reviewers won't request adherence to convention, there's not much we can do.
There was a problem hiding this comment.
but if reviewers won't request adherence to convention, there's not much we can do.
That's been my main concern.
We get such a mix of developers in the apps that some will go for copy/paste within SRW, and others will try to write something from scratch (that's sometimes quite thrilling) or copy/paste it from somewhere else to try to slot it in. That doesn't work yet (give us time)!
We could definitely try it out as a potential clean-up item.
| base_file: '{{ workflow.EXPTDIR }}/input.nml.{{ workflow.CCPP_PHYS_SUITE }}' | ||
| update_values: | ||
| atmos_model_nml: | ||
| blocksize: !int '{{ task_run_fcst.BLOCKSIZE // 1 }}' |
There was a problem hiding this comment.
What else might it be, and what does // 1 trigger if it's the wrong thing -- exit with exception and stacktrace? Is this meant to avoid a problem with !int?
| f""" | ||
| if metatask in rocoto_config["tasks"]: | ||
| logging.info( | ||
| dedent( |
There was a problem hiding this comment.
Is this really meant to be a multiline comment? I mean, should the comment really span multiple lines on the console? I'm guessing not... If there are multiple lines, I think they should probably be subject to separate logging.info() calls, as otherwise e.g. greping for a log message of interest may only get part of the message, if it's split over lines. IMO long log messages are fine. If it's just that the line is too long in Python, I think this could just be like
>>> logging.info(
... "This is a very long log message but I want to control how long"
... " it appears in Python because it is difficult to read very long"
... " lines that wrap around"
... )
[2024-11-25T16:48:16] INFO This is a very long log message but I want to control how long it appears in Python because it is difficult to read very long lines that wrap around
Any of the strings could be f-strings.
There was a problem hiding this comment.
I really like this argument. I think it will be out of scope for the set up main SRW integration PRs, but I'd happily come back and take out this nonsense in a cleanup PR!
Some SRW developers really want to control how messages show up in the console, too.
DESCRIPTION OF CHANGES:
These changes allow the forecast to be configured and run by UW Tools.
CONTRIBUTORS (optional):
@NaureenBharwaniNOAA