Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMORgasbord: yaml parsing #368

Merged
merged 34 commits into from
Mar 3, 2025
Merged

CMORgasbord: yaml parsing #368

merged 34 commits into from
Mar 3, 2025

Conversation

ilaflott
Copy link
Member

@ilaflott ilaflott commented Feb 25, 2025

Background

PR #363 was/is the pre-amble to this PR. This PR seeks to close issue #320, and make general improvements related to that as-needed.

Describe your changes

expanding this as i go. Currently we have:

  • expands fre cmor yaml call input arguments
  • takes the widely used join_constructor and puts it in fre/yamltools/constructors.py, to assist with avoiding circular import issues i encountered in PR 320.cmor yaml parsing #363, remove duplication of that code elsewhere
  • takes other widely used routines (output_yaml, yaml_load) and puts them in fre/yamltools/helpers.py, removed duplicates where applicable
  • centralize the way the yaml module gets imported and gets the !join constructor' inside fre/yamltools/__init__.py.
  • make everything that needs the yaml module with join_constructor import it via yamltools/__init__.py
  • leave commented-out logging efforts in fre/list_, since, in that case, we kind of WANT the stdout... so should ponder an intelligent work around and update the unittests for fre list in near future
  • DEBUG_RUN_ONE_MODE now a proper click flag option for fre cmor run/yaml calls, which should close CMORgasbord: make current DEBUG_MODE_RUN_ONE flag an optional arg to fre cmor run #324 ! called run_one_mode in CLI calls, and run_one within fre/cmor internals
  • function calls now "do the right thing" with that flag
  • relevant unit-test calls adjusted to use the new run_one_mode flag
  • new unit test, fre/tests/test_fre_yamltools_cli.py::test_cli_fre_yamltools_combine_cmoryaml
  • new unit test, fre/yamltools/tests/test_combine_yamls_script.py::test_combine_cmor_yaml
  • new test-data file, fre/yamltools/tests/AM5_example/cmor_yamls/cmor.am5.yaml
  • new test-data file for comparison to aforementioned unittests, fre/yamltools/tests/AM5_example/COMPARE_TEST_OUTPUT_cmor.yaml
  • new "info parser" holding a CMORYaml class definition, fre/yamltools/cmor_info_parser.py, which get leveraged for e.g. combining an un-parsed model yaml (AM5_example/am5.yaml) with a model-specific cmor-tool-yaml (AM5_example/cmor_yamls/cmor.am5.yaml)

Issue ticket number and link (if applicable)

#320 and via sidequest, #324

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
    • caveat here is fre/list_ things i touched: need to consider how we handle fre list stdout

@ilaflott ilaflott changed the title init commit, smth small, to start draft PR cmor yaml parsing Feb 25, 2025
ilaflott and others added 11 commits February 25, 2025 14:55
return statement included before calling the run subtool- pprint out to screen instead
forgot the yamltools bit
turn on run-one-debug mode at top of testing script
fix minor syntax error causing all the failures
fix the import statements.
…ust import statements when join_constructor pops up
…verything in yamltools can just from . import * it. remove the (yaml)constructor arguments to yaml class constructors, remove the add_constructor bit from initializing since we get it by importing the yamltools directory now. remove dependence on combine_yamls when things like load_yaml or yaml_output are called- put these in helper and adjust statements everywhere where required. make attempt at sticking fre_logger into fre list calls- but actually this may need some thought about what we want since we are parsing std out for the unit tests...
@ilaflott
Copy link
Member Author

ilaflott commented Feb 26, 2025

OK! at this point, i did some cleaning up and refactoring to find my way better around yamltools as necessary for fre cmor yaml efforts. and I did so preserving the test outcomes- yay!

now, lets get back to the real work where PR #363 (R.I.P.) was getting interesting, which was the following call

@ilaflott
Copy link
Member Author

...and that call was...

> fre -v yamltools combine-yamls -y fre/yamltools/tests/AM5_example/am5.yaml -e c96L65_am5f7b12r1_amip -p ncrc5.intel -t prod-openmp --use cmor --output FOO_cmor.yaml

... which produces

INFO:freyamltools.py:combine_yamls calling fre/yamltools/combine_yamls_script.py::consolidate_yamls
INFO:combine_yamls_script.py:consolidate_yamls combining+parsing yaml files, for cmor usage
INFO:combine_yamls_script.py:consolidate_yamls about to initialize the CmorYaml
INFO:cmor_info_parser.py:__init__ initializing a CMORYaml object
INFO:cmor_info_parser.py:__init__ CMORYaml initialized!
INFO:combine_yamls_script.py:consolidate_yamls 
    DONE initializing CmorYaml = 
    InitCMORYaml( 
                               yml = fre/yamltools/tests/AM5_example/am5.yaml 
                               name = c96L65_am5f7b12r1_amip 
                               platform = ncrc5.intel 
                               target = prod-openmp 
                               mainyaml_dir = fre/yamltools/tests/AM5_example

INFO:combine_yamls_script.py:consolidate_yamls will be writing out the combined yaml dictionary!
INFO:combine_yamls_script.py:get_combined_cmoryaml calling CmorYaml.combine_model() for yaml_content and loaded_yaml
INFO:cmor_info_parser.py:combine_model    model yaml: fre/yamltools/tests/AM5_example/am5.yaml
INFO:cmor_info_parser.py:combine_experiment ey_path = fre/yamltools/tests/AM5_example/cmor_yamls/cmor.am5.yaml
INFO:cmor_info_parser.py:merge_cmor_yaml    experiment yaml: cmor.am5.yaml
{ 'build': { 'compileYaml': 'compile_yamls/compile.yaml',
             'platformYaml': 'compile_yamls/platforms.yaml'},
  'cmor': { 'directories': { 'analysis_dir': '/nbhome/$USER/am5/am5f7b12r1/c96L65_am5f7b12r1_amip',
                             'exp_config_dir': 'fre/tests/test_files',
                             'fre_analysis_home': '/home/fms/local/opt/fre-analysis/test',
                             'history_dir': '/archive/$USER/am5/am5f7b12r1/c96L65_am5f7b12r1_amip/ncrc5.intel-prod-openmp/history',
                             'pp_dir': '/archive/$USER/am5/am5f7b12r1/c96L65_am5f7b12r1_amip/ncrc5.intel-prod-openmp/pp',
                             'ptmp_dir': '/xtmp/$USER/ptmp',
                             'table_dir': 'fre/tests/test_files/cmip6-cmor-tables/Tables'},
            'targets': { 'Omon-ts-gr1': { 'indir': 'fre/tests/test_files/ocean_sos_var_file',
                                          'json_exp_config': 'fre/tests/test_files/CMOR_input_example.json',
                                          'json_table_config': 'fre/tests/test_files/cmip6-cmor-tables/Tables/CMIP6_Omon.json',
                                          'json_var_list': 'fre/tests/test_files/varlist',
                                          'opt_var_name': None,
                                          'outdir': 'fre/tests/test_files/outdir'}}},
  'name': 'c96L65_am5f7b12r1_amip',
  'platform': 'ncrc5.intel',
  'target': 'prod-openmp'}

but more importantly, the contents of that yaml were, via cat FOO_cmor.yaml:

name: c96L65_am5f7b12r1_amip
platform: ncrc5.intel
target: prod-openmp
build:
  compileYaml: compile_yamls/compile.yaml
  platformYaml: compile_yamls/platforms.yaml
cmor:
  directories:
    history_dir: /archive/$USER/am5/am5f7b12r1/c96L65_am5f7b12r1_amip/ncrc5.intel-prod-openmp/history
    pp_dir: /archive/$USER/am5/am5f7b12r1/c96L65_am5f7b12r1_amip/ncrc5.intel-prod-openmp/pp
    analysis_dir: /nbhome/$USER/am5/am5f7b12r1/c96L65_am5f7b12r1_amip
    ptmp_dir: /xtmp/$USER/ptmp
    fre_analysis_home: /home/fms/local/opt/fre-analysis/test
    table_dir: fre/tests/test_files/cmip6-cmor-tables/Tables
    exp_config_dir: fre/tests/test_files
  targets:
    Omon-ts-gr1:
      json_table_config: fre/tests/test_files/cmip6-cmor-tables/Tables/CMIP6_Omon.json
      json_exp_config: fre/tests/test_files/CMOR_input_example.json
      indir: fre/tests/test_files/ocean_sos_var_file
      json_var_list: fre/tests/test_files/varlist
      outdir: fre/tests/test_files/outdir
      opt_var_name: null

so if we can get back here, preserving current testing results, we can definitely move forward with the approach

@ilaflott ilaflott changed the title cmor yaml parsing CMORgasbord: yaml parsing Feb 26, 2025
…flag, default false. add the debug run one mode to the yaml processing calls as well.
…his red x into a green check. will adjust testing/config file needs as we go along to ensure we are writing sensible things
…regious assumption tha the gfdl name is going to be the mip name. i know this is not the case- just a prototype for possibly interfacing with slurm.
@ilaflott
Copy link
Member Author

yay!

(fre-cli) an210: fre-cli $] fre -v yamltools combine-yamls -y fre/yamltools/tests/AM5_example/am5.yaml -e c96L65_am5f7b12r1_amip -p ncrc5.intel -t prod-openmp --use cmor --output FOO_cmor.yaml
INFO:cmor_info_parser.py:__init__ initializing a CMORYaml object
INFO:cmor_info_parser.py:__init__ CMORYaml initialized!
INFO:combine_yamls_script.py:get_combined_cmoryaml calling CMORYaml.combine_model() for yaml_content and loaded_yaml
INFO:cmor_info_parser.py:combine_model    model yaml: fre/yamltools/tests/AM5_example/am5.yaml
INFO:cmor_info_parser.py:combine_experiment ey_path = fre/yamltools/tests/AM5_example/cmor_yamls/cmor.am5.yaml
INFO:cmor_info_parser.py:merge_cmor_yaml    experiment yaml: cmor.am5.yaml
INFO:combine_yamls_script.py:get_combined_cmoryaml Combined cmor-yaml information cleaned+saved as dictionary
INFO:combine_yamls_script.py:get_combined_cmoryaml Combined cmor-yaml information saved to FOO_cmor.yaml
(fre-cli) an210: fre-cli $] cat FOO_cmor.yaml 
name: c96L65_am5f7b12r1_amip
platform: ncrc5.intel
target: prod-openmp
build:
  compileYaml: compile_yamls/compile.yaml
  platformYaml: compile_yamls/platforms.yaml
cmor:
  directories:
    history_dir: /archive/$USER/am5/am5f7b12r1/c96L65_am5f7b12r1_amip/ncrc5.intel-prod-openmp/history
    pp_dir: /archive/$USER/am5/am5f7b12r1/c96L65_am5f7b12r1_amip/ncrc5.intel-prod-openmp/pp
    analysis_dir: /nbhome/$USER/am5/am5f7b12r1/c96L65_am5f7b12r1_amip
    ptmp_dir: /xtmp/$USER/ptmp
    fre_analysis_home: /home/fms/local/opt/fre-analysis/test
    table_dir: fre/tests/test_files/cmip6-cmor-tables/Tables
    exp_config_dir: fre/tests/test_files
  targets:
    Omon-ts-gr1:
      json_table_config: fre/tests/test_files/cmip6-cmor-tables/Tables/CMIP6_Omon.json
      json_exp_config: fre/tests/test_files/CMOR_input_example.json
      indir: fre/tests/test_files/ocean_sos_var_file
      json_var_list: fre/tests/test_files/varlist
      outdir: fre/tests/test_files/outdir
      opt_var_name: null

…e unit tests into existence and add in test case ive been running based on the am5 yaml etc. the contents of the yaml too must be checked
…ine-yaml test call

add comparison conditions for lloading the fresh created output file and comparing to static test file data
…ot to test for the combination doing its job. test the combine_yamls as a python module call, and if the output combined yaml exists already, remove it before recreating
@ilaflott ilaflott marked this pull request as ready for review February 27, 2025 21:24
@ilaflott ilaflott self-assigned this Feb 27, 2025
@ilaflott ilaflott added new functioning New feature or request priority: HIGH labels Feb 27, 2025
Copy link

@chanwilson chanwilson left a comment

Choose a reason for hiding this comment

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

Nothing showstopper just avoiding technical debt

@ilaflott ilaflott requested a review from chanwilson February 28, 2025 21:37
Copy link
Collaborator

@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.

Looks great! @singhd789 there are a few yamls-related touches in here that you're probably already aware of (helpers.py)

from . import *


def experiment_check(mainyaml_dir, experiment, loaded_yaml):
Copy link
Collaborator

Choose a reason for hiding this comment

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

There seem to be some shared functions in here and in pp_info_parser.py. These could probably eventually be cleaned and put in one script to use (like experiment_check, combine_model, combine_experiment) - maybe in an abstract class you mentioned to me one time. This could probably be a clean-up in the aftermath of CMIP tasks though

Copy link
Member Author

Choose a reason for hiding this comment

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

100% yes

@singhd789 singhd789 dismissed chanwilson’s stale review March 3, 2025 16:26

Ian told me to dismiss this, keep momentum going!!

@ilaflott
Copy link
Member Author

ilaflott commented Mar 3, 2025

I told @singhd789 to dismiss @chanwilson to keep the stuff moving!

@ilaflott ilaflott merged commit bb6f681 into main Mar 3, 2025
2 checks passed
@ilaflott ilaflott deleted the 320.cmor_yaml_redux branch March 3, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new functioning New feature or request priority: HIGH
Projects
None yet
4 participants