Remove deprecated parameters

[forsyth2]

Issue resolution

• Closes Clean up code for v3.0.0 #653. Associated with Remove deprecated parameters zppy-interfaces#11.

Select one: This pull request is...

• a bug fix: increment the patch version
• a small improvement: increment the minor version
• an incompatible (non-backwards compatible) API change: increment the major version

1. Does this do what we want it to do?

Objectives:

• Objective 1
• Objective 2
• ...
• Objective n

Required:

• Product Management: I have confirmed with the stakeholders that the objectives above are correct and complete.
• Testing: I have added or modified at least one "min-case" configuration file to test this change. Every objective above is represented in at least one cfg.
• Testing: I have considered likely and/or severe edge cases and have included them in testing.

If applicable:

• Testing: this pull request introduces an important feature or bug fix that we must test often. I have updated the weekly-test configuration files, not just a "min-case" one.
• Testing: this pull request adds at least one new possible parameter to the cfg. I have tested using this parameter with and without any other parameter that may interact with it.

2. Are the implementation details accurate & efficient?

Required:

• Logic: I have visually inspected the entire pull request myself.
• Logic: I have left GitHub comments highlighting important pieces of code logic. I have had these code blocks reviewed by at least one other team member.

If applicable:

• Dependencies: This pull request introduces a new dependency. I have discussed this requirement with at least one other team member. The dependency is noted in zppy/conda, not just an import statement.

3. Is this well documented?

Required:

• Documentation: by looking at the docs, a new user could easily understand the functionality introduced by this pull request.

4. Is this code clean?

Required:

• Readability: The code is as simple as possible and well-commented, such that a new team member could understand what's happening.
• Pre-commit checks: All the pre-commits checks have passed.

If applicable:

• Software architecture: I have discussed relevant trade-offs in design decisions with at least one other team member. It is unlikely that this pull request will increase tech debt.

[forsyth2]

Possible text for v3.0.0 release notes:

zppy v3.0.0 introduces several "breaking" changes, hence the increment of the major version.

Most importantly, the e3sm_to_cmip functionality has been moved out of the ts task into its own task. (#650). So, any users who have been using ts_fmt="cmip" will have to update their zppy cfg files. That primarily affects current ILAMB users. Parameter changes are noted below. More details can be found in the parameter documentation on default.ini.

• No longer used anywhere and thus removed: e3sm_to_cmip_environment_commands, ts_fmt
• Use this in the e3sm_to_cmip task rather than the ts task: cmip_metadata
• New parameters for the e3sm_to_cmip task: cmip_vars, ts_grid, ts_subsection
• New parameters for the ilamb task: e3sm_to_cmip_atm_subsection, e3sm_to_cmip_land_subsection

As part of the update to v3.0.0, we also removed several deprecated parameters unrelated to e3sm_to_cmip and ilamb:

• The scratch parameter, which had previously been used in the tc_analysis task but is no longer used in any form.
• The atmosphere_only parameter in the global_time_series task. Now, simply remove the 3 ocean plots (change_ohc,max_moc,change_sea_level) from plots_original.
• The plot_names parameter in the global_time_series task. Now, use plots_original.

[forsyth2]

Ok, it looks like nothing broke according to my testing

Testing steps

cd ~/ez/zppy-interfaces
git status
git log
# Last commit, from 12/18: Remove deprecated parameters
conda clean --all --y
conda env create -f conda/dev.yml -n zi_dev_pr11
conda activate zi_dev_pr11
pip install .
pytest tests/unit/global_time_series/test_*.py
# 7 passed in 22.20s

# Skip e3sm_diags setup

cd ~/ez/zppy
conda env create -f conda/dev.yml -n zppy_dev_pr654
conda activate zppy_dev_pt654
pip install .
pytest tests/test_*.py
# 1 failed, 22 passed in 0.48s

# Needed to remove plot_names and atmosphere_only from tests...
pytest tests/test_*.py
# 23 passed in 0.55s

# Edit tests/integration/utils.py:
# UNIQUE_ID = "test_zppy_pr654_20241218"
# For get_chyrsalis_expansions:
#        "global_time_series_environment_commands": "source /gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh; conda activate zi_dev_pr11",
# Keep this as is:     generate_cfgs(unified_testing=False, dry_run=False)
python tests/integration/utils.py

# Update tests/integration/test_weekly.py:
# subdirs_to_check = ["mpas_analysis", "global_time_series"]
# Skip the bundles tests
# Also set non-required tasks in v2 and v3 cfgs to `active = False`
python tests/integration/utils.py 

zppy -c tests/integration/generated/test_weekly_comprehensive_v3_chrysalis.cfg
zppy -c tests/integration/generated/test_weekly_comprehensive_v2_chrysalis.cfg

cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v3_output/test_zppy_pr654_20241218/v3.LR.historical_0051/post/scripts/
grep -v "OK" *status
# No errors
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v2_output/test_zppy_pr654_20241218/v2.LR.historical_0201/post/scripts
grep -v "OK" *status
# No errors

# Run integration tests
cd ~/ez/zppy
pytest tests/integration/test_weekly.py
# 2 passed, 3 skipped in 179.14s (0:02:59)

[forsyth2]

@chengzhuzhang What are your thoughts on my comment above? In particular:

• Is there anything else about the zppy API we want to change (besides the e3sm_to_cmip task and removing these deprecated parameters)? Now is a perfect opportunity to change those things because we'll be incrementing the major version anyway.
  • Conversely, one could also argue we shouldn't merge the changes in this PR because they aren't strictly necessary. That is, removing these deprecated parameters simply cleans up the code whereas the e3sm_to_cmip transition (Add e3sm_to_cmip task #650) fundamentally altered the API. (i.e., if we merge these changes global_time_series users will also need to update their cfgs because the code will no longer be backwards-compatible. If we don't, then only ilamb users will be affected by the jump in major version.)

I tested these changes and it looks like everything works out alright without these parameters. If you want to review yourself, it's this PR & E3SM-Project/zppy-interfaces#11.

[forsyth2]

@xylar @tomvothecoder Do either of you have thoughts on the decision I described above? I.e., should a major version change 1) modernize the codebase, removing all deprecated things or 2) only remove things completely incompatible with the new approach/feature?

[chengzhuzhang]

I'm a little unsure to remove this parameter, before the test tc_analysis tests can pass on Perlmutter, let's keep your change now, and can decide latter if removing this parameter is okay, after Perlmutter change.

[forsyth2]

This is just the default value for scratch. The actual use of scratch was removed in the result_dir={{ scratch }}/tc-analysis_${Y1}_${Y2}/ -> result_dir={{ output }}/post/atm/tc-analysis_${Y1}_${Y2}/ change (see https://github.com/E3SM-Project/zppy/pull/632/files#r1878679038).

Are you saying 1) undo that change or 2) just hold off on removing scratch as a parameter completely until we make sure that change works on Perlmutter?

[chengzhuzhang]

I'm wondering if we can test tc_analysis on Perl-mutter to make sure, it works without needing to including intermediate output underscratch file system? If so, we are free to remove scratch parameter and the codes what supports this.

[forsyth2]

Yes, we can do this. I was holding off until the rc testing period, because I need to copy over all the test data, etc. to Perlmutter (and Compy). But I can try to do that earlier.

[chengzhuzhang]

This would be great, thank you!

[chengzhuzhang]

@forsyth2 I was nervous to make the change more abrupt, but after reviewing the code, I can see the parameter space is much cleaner. I think it is good to have this PR for v3. Does the parameter changes for global_time_series affect users who only produce original 8 panels plot?

[forsyth2]

Does the parameter changes for global_time_series affect users who only produce original 8 panels plot?

Correct. atmosphere_only and plot_names were used exclusively for the original 8-panel plot. Once global-time-series started getting more customizable, other parameters proved more useful.

[chengzhuzhang]

Does the parameter changes for global_time_series affect users who only produce original 8 panels plot?

Correct. atmosphere_only and plot_names were used exclusively for the original 8-panel plot. Once global-time-series started getting more customizable, other parameters proved more useful.

By searching through the zppy .cfg files in https://github.com/E3SM-Project/SimulationScripts, It looks like atmosphere_only is used pretty widely now. In this case, if we remove this parameter we need to make sure to let users know this breaking change as well.

[tomvothecoder]

@xylar @tomvothecoder Do either of you have thoughts on the decision I described above? I.e., should a major version change 1) modernize the codebase, removing all deprecated things or 2) only remove things completely incompatible with the new approach/feature?

I think it makes sense to modernize the code by removing all deprecated elements. Since removing deprecated code is a breaking change, it can be bundled with other breaking changes in the same release. This will help reduce technical debt in the long run.

The downside in the short term is the upfront effort required to remove the deprecated code, but I believe it's well worth the investment.

[xylar]

Sorry @forsyth2, I missed the question. I completely agree with @tomvothecoder.

[forsyth2]

@tomvothecoder @xylar Thanks! That sounds good to me; we just need to make sure we alert users of the breaking changes relevant to them.