Repository navigation
Conversation
…to 874.refactor-fre-yamltools
…to 874.refactor-fre-yamltools
|
This PR would probably need to go along with these two: |
|
NOTES:
|
- suppress output for when fre list yamls is not piped to fre yamltools combine - switch combining order for platforms and compile (so users can define reusable variables in platforms)
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical integration and packaging issues block safe approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 7
Open (12)
What changed in this PR
Refactors YAML composition around uwtools, updates fixtures and CLI/listing workflows, and removes obsolete AM5 examples.
Changes:
- Adds
uwtoolspackaging dependencies. - Migrates YAML fixtures and tests to the new composition model.
- Adds the refactored combiner and piped YAML support.
| File | Summary |
|---|---|
meta.yaml |
Adds the uwtools dependency. |
fre/yamltools/tests/yamls/settings.yaml |
Adds shared settings fixture. |
fre/yamltools/tests/yamls/pp_yamls/pp.c96_amip.yaml |
Migrates post-processing configuration. |
fre/yamltools/tests/yamls/pp_yamls/pp-TEST.c96_amip.yaml |
Updates test post-processing configuration. |
fre/yamltools/tests/yamls/model.yaml |
Adds the renamed model fixture. |
fre/yamltools/tests/yamls/grid_yamls/TEST_grids.yaml |
Adds grid fixture data. |
fre/yamltools/tests/yamls/compile_yamls/platforms.yaml |
Migrates platform properties. |
fre/yamltools/tests/yamls/compile_yamls/compile.yaml |
Migrates compile properties. |
fre/yamltools/tests/yamls/compile_yamls/compile_fail/wrong_platforms.yaml |
Updates failing platform fixture. |
fre/yamltools/tests/yamls/compile_yamls/compile_fail/wrong_compile.yaml |
Updates failing compile fixture. |
fre/yamltools/tests/yamls/compile_yamls/compile_fail/am5-wrong_datatype.yaml |
Adds datatype failure fixture. |
fre/yamltools/tests/yamls/compile_yamls/compile_fail/am5-wrong_compilefile.yaml |
Adds missing compile-file fixture. |
fre/yamltools/tests/yamls/COMPARE_TEST_OUTPUT_cmor.yaml |
Adds expected CMOR output. |
fre/yamltools/tests/yamls/cmor_yamls/cmor.yaml |
Migrates CMOR references. |
fre/yamltools/tests/yamls/analysis_yamls/land.yaml |
Migrates land analysis properties. |
fre/yamltools/tests/yamls/analysis_yamls/clouds.yaml |
Migrates cloud analysis properties. |
fre/yamltools/tests/test_combine_yamls_script.py |
Updates combiner tests. |
fre/yamltools/tests/AM5_example/settings.yaml |
Removes obsolete settings fixture. |
fre/yamltools/tests/AM5_example/pp_yamls/pp.c96_amip.yaml |
Removes obsolete post-processing fixture. |
fre/yamltools/tests/AM5_example/compile_yamls/compile_fail/am5-wrong_datatype.yaml |
Removes obsolete failure fixture. |
fre/yamltools/tests/AM5_example/compile_yamls/compile_fail/am5-wrong_compilefile.yaml |
Removes obsolete failure fixture. |
fre/yamltools/tests/AM5_example/am5.yaml |
Removes obsolete AM5 fixture. |
fre/yamltools/helpers.py |
Updates YAML helper imports. |
fre/yamltools/freyamltools.py |
Adds the new combine CLI workflow. |
fre/yamltools/combine_yamls_script_new.py |
Implements uwtools-based composition. |
fre/yamltools/__init__.py |
Removes legacy constructor registration. |
fre/list_/list_yamls_script.py |
Adds piped YAML support. |
environment.yml |
Adds the uwtools channel and dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # we don't need all this output but maybe it can be wrapped up in fre_logger.debug if needed? | ||
| with open(os.devnull, 'w', encoding="utf-8") as f, contextlib.redirect_stdout(f): | ||
| print("This will not be displayed") | ||
| combined_yaml_dict = config.compose(configs=yaml_list, realize=True).as_dict() |
There was a problem hiding this comment.
I think this call to uw config compose will fail if an empty settings yaml is used, as it will be trying to compose a directory.
|
|
||
| # this boots yaml with !join- see __init__ | ||
| import json | ||
| import yaml |
| PP_AMIP_CHUNK96: "P1Y" | ||
| PP_AMIP_CHUNK384: "P1Y" | ||
| PP_XYINTERP96: "180,288" | ||
| PP_XYINTERP384: "720,1152" |
| # check_fre_version(combined) | ||
| init_obj = LetsGo(yamls, experiment, platform, target, output) | ||
|
|
||
| init_file = f"{Path.cwd()}/init.yaml" |
There was a problem hiding this comment.
I was wondering if this init file is strictly necessary, and if so, it would be safer to use a defined temporary directory instead of a known filename in the current directory. (What if someone had a init.yaml there for some other project? This would remove it.)
| @click.option("--use", type=click.Choice(['compile','pp', 'cmor']), | ||
| help="Process user is combining yamls for. Can pass 'compile', 'pp', or 'cmor'", required=True) |
There was a problem hiding this comment.
Will be removed (old subtool) with other PRs
| try: | ||
| #out = | ||
| cy.consolidate_yamls(modelyaml, COMP_EXPERIMENT, COMP_PLATFORM, COMP_TARGET, use, output = None) | ||
| cy.yamltools_combine_subtool(yamls, COMP_EXPERIMENT, COMP_PLATFORM, COMP_TARGET, output = None) |
- updates to other tests will ideally be done in PRs after this is in
…m:singhd789/fre-cli into 874.refactor-fre-yamltools
…to 874.refactor-fre-yamltools
update fre list subtools and tests for uwtools integration
update fre pp configure-yaml and tests for uwtools integration
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #975 +/- ##
==========================================
- Coverage 83.46% 78.96% -4.51%
==========================================
Files 68 69 +1
Lines 3811 3903 +92
==========================================
- Hits 3181 3082 -99
- Misses 630 821 +191
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
…m:singhd789/fre-cli into 874.refactor-fre-yamltools
|
Note: this PR might help the codecov - #984 Let's try to get that one in first. |
|
|
||
| yamls = ly.list_yamls_subtool(yamlfile = yml, | ||
| experiment = e, | ||
| application= "postprocess") |
There was a problem hiding this comment.
Could this target for the list yamls be general and not specific to postprocessing?
There was a problem hiding this comment.
application = ['postprocess', 'analysis']
?
But even then we're stuck with hard-coding postprocess and analysis here in configure_script_yaml.py
Ideally, we want to retrieve these application keys from the yaml, right?
| self.y = yamls.split(",") | ||
| self.e = str(experiment) | ||
| self.p = str(platform) | ||
| self.t = str(target) |
There was a problem hiding this comment.
Should experiment, platform, and target be required or optional?
If they are optional like this, None will be the result if the yaml refers to the experiment, platform, or target.
I suggest we should either make them required, or have a hard error if the yaml uses them and they are not defined.
| :type no_clean: boolean | ||
| """ | ||
| # fre_logger.info('checking fre_cli_version compatibility...') | ||
| # check_fre_version(combined) |
There was a problem hiding this comment.
This FRE version checking is on my mind, ha. I'm thinking about this Niki problem with using fre/test which is related to this.
Should this be uncommented?
| fre_logger.error("List experiments via `fre list -y [model YAML]`") | ||
| raise ValueError("Experiment passed DNE") | ||
|
|
||
| settings_data = "" |
There was a problem hiding this comment.
settings yaml is optional. If it's not used, I'm wondering if it should be None rather than an empty string.
| if application not in exp_data.keys(): | ||
| raise ValueError(" *** Application passed is not defined in the model YAML. *** ") | ||
|
|
||
| yamls.extend([platform_data, settings_data]) |
There was a problem hiding this comment.
Using settings_data here when the user doesn't have a settings yaml might be safe, but seems iffy if the settings_data is an empty string.
| fail = [] | ||
| fre_logger.info("") | ||
| for y in yamls_full_path.split(","): | ||
| if not Path(y).exists(): |
There was a problem hiding this comment.
If the settings yaml is not used, the empty string is still part of the yaml list, so this check passes because the base directory exists.
| yaml_list.extend(self.y) | ||
|
|
||
| for y in yaml_list: | ||
| if not Path(y).exists(): |
There was a problem hiding this comment.
I think that an empty settings yaml would also pass here, due to the parent directory existing.
Describe your changes
Issue ticket number and link (if applicable)
Fixes #XXX (replace XXX with the issue number and GitHub will autolink the PR to the issue)
Checklist before requesting a review
Note: If you are a code maintainer updating the tag or releasing a new fre-cli version, please use the
release_procedure.mdtemplate. To quickly use this template, open a new pull request, choose your branch, and add?template=release_procedure.mdto the end of the url.