Initial implementation

[forsyth2]

Initial implementation for zppy-interfaces. Resolves discussion at E3SM-Project/zppy#641. Implements E3SM-Project/zppy#398.

This pull request should be reviewed in conjunction with E3SM-Project/zppy#642.

[forsyth2]

@xylar @chengzhuzhang I have more work to do, but I think I have the main idea of the refactor captured in this pull request and the corresponding E3SM-Project/zppy#642.

I should stress I haven't run this code yet. I still need to get the environments sorted out to be able to call zppy-interfaces from zppy. That is, while I think the main idea is captured here, there is still polishing/testing work left. However, I wanted to tag you here to allow for any further design input early on.

Specifically, my remaining action items:

• Create a conda directory and setup the yaml/toml files necessary to produce a dev environment.
• Use zppy to generate stand-alone ts and mpas_analysis output.
• Run global-time-series interface in the dev environment on that output.
• Run the global_time_series task in zppy (i.e., make sure zppy can correctly call zppy-interfaces).
• Create a tests/ directory and add some integration tests. zppy should simply test that it can invoke zppy-interfaces global-time-series. zppy-interfaces however should test that Global Time Series plots actually generate correctly. As for unit tests, Create global time series Viewers zppy#616 will do that once it's ported to this repo.
• Add pre-commit checks
• Add documentation for this repo/package.

[forsyth2]

I'm following zstash's design here: have one arg parser for the package as a whole (e.g., https://github.com/E3SM-Project/zstash/blob/main/zstash/main.py#L34) and one each for the individual utilities [commands in zstash or interfaces in this package] (e.g., https://github.com/E3SM-Project/zstash/blob/main/zstash/create.py#L103)

The result is that calling the global time series code would be done via:

zppy-interfaces global-time-series <args>

[xylar]

@forsyth2, this seems like a bit of a clunky interface to me. Since the tools don't have much to unify them, I would just given them individual entry points. You could give them a unique prefix like zi-global-time-series if you like.

zstash is a coherent package with clear subcommands similar to git or conda so it makes sense to use this kind of an approach there but I think zppy-interfaces doesn't clearly benefit in the same way.

[xylar]

This is obviously just my opinion. You can stick with the zppy-intefaces main command with subcommands if you like.

[forsyth2]

@xylar If we do completely separate commands, would it make more sense to have distinct dependency requirement lists too? (As opposed to a single dev.yml for all of zppy-interfaces). I'm not sure if that was something we had decided on.

[forsyth2]

I could see several utilities being useful to multiple interfaces -- for example, the OutputViewer class for Global Time Series that E3SM-Project/zppy#616 will introduce will also be useful for adding Viewers to the PMP task in E3SM-Project/zppy#640. It follows that it may be useful to have a single dev environment for all of zppy-interfaces.

[xylar]

@xylar If we do completely separate commands, would it make more sense to have distinct dependency requirement lists too? (As opposed to a single dev.yml for all of zppy-interfaces). I'm not sure if that was something we had decided on.

No, I would just have a single set of dependencies that covers the full package (all commands and any public functions, etc.).

[xylar]

I could see several utilities being useful to multiple interfaces -- for example, the OutputViewer class for Global Time Series that E3SM-Project/zppy#616 will introduce will also be useful for adding Viewers to the PMP task in E3SM-Project/zppy#640. It follows that it may be useful to have a single dev environment for all of zppy-interfaces.

Yes, and there should be a place for a common framework that includes things like this.

[xylar]

I made a few suggestions but this is looking great so far! you'll need some more "glue" like __init__.py and pyproject.toml files for it to actually work but it's coming along very quickly!

[forsyth2]

Thanks @xylar!

[xylar]

A few comments.

[forsyth2]

Notes on latest changes

[forsyth2]

Need to figure out how to import the code properly to the tests.

I've moved many of the tests and the modularized/refactored functions from E3SM-Project/zppy#616 directly into this pull request. Specifically I've moved the changes that are more general refactoring and not specific to Land support or Viewer generation.

[forsyth2]

I do think a cfg would be cleaner, but I want to get this all working properly first before spending time on that.

[forsyth2]

Ok I think we're close to being able to replicate existing functionality using the split-repo approach.

3 issues remaining, noted as part of this review.

[xylar]

@forsyth2, haven't heard anything further on this. Has this just had to move to the back burner or are you stuck anywhere that @altheaden, @tomvothecoder or I could help with?

[forsyth2]

Has this just had to move to the back burner

@xylar Yes, competing priorities at the moment unfortunately. Hoping to get back to this sometime this week.

[xylar]

@forsyth2, it sounds like you should delete whatever installation of mambaforge/miniconda/whatever that you are currently using and start fresh. Download miniforge3:
https://github.com/conda-forge/miniforge?tab=readme-ov-file#miniforge3
Install that in ~/miniforge3 (unless you have a reason to install it somewhere else) and use that from now on. Please update documentation everywhere to refer to miniforge3 rather than either miniconda or mambaforge. Newer versions of conda, like that from miniforge3, will provide you with libmamba as the default package solver, which means there is no added value in using the old (deprecated) mamba command. Solving your environment should be fast.

[xylar]

cd zppy_interfaces/global_time_series
python __main__.py

While that's fine, it should also be possible to use the entry point after you've run pip install .:

zi-global-time-series ...

[forsyth2]

Updating conda build

Previously had this defined in .bashrc to set up conda:

lcrc_conda()
{
  # >>> conda initialize >>>                                                                                                                                                     
  # !! Contents within this block are managed by 'conda init' !!                                                                                                                 
  __conda_setup="$('/home/ac.forsyth2/miniconda3/bin/conda' 'shell.bash' 'hook' 2> /dev/null)"
  if [ $? -eq 0 ]; then
      eval "$__conda_setup"
  else
      if [ -f "/home/ac.forsyth2/miniconda3/etc/profile.d/conda.sh" ]; then
          # . "/home/ac.forsyth2/miniconda3/etc/profile.d/conda.sh"  # commented out by conda initialize                                                                         
          echo ""
      else
          export PATH="/home/ac.forsyth2/miniconda3/bin:$PATH"
      fi
  fi
  unset __conda_setup
  # <<< conda initialize <<<                                                                                                                                                     
}

Deleted that block. Ran source ~/.bashrc

Went to https://github.com/conda-forge/miniforge (full link was giving a security error)

wget "https://github.com/conda-forge/miniforge/releases/latest/download/Miniforge3-$(uname)-$(uname -m).sh"
bash Miniforge3-$(uname)-$(uname -m).sh

Miniforge3 will now be installed into this location:
/home/ac.forsyth2/miniforge3

To activate this environment, use:

    micromamba activate /gpfs/fs1/home/ac.forsyth2/miniforge3

Or to execute a single command in this environment, use:

    micromamba run -p /gpfs/fs1/home/ac.forsyth2/miniforge3 mycommand

installation finished.
Do you wish to update your shell profile to automatically initialize conda?
This will activate conda on startup and change the command prompt when activated.
If you'd prefer that conda's base environment not be activated on startup,
   run the following command when conda is activated:

conda config --set auto_activate_base false

You can undo this by running `conda init --reverse $SHELL`? [yes|no]

You have chosen to not have conda modify your shell scripts at all.
To activate conda's base environment in your current shell session:

eval "$(/home/ac.forsyth2/miniforge3/bin/conda shell.YOUR_SHELL_NAME hook)" 

To install conda's shell functions for easier access, first activate, then:

conda init

Thank you for installing Miniforge3!

eval "$(/home/ac.forsyth2/miniforge3/bin/conda shell.bash hook)"
conda init
# ==> For changes to take effect, close and re-open your current shell. <==

Now, this is at the end of my .bashrc:

# >>> conda initialize >>>                                                                                                                                                       
# !! Contents within this block are managed by 'conda init' !!                                                                                                                   
__conda_setup="$('/gpfs/fs1/home/ac.forsyth2/miniforge3/bin/conda' 'shell.bash' 'hook' 2> /dev/null)"
if [ $? -eq 0 ]; then
    eval "$__conda_setup"
else
    if [ -f "/gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh" ]; then
        . "/gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh"
    else
        export PATH="/gpfs/fs1/home/ac.forsyth2/miniforge3/bin:$PATH"
    fi
fi
unset __conda_setup
# <<< conda initialize <<<  

Wrapped that in a function to call myself rather than having it auto-run:

lcrc_conda()
{
  # >>> conda initialize >>>                                                                                                                                                       
  # !! Contents within this block are managed by 'conda init' !!                                                                                                                   
  __conda_setup="$('/gpfs/fs1/home/ac.forsyth2/miniforge3/bin/conda' 'shell.bash' 'hook' 2> /dev/null)"
  if [ $? -eq 0 ]; then
      eval "$__conda_setup"
  else
      if [ -f "/gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh" ]; then
          . "/gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh"
      else
          export PATH="/gpfs/fs1/home/ac.forsyth2/miniforge3/bin:$PATH"
      fi
  fi
  unset __conda_setup
  # <<< conda initialize <<<  
}

Creating new environments

cd ~/ez/zppy-interfaces
conda clean --all --y
conda env create -f conda/dev.yml -n zppy_interfaces_dev_pr_1_20241121
conda activate zppy_interfaces_dev_pr_1_20241121
pip install .

cd ~/ez/zppy
conda clean --all --y
conda env create -f conda/dev.yml -n zppy_dev_pr_642_20241121
conda activate zppy_dev_pr_642_20241121
pip install .

Testing/debugging

$ zi-global-time-series
usage: zi-global-time-series <args>
zi-global-time-series: error: the following arguments are required: use_ocn, input, input_subdir, moc_file, case_dir, experiment_name, figstr, color, ts_num_years, plots_original, atmosphere_only, plots_atm, plots_ice, plots_lnd, plots_ocn, nrows, ncols, results_dir, regions, start_yr, end_yr

Re: #1 (comment) -- running the entry point requires the parameters be passed in on the command line. (It runs the main function, not the if __name__ == "__main__": block).

Edit zppy/tests/integration/utils.py:

UNIQUE_ID = "test-642-working-env-20241121"

python tests/integration/utils.py
zppy -c tests/integration/generated/test_min_case_global_time_series_original_8_chrysalis.cfg
# Wait for that to finish
cd /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_global_time_series_original_8_output/test-642-working-env-20241121/v3.LR.historical_0051/post/scripts
cat global_time_series_1985-1995.o632171 

That gives:

usage: zi-global-time-series <args>
zi-global-time-series: error: the following arguments are required: end_yr
Copy images to directory
cp: cannot stat '*.pdf': No such file or directory
cp: cannot stat '*.png': No such file or directory

In the bash file:

zi-global-time-series --use_ocn True --global_ts_dir /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_global_time_series_original_8_output/test-642-working-env-20241121/v3.LR.histori\
cal_0051/post/scripts/global_time_series_1985-1995_dir --input /lcrc/group/e3sm2/ac.wlin/E3SMv3/v3.LR.historical_0051 --input_subdir archive/ocn/hist --moc_file mocTimeSeries_1\
985-1995.nc --case_dir /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_global_time_series_original_8_output/test-642-working-env-20241121/v3.LR.historical_0051 --experiment_name v3.\
LR.historical_0051 --figstr v3.LR.historical_0051 --color Blue --ts_num_years 5 --plots_original net_toa_flux_restom,global_surface_air_temperature,toa_radiation,net_atm_energy\
_imbalance,change_ohc,max_moc,change_sea_level,net_atm_water_imbalance --atmosphere_only False --plots_atm None --plots_ice None --plots_lnd None --plots_ocn None --regions glb\
,n,s --results_dir ${results_dir} --start_yr 1985 --end_yr 1995

Re: #1 (comment) -- the \ are a product of my terminal size and are not actually present in the command.

Made some edits in the zppy repo.

cd /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_global_time_series_original_8_output/test-642-working-env-20241121/v3.LR.historical_0051/post/scripts
rm -rf global_time_series_1985-1995* # Allow us to re-run just the global_time_series part
cd /home/ac.forsyth2/ez/zppy
pip install .
python tests/integration/utils.py
zppy -c tests/integration/generated/test_min_case_global_time_series_original_8_chrysalis.cfg

The .o file still gives:

zi-global-time-series: error: the following arguments are required: start_yr, end_yr

Continued various debugging attempts on both the zppy and zppy-interfaces sides. Eventually got:

zi-global-time-series: error: unrecognized arguments: --use_ocn --input --input_subdir --moc_file --case_dir --experiment_name --figstr --color --ts_num_years --plots_original --atmosphere_only --plots_atm --plots_ice --\
plots_lnd --plots_ocn --nrows --ncols --results_dir --regions --start_yr --end_yr

which seems to be the opposite problem.

Looks like the issue is the args actually have to be defined with -- for the arg parser to work.

zppy successfully generating plots by calling zppy-interfaces

Actually got zppy to call zppy-interfaces successfully. Still have the TS error on the zppy-interfaces side though.

After more debugging, was successful in getting plots to show up at https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_min_case_global_time_series_original_8_www/test-642-working-env-20241121/v3.LR.historical_0051/global_time_series/global_time_series_1985-1995_results/

[forsyth2]

Cleaning up the code

Made edits to both zppy and zppy-interfaces (Commit for each is named "Updates").

This included build updates for zppy-interfaces, so let's create a new env:

cd ~/ez/zppy-interfaces
conda clean --all --y
conda env create -f conda/dev.yml -n zppy_interfaces_dev_pr_1_20241121v2
conda activate zppy_interfaces_dev_pr_1_20241121v2
pip install .

Rerun min-case

Rerun the min-case to make sure everything still works:

Edit zppy/tests/integration/utils.py:

UNIQUE_ID = "test-642-working-env-20241121v2"

        "global_time_series_environment_commands": "source /gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh; conda activate zppy_interfaces_dev_pr_1_20241121v2",

python tests/integration/utils.py
pip install .
zppy -c tests/integration/generated/test_min_case_global_time_series_original_8_chrysalis.cfg
# Wait for that to finish
cd /lcrc/group/e3sm/ac.forsyth2/zppy_min_case_global_time_series_original_8_output/test-642-working-env-20241121v2/v3.LR.historical_0051/post/scripts
grep -v "OK" *status

Results at https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_min_case_global_time_series_original_8_www/test-642-working-env-20241121v2/v3.LR.historical_0051/global_time_series/global_time_series_1985-1995_results/

Run unit tests

cd ~/ez/zppy-interfaces
python -u -m unittest tests/global_time_series/test_global_time_series.py

cd ~/ez/zppy
python -u -m unittest tests/test_*.py

test_sections and test_subsections had to be updated.

All unit tests are now passing.

[forsyth2]

I think I have the initial implementation of the zppy/zppy-interfaces refactor done.

[Required reviewers] @xylar @chengzhuzhang I'd like to get approval from both of you before merging this.

[Optional reviewers] @tomvothecoder @altheaden you may want to review the conda/build portions of this PR.

Remaining to-do

After the refactor PRs merge:

• Run zppy's Weekly tests on main to make sure everything is still working properly after the merges.
• Adjust the land support/Viewer generation PR (Create global time series Viewers zppy#616) accordingly
• Help integrate the PMP PR (Workflow for ZPPY- PCMDI for E3SM diagnostics (with fixes on issues found in #624)  zppy#640) accordingly

Add these as GitHub issues to get to later:

• Add documentation, according to https://docs.e3sm.org/zppy/_build/html/main/contributing.html#initial-setup-obsolete-for-reference-only. (Looks like it's better to do that as its own PR) [definitely need to do this before the Unified release!]
• Add option to specify parameters through cfg rather than command line. E.g., https://github.com/E3SM-Project/e3sm_diags/blob/main/e3sm_diags/parser/core_parser.py#L809
• Switch pre-commit checks to ruff, as in https://github.com/xCDAT/xcdat/pull/702/files.
• Rethink how testing is done in zppy & zppy-interfaces. zppy should simply test that it can invoke zppy-interfaces global-time-series. zppy-interfaces however should test that Global Time Series plots actually generate correctly. Maybe have zppy test that zppy is passing in the right parameters and then have zppy-interfaces actually do the image comparisons of the plots, using pre-generated ts and mpas_analysis output.

Helpful details for reviewers

This is actually 2 PRs. This one and E3SM-Project/zppy#642.

zppy

On E3SM-Project/zppy#642:

• Anything under tests/integration/generated can be ignored. Those are just auto-generated testing files.
• I added a global_time_series-specific environment_commands to the test suite, so that zppy-interfaces could be used.
  • This is reflected in the 6 template cfgs (3 of these templates are "min-case" files which I use for debugging, and the other 3 are the big tests we run roughly weekly on main. For this PR, I've been debugging with the min_case_global_time_series_original_8 min-case.)
  • Also reflected in tests/integration/utils.py(This file is used to make run-able cfgs out of the templates).
• I organized zppy/templates to be more logical.
  • There is now a subdirectory zppy/templates/inclusions which is for adding text word-for-word.
  • default.ini and the campaign files are now kept in zppy/defaults.
  • zppy/examples includes files that aren't actually used anywhere and thus serve only as examples.
  • The global_time_series-specific files (coupled_global.py, ocean_month.py, readTS.py) have been moved over to zppy-interfaces/zppy_interfaces/global_time_series, out of the zppy repo completely. Note, the readTS.py file has been included into coupled_global.py and other utilities have been pulled out into a new utils.py.
  • The jinja2 template bash scripts remain in zppy/templates itself.
  • These directory changes can be seen throughout the PR, including in the unit test changes.
• Much of zppy/templates/global_time_series.bash has been moved over to zppy_interfaces/global_time_series/__main__.py as python code rather than bash code.

zppy-interfaces

This PR

• Conda/build updates: .flake8, .pre-commit-config.yaml, conda/dev.yml, pyproject.toml, zppy_interfaces/version.py
• I noticed many changes in Create global time series Viewers zppy#616 were actually more general and not specifically about Land support and/or Viewer generation. I've pulled out those changes (notably refactoring of coupled_global.py into more concise, composable functions & testing those functions in unit tests)
  • Unit test: tests/global_time_series/test_global_time_series.py
  • zppy_interfaces/global_time_series/coupled_global.py and zppy_interfaces/global_time_series/utils.py reflect these changes.
• The only entry point right now is zi-global-time-series which runs zppy_interfaces/global_time_series/__main__.py main.
  • Please note that this modifies the case directory (only the posts/ subdirectory though), which could be seen as modifying the input. However, this was the case before the refactor as well. Pre-refactor, the case dir was defined to be {{ output }}, so we knew we were modifying the directory the user specified as output (which they could have set to be identical to {{ input }}). Now, we sort of assume output dir = input dir = case dir, relying on zppy to pass in the right output dir.
  • The if __name__ == "__main__": block of that file is for debugging manually, and is not called by zppy.

[tomvothecoder]

My brief review of the DevOps related files with some comments and suggestions.

I recommend setting up a GH Actions build workflow similar to zppy that runs the pre-commit hooks and unit tests: https://github.com/E3SM-Project/zppy/blob/main/.github/workflows/build_workflow.yml

[tomvothecoder]

Suggested change

[forsyth2]

Thanks @tomvothecoder! I've implemented your suggestions on both PRs.

Details of testing

cd ~/ez/zppy-interfaces
conda clean --all --y
conda env create -f conda/dev.yml -n zppy_interfaces_dev_pr_1_20241121v4
conda activate zppy_interfaces_dev_pr_1_20241121v4
pip install .
pytest tests/global_time_series

cd ~/ez/zppy
conda clean --all --y
conda env create -f conda/dev.yml -n zppy_dev_pr_642_20241121v4
conda activate zppy_dev_pr_642_20241121v4
pip install .
pytest tests/test*.py # Skip integration test directory

That gives a warning -- one we're already escaping -- but all tests pass

===================================================================================================== warnings summary ======================================================================================================
zppy/utils.py:191
  /gpfs/fs1/home/ac.forsyth2/ez/zppy/zppy/utils.py:191: DeprecationWarning: invalid escape sequence \.
    tmp = re.sub("\.[^.]*\.nc$", "", tmp)  # noqa: W605

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=============================================================================================== 23 passed, 1 warning in 0.20s ==============================================================================================

# Now, run min case
# Edit tests/integration/utils.py
# UNIQUE_ID = "test-642-working-env-20241121v4"
# "global_time_series_environment_commands": "source /gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh; conda activate zppy_interfaces_dev_pr_1_20241121v4",
python tests/integration/utils.py
zppy -c tests/integration/generated/test_min_case_global_time_series_original_8_chrysalis.cfg

Plots are generated at https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_min_case_global_time_series_original_8_www/test-642-working-env-20241121v4/v3.LR.historical_0051/global_time_series/global_time_series_1985-1995_results/.

After some edits to the build_workflow.yml files I was able to get checks passing on E3SM-Project/zppy#642 and on this PR.

[xylar]

@forsyth2, this looks great! On small suggested change.

[forsyth2]

Thanks for the reviews @xylar!

[chengzhuzhang]

Hi @forsyth2 this is exciting change. Thanks for making it happening. I think @xylar and @tomvothecoder had solid reviews for DevOp codes. and things looks promising. I'm approving this PR, it addresses the initial implementation.

Regarding to usability, I'm wondering if it is general enough, for instance, does zi-global-time-series support land only simulation with easy syntax? Maybe the coupled_global.py can be polished a bit. Anyway, these can be addressed in a future PR.

[forsyth2]

Thanks @chengzhuzhang!

Regarding to usability, I'm wondering if it is general enough, for instance, does zi-global-time-series support land only simulation with easy syntax? Maybe the coupled_global.py can be polished a bit. Anyway, these can be addressed in a future PR.

• Create global time series Viewers zppy#616 will take care of adding Land support.
• The issue (which I still need to create) for using a cfg rather than command line options will go a long way towards generalized usability.

[chengzhuzhang]

• The issue (which I still need to create) for using a cfg rather than command line options will go a long way towards generalized usability.

Thank you @forsyth2 , command line options are good to have as well, just we need to think through all the options and make them intuitive to use.

[forsyth2]

I added the issues mentioned in #1 (review): #2, #3, #4, #5. I also redid the issue labels (the color tags) to match zppy's labels.

[forsyth2]

It looks like any remaining comments/concerns can be addressed in later pull requests, so I think it's safe to say we have the initial implementation done. I'm going to merge this PR and E3SM-Project/zppy#642.

The plan after that is:

• Run zppy's Weekly tests on main to make sure everything is still working properly after the merges.
• Adjust the land support/Viewer generation PR (Create global time series Viewers zppy#616) accordingly
• Help integrate the PMP PR (Workflow for ZPPY- PCMDI for E3SM diagnostics (with fixes on issues found in #624)  zppy#640) accordingly -- I have a meeting scheduled to discuss this.
• The new zppy-interfaces issues I mentioned above.