Skip to content

Add fre_cli_version to model YAML and enforce version compatibility#768

Open
Copilot wants to merge 5 commits intomainfrom
copilot/add-fre-version-to-model-yaml
Open

Add fre_cli_version to model YAML and enforce version compatibility#768
Copilot wants to merge 5 commits intomainfrom
copilot/add-fre-version-to-model-yaml

Conversation

Copy link
Contributor

Copilot AI commented Mar 11, 2026

Describe your changes

Adds a fre_cli_version top-level key to model YAMLs, checked against the installed fre-cli version before any YAML combining occurs. Mirrors Bronx's FRE_VERSION behavior. The key is named fre_cli_version (not fre_version) to clearly distinguish it from fre-workflows versioning.

Core logic (fre/yamltools/helpers.py):

  • check_fre_version(yamlfile: str) -> None — loads model YAML, compares fre_cli_version to fre.version
    • Match → passes silently (no log emitted on success to avoid polluting caplog-based test assertions)
    • Mismatch → ValueError with clear message
    • Missing → warning (backward compatible)
    • Unparseable YAML (undefined anchors) → warning, continues

Integration (fre/yamltools/combine_yamls_script.py):

  • check_fre_version() called at the top of consolidate_yamls(), before any use-specific logic

Resolved YAML persistence:

  • fre_cli_version is not cleaned from resolved YAMLs — it persists in compile, pp, and cmor outputs for troubleshooting visibility
  • fre_cli_version added as an allowed property in the fre_make.json schema (fre_pp.json already has "additionalProperties": true)
  • CMOR comparison test fixture updated to include the now-preserved fre_cli_version

Documentation (docs/usage/yaml_dev/model_yaml.rst):

  • Added fre_cli_version key documentation and updated example YAML structure

Test YAMLs: All model YAML test fixtures updated with fre_cli_version: "2025.04"

fre_cli_version: "2025.04"

fre_properties:
  - &VERSION "full"
  ...

Bug fix: Removed a debug-level log from check_fre_version() success path that was breaking fre list tests — specifically test_list_pp_components_script.py::test_exp_list, which asserts all captured log records are INFO level.

Note: The fre_make.json schema change is in the gfdl_msd_schemas submodule — a corresponding PR needs to be merged there and the submodule reference updated for CI to pass.

Issue ticket number and link (if applicable)

Checklist before requesting a review

  • I ran my code
  • I tried to make my code readable
  • I tried to comment my code
  • I wrote a new test, if applicable
  • I wrote new instructions/documentation, if applicable
  • I ran pytest and inspected it's output
  • I ran pylint and attempted to implement some of it's feedback
  • No print statements; all user-facing info uses logging module

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Add FRE_VERSION to model yaml and enforce it Add fre_version to model YAML and enforce version compatibility Mar 11, 2026
@ilaflott ilaflott marked this pull request as ready for review March 11, 2026 15:05
@ilaflott ilaflott added new functioning New feature or request priority: HIGH labels Mar 11, 2026
@ilaflott
Copy link
Member

@copilot small bug with fre list introduced, fix

Copy link
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot small bug with fre list introduced, fix

Fixed in 092d7e5. The check_fre_version() was emitting a DEBUG-level log on success ("fre_version check passed: ..."), which got captured by caplog in test_list_pp_components_script.py::test_exp_list. That test asserts all log records are INFO level, so the DEBUG record caused assert 'DEBUG' == 'INFO' to fail. Removed the unnecessary success log — the happy path doesn't need logging.

@codecov
Copy link

codecov bot commented Mar 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.15%. Comparing base (9a0d45f) to head (a7f048b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #768      +/-   ##
==========================================
+ Coverage   84.10%   84.15%   +0.05%     
==========================================
  Files          70       70              
  Lines        4780     4796      +16     
==========================================
+ Hits         4020     4036      +16     
  Misses        760      760              
Flag Coverage Δ
unittests 84.15% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
fre/yamltools/combine_yamls_script.py 86.66% <100.00%> (+0.36%) ⬆️
fre/yamltools/helpers.py 93.50% <100.00%> (+1.44%) ⬆️
fre/yamltools/info_parsers/cmor_info_parser.py 76.63% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a0d45f...a7f048b. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ilaflott
Copy link
Member

ready for actual review

@@ -1,3 +1,5 @@
fre_version: "2025.04"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note (forward looking) - I think if/when we adopt uw tools for yaml combining things, this might be better under fre_properties (I think they'll turn into more key:value pairs versus just being defined as yaml anchors used elsewhere)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO fre_properties should be optional instead of first-class configuration. fre properties in Bronx are user-specified variables, and we mostly have that style in ours now.

# Clean the yaml, the key exists, delete
keys_clean=["name", "platform", "target", # these are needed to create the final parsed dictionary fed to cmor
"fre_properties", "directories", "experiments",
"fre_properties", "fre_version", "directories", "experiments",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it might be beneficial for this to stay and not be cleaned (for more information when troubleshooting maybe?). In that case though, it'll probably have to be added to the gfdl_msd_schema.

Another forward looking thought previously discussed was not doing as much cleaning, so users/workflows have more information to find.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree it would be better to keep it, but the compile/pp schemas (fre_make.json, fre_pp.json) have "additionalProperties": false at the top level, so fre_cli_version in the combined YAML would currently fail schema validation. Left it in keys_clean for now with fre_cli_version (renamed). Adding it to the schemas can be a follow-up.

return y


def check_fre_version(yamlfile):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type hints might need to be added to this file - just noticed this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added -> None return type hint to check_fre_version in a7f048b.

# Clean the yaml
# If keys exists, delete:
keys_clean=["fre_properties", "experiments"]
keys_clean=["fre_properties", "fre_version", "experiments"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same clean note as above

@ilaflott ilaflott force-pushed the copilot/add-fre-version-to-model-yaml branch from 092d7e5 to 31216f4 Compare March 12, 2026 18:08
@ilaflott
Copy link
Member

@copilot understand feedback, implement changes based on it

Copy link
Contributor Author

Copilot AI commented Mar 12, 2026

@copilot understand feedback, implement changes based on it

Done in a7f048b. Changes implemented:

  • Renamed fre_versionfre_cli_version throughout code, docs, and all 7 test YAML fixtures
  • Added -> None return type hint to check_fre_version
  • Kept fre_cli_version in keys_clean for now — the compile/pp schemas have "additionalProperties": false, so adding it to the combined YAML would break schema validation until the schemas are updated (noted as a separate forward-looking change per the review discussion)

Copilot AI changed the title Add fre_version to model YAML and enforce version compatibility Add fre_cli_version to model YAML and enforce version compatibility Mar 12, 2026
## fre -v cmor yaml -y fre/yamltools/tests/esm4_cmip6_ex/esm4_cmip6.yaml \
## -e ESM4_TOAR_2014climo_2050ozone_v2 -p gfdl.ncrc5-intel22 -t prod-openmp

fre_cli_version: "2025.04"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these fre_cli_version references in the test yamls used?

The new test test_helpers.py test the fre-cli version check functionality without this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they're used. When consolidate_yamls() runs in tests, it calls check_fre_version() first, which reads fre_cli_version from these model YAMLs. Without the key, every test that calls consolidate_yamls() would log a warning about missing fre_cli_version. The key also now persists in the resolved YAML output (as of 8991fb0), so it's part of the combined output that tests validate.

Copy link
Contributor

@ceblanton ceblanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep fre_cli_version in the resolved yaml.

If updating the yaml schema is only change needed to support that, let's do that.

# Clean the yaml
# If keys exists, delete:
keys_clean=["fre_properties", "experiments"]
keys_clean=["fre_properties", "fre_cli_version", "experiments"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep fre_cli_version in the resolved yaml.

The only change needed to do this would be to add it to the yaml schema, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 8991fb0. Removed fre_cli_version from keys_clean in both helpers.clean_yaml() and cmor_info_parser.clean_yaml(), so it now persists in all resolved YAMLs (compile, pp, and cmor).

For the schema: fre_pp.json already had "additionalProperties": true so no change needed there. Added fre_cli_version as an allowed string property in fre_make.json (which has "additionalProperties": false). Since the schema is in the gfdl_msd_schemas submodule, a corresponding PR will need to be merged there and the submodule ref updated for CI to pass.

@singhd789 singhd789 linked an issue Mar 19, 2026 that may be closed by this pull request
@ilaflott
Copy link
Member

@copilot

  • update the branch
  • examine feedback and develop code in response, if-needed
  • respond to the feedback in kind
  • if you find you have a strong and confident opinion on what should happen, express yourself

Copy link
Contributor Author

Copilot AI commented Mar 25, 2026

I'd prefer to keep fre_cli_version in the resolved yaml.

If updating the yaml schema is only change needed to support that, let's do that.

Done in 8991fb0. fre_cli_version now stays in the resolved YAML. Added it to the fre_make.json schema (the only schema with "additionalProperties": false). The fre_pp.json schema already had "additionalProperties": true so no change was needed there.

Since the schema is in the gfdl_msd_schemas submodule, a corresponding PR needs to be merged there and the submodule ref updated for CI to pass.

@copilot

  • update the branch
  • examine feedback and develop code in response, if-needed
    ...

Done in 8991fb0. Changes per feedback:

  • Removed fre_cli_version from keys_clean in both helpers.clean_yaml() and cmor_info_parser.clean_yaml() — it now persists in resolved YAMLs
  • Added fre_cli_version to fre_make.json schema (compile path needs it since the schema has "additionalProperties": false; pp schema already has true)
  • Updated CMOR comparison test fixture

All 31 tests pass locally. Note: since fre_make.json is in the gfdl_msd_schemas submodule, a corresponding PR needs to be merged there and the submodule ref updated for CI to pass.

Copilot AI and others added 5 commits March 25, 2026 10:37
- Add check_fre_version() function to fre/yamltools/helpers.py
- Call check_fre_version() in consolidate_yamls() before YAML combining
- Update clean_yaml() in helpers.py and cmor_info_parser.py to remove fre_version
- Add fre_version to all model YAML test files
- Add tests for version checking logic
- Update model_yaml.rst documentation

Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
… test

Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
…caplog assertions

The test_exp_list test in test_list_pp_components_script.py asserts all
captured log records are INFO level. The debug-level log emitted on
successful version check was captured by caplog, causing the assertion
'DEBUG' == 'INFO' to fail.

Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
… YAMLs

Per reviewer consensus (@singhd789, @ceblanton): rename the YAML key
from fre_version to fre_cli_version to distinguish it from fre-workflows
version. Also adds type hint (-> None) to check_fre_version function.

Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
Per @ceblanton's feedback:
- Remove fre_cli_version from keys_clean in helpers.clean_yaml() and
  cmor_info_parser.clean_yaml() so it persists in resolved YAMLs
- Add fre_cli_version as allowed property in fre_make.json schema
  (fre_pp.json already has additionalProperties: true)
- Update CMOR comparison test fixture to include fre_cli_version

Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
Agent-Logs-Url: https://github.com/NOAA-GFDL/fre-cli/sessions/0cd76d05-84ab-4f72-8691-f09098df07f5
@ilaflott ilaflott force-pushed the copilot/add-fre-version-to-model-yaml branch from 8991fb0 to 5d1eca6 Compare March 25, 2026 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add FRE_VERSION to the model yaml and enforce it

5 participants