Enable CDAT-migrated E3SM Diags #651
Conversation
|
To do:
|
| output = "#expand user_output#zppy_weekly_comprehensive_v3_output/#expand unique_id#/#expand case_name#" | ||
| partition = "#expand partition_short#" | ||
| qos = "#expand qos_short#" | ||
| www = "#expand user_www#zppy_weekly_comprehensive_v3_www/#expand unique_id#" | ||
| years = "1985:1989:2", |
There was a problem hiding this comment.
output and www need updated names!
|
Currently running full weekly test suite (doing this pre-merge because the CDAT migration was a large change, so there's a higher chance of breaking zppy -c tests/integration/generated/test_weekly_comprehensive_v3_chrysalis.cfg
zppy -c tests/integration/generated/test_weekly_comprehensive_v2_chrysalis.cfg
zppy -c tests/integration/generated/test_weekly_bundles_chrysalis.cfg # Runs 1st part of bundles cfg |
Continuing testing from previous commentContinuing testing gives: zppy -c tests/integration/generated/test_weekly_bundles_chrysalis.cfg # Runs 2nd part of bundles cfg
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v3_output/test-346-20241210/v3.LR.historical_0051/post/scripts/
grep -v "OK" *status
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v2_output/test-346-20241210/v2.LR.historical_0201/post/scripts
grep -v "OK" *status
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_bundles_output/test-346-20241210/v3.LR.historical_0051/post/scripts
grep -v "OK" *status
# Run integration tests
cd ~/ez/zppy
pytest tests/integration/test_*.pyThat gives: Bash generation testThe Image check failuresThere's a large number of image check diffs in subdirectories of:
But as far as I can tell, the errors are mostly benign. Example errors:
Some errors seem more concerning: Conclusions@chengzhuzhang @tomvothecoder What's the best path forward here? None of the Do we accept these diffs, merge this PR, and update the expected images? Or do these need to be investigated more thoroughly? |
I think we should expect that most of the differences will be benign. The underlying results from the dev branch won't exactly match the CDAT codebase, which can produce diffs. However, any formatting differences (e.g., labels for axes) or major differences like the one you mentioned above should be investigated. I will open a separate GitHub issue in Afterwards, we can update the expected results and you can re-run the |
|
Thanks @tomvothecoder! |
|
zppy & post-CDAT-migration e3sm_diags to-do list, for the 1/15 Unified release-candidate deadline:
@tomvothecoder @chengzhuzhang ^FYI for timeline planning |
|
@forsyth2 I updated your checklist above. I just merged E3SM-Project/e3sm_diags#907 and you should be able to re-test now. Hopefully this fixes the diffs in |
|
@tomvothecoder Thanks! E3SM-Project/e3sm_diags#907 has extensive changes, so I will integrate that into |
forsyth2
force-pushed
the
issue-346-diags-post-refactor
branch
3 times, most recently
from
84a2b59 to
c2188bf
Compare
|
Since the zppy release was delayed anyway yesterday (from the issues described on #634 (reply in thread)), I tried testing this pull request using the latest E3SM Diags (E3SM-Project/e3sm_diags#907), however I ran into a number of issues. # Set up branch
cd ~/ez/zppy
git checkout issue-346-diags-post-refactor
# Set up and test zppy-interfaces env
# zi_dev_weekly_20250114 still exists from weekly testing(That environment was created in #634 (comment)) (These actually appear to be from earlier runs. The MPAS-Analysis errors are noted starting with #634 (reply in thread)). |
|
@forsyth2 thank you for keep testing. For
|
|
Testing with CDAT-migrated E3SM Diags for Testing summaryv2 test:
v3 test:
*Address the issue of file paths (latest comment on that: E3SM-Project/e3sm_diags#866 (comment)) Note: the missing images don't show up as image diffs, like the mismatched images do. Missing images mean we expected that image (it's in the expected results), but there was no actual to diff against. There's quite a few errors of both types (missing, mismatched) to consider... Testing detailsTesting steps# Set up and test zppy-interfaces env
cd ~/ez/zppy-interfaces
git status
# Check no file changes will persist when we switch branches.
git fetch upstream main
git checkout -b test_zi_weekly_20250117 upstream/main
git log
# Last commit, from 1/17: Further PR template updates (#15)
conda clean --all --y
conda env create -f conda/dev.yml -n zi_dev_weekly_20250117
conda activate zi_dev_weekly_20250117
pip install .
pytest tests/unit/global_time_series/test_*.py
# 10 passed in 24.69s
# Set up e3sm_diags env
cd ~/ez/e3sm_diags
git status
# Check no file changes will persist when we switch branches.
git fetch upstream
git checkout -b main upstream main
git reset --hard upstream/main
git log
# Confirmed last commit, from 1/17: Revert "Drop Python 3.9 support (#919)"
conda clean --all --y
conda env create -f conda-env/dev.yml -n e3sm_diags_20250117
conda activate e3sm_diags_20250117
pip install .With both commits:# Set up branches
cd ~/ez/zppy
git status
# Check no file changes will persist when we switch branches.
git checkout issue-346-diags-post-refactor
git log
# b90506f7d18b77d41a4a74b5beff413bb6f1fe2b => 1/14 Update PR template (#658)
git fetch upstream
git rebase -i
# Drop the testing commit
# 2 commits:
# Remove symbolic links for ts
# Enable CDAT-migrated E3SM Diags
# Now rebased on 1/17: Further PR template updates (#662)
# Set up zppy env
conda clean --all --y
conda env create -f conda/dev.yml -n zppy_dev_pr651_both_commits_20250117
conda activate zppy_dev_pr651_both_commits_20250117
pip install .
# Unit tests for zppy
pytest tests/test_*.py
# 23 passed in 0.29s
# Integration testing for zppy
# Edit tests/integration/utils.py:
# UNIQUE_ID = "test_pr651_both_commits_20250117"
# "diags_environment_commands": "source /gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh; conda activate e3sm_diags_20250117",
# "global_time_series_environment_commands": "source /gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh; conda activate zi_dev_weekly_20250117",
# Keep this as-is: generate_cfgs(unified_testing=False, dry_run=False)
# Run zppy to produce actual results to compare in the integration tests
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_pr651_both_commits_20250117/v3.LR.historical_0051/post/scripts/
grep -v "OK" *status
# No errors
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v2_output/test_pr651_both_commits_20250117/v2.LR.historical_0201/post/scripts
grep -v "OK" *status
# No errors
cd ~/ez/zppy
git checkout issue-346-diags-post-refactor
git branch # Check that we're on issue-346-diags-post-refactor
ls tests/integration/test_*.py
pytest tests/integration/test_bash_generation.py
# 1 failed in 2.15s
# Diffs are expected
pytest tests/integration/test_campaign.py
# 6 passed in 1.93s
pytest tests/integration/test_defaults.py
# 1 passed in 0.32s
pytest tests/integration/test_last_year.py
# 1 passed in 0.26s
pytest tests/integration/test_weekly.py # Skip bundles tests for now
# FAILED tests/integration/test_weekly.py::test_comprehensive_v2_images - AssertionError
# FAILED tests/integration/test_weekly.py::test_comprehensive_v3_images - AssertionError
# 2 failed, 3 skipped in 1351.74s (0:22:31)
# For v2:
# Total number of images checked: 4376
# Missing images: 81
# For v3:
# Total number of images checked: 5216
# Missing images: 102With first commit only:# Set up branches
cd ~/ez/zppy
git status
# Check no file changes will persist when we switch branches.
git checkout issue-346-diags-post-refactor
git checkout -b issue-346-diags-post-refactor-1st-commit
git fetch upstream
git rebase -i upstream/main
# Keep only the 1st commit:
# Enable CDAT-migrated E3SM Diags
# Now rebased on 1/17: Further PR template updates (#662)
# Set up zppy env
conda clean --all --y
conda env create -f conda/dev.yml -n zppy_dev_pr651_1st_commit_20250117
conda activate zppy_dev_pr651_1st_commit_20250117
pip install .
# Unit tests for zppy
pytest tests/test_*.py
# 23 passed in 0.23s
# Integration testing for zppy
# Edit tests/integration/utils.py:
# UNIQUE_ID = "test_pr651_1st_commit_20250117"
# "diags_environment_commands": "source /gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh; conda activate e3sm_diags_20250117",
# "global_time_series_environment_commands": "source /gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh; conda activate zi_dev_weekly_20250117",
# Keep this as-is: generate_cfgs(unified_testing=False, dry_run=False)
# Run zppy to produce actual results to compare in the integration tests
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_pr651_1st_commit_20250117/v3.LR.historical_0051/post/scripts/
grep -v "OK" *status
# No errors
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v2_output/test_pr651_1st_commit_20250117/v2.LR.historical_0201/post/scripts
grep -v "OK" *status
# No errors
cd ~/ez/zppy
git branch # Check that we're on issue-346-diags-post-refactor-1st-commit
ls tests/integration/test_*.py
pytest tests/integration/test_bash_generation.py
# 1 failed in 2.49s
# Diffs are expected
pytest tests/integration/test_campaign.py
# 6 passed in 1.78s
pytest tests/integration/test_defaults.py
# 1 passed in 0.33s
pytest tests/integration/test_last_year.py
# 1 passed in 0.25s
pytest tests/integration/test_weekly.py # Skip bundles tests for now
# FAILED tests/integration/test_weekly.py::test_comprehensive_v2_images - AssertionError
# FAILED tests/integration/test_weekly.py::test_comprehensive_v3_images - AssertionError
# 2 failed, 3 skipped in 1339.78s (0:22:19)
# For v2:
# Total number of images checked: 4376
# Missing images: 117
# For v3:
# Total number of images checked: 5216
# Missing images: 138 |
|
@forsyth2 nice to see all task status shows |
|
@chengzhuzhang @tomvothecoder I'm just checking in to see if either of you were able to look into the diffs any further. I know we discussed the challenges of the image diff checks. The internet suggests a number of ways to do a more nuanced approach to image diff checking, which I could try to implement, but we still have the problem of a lot of diffs being the same thing qualitatively. For instance, looking at some of the diffs, I see axis label differences "-60 => 60 S". It would be fantastic if we had some way for the code to automatically recognize these things and then group them together -- e.g., "here's all the diffs that are just axis label differences" or "here's all the diffs where the metrics differ". That might be infeasible though. |
|
@forsyth2 thank you for the paths. Could you also provide the e3sm_diags run directories of these runs? The metrics overview tables are not included in the image checker. Also, does the image checker output the filenames of missing images? |
@chengzhuzhang For
For the
It prints them as part of the test output, but they don't show up in the
and not at the output under:
|
|
@forsyth2 thanks, but I'm still having trouble locating these files: |
Sorry, I was trying to say all these notes are in this directory. |
|
@forsyth2 thank you. And could you also give the links or paths for the baseline runs (which provides the expected results)? |
It's the |
There was a problem hiding this comment.
@chengzhuzhang We discussed just merging this PR, because it appears the integration test diffs are most likely coming from 1) e3sm_diags itself or 2) the testing code (e.g., a path might need to be updated).
I can do that, but can I first get a visual inspection from you on the combined commits? (https://github.com/E3SM-Project/zppy/pull/651/files, changes explained in comments as part of this self-review). Thanks!
| @@ -67,64 +67,6 @@ create_links_climo_diurnal() | |||
| cd .. | |||
| } | |||
|
|
|||
| create_links_ts() | |||
There was a problem hiding this comment.
- We completely remove the
create_linksfunctions. No more usingcdscanto compile a list of files into an xml.
| # Create xml files for time series variables | ||
| ts_dir_source={{ output }}/post/atm/{{ grid }}/ts/monthly/{{ '%dyr' % (ts_num_years) }} | ||
| create_links_ts ${ts_dir_source} ${ts_dir_primary} ${Y1} ${Y2} 5 | ||
| ts_dir_primary={{ output }}/post/atm/{{ grid }}/ts/monthly/{{ '%dyr' % (ts_num_years) }} |
There was a problem hiding this comment.
- Rather than using the
create_linksfunction to populatets_dir_primarywith an xml listing files fromts_dir_source, we just setts_dir_primaryto point the files directly. No morets_dir_sourceat all. No more file copying or compiling files into a list. Just pointing zppy directly to the source directory.
| ts_dir_source={{ reference_data_path_ts }}/{{ ts_num_years_ref }}yr | ||
| ts_dir_ref=ts_ref | ||
| create_links_ts ${ts_dir_source} ${ts_dir_ref} ${ref_Y1} ${ref_Y2} 6 | ||
| ts_dir_ref={{ reference_data_path_ts }}/{{ ts_num_years_ref }}yr |
There was a problem hiding this comment.
- Same as above, but for model-vs-model's ref
| {%- endif %} | ||
| ts_rof_dir_source="{{ output }}/post/rof/native/ts/monthly/{{ ts_num_years }}yr" | ||
| create_links_ts_rof ${ts_rof_dir_source} ${ts_rof_dir_primary} ${Y1} ${Y2} 7 | ||
| ts_rof_dir_primary="{{ output }}/post/rof/native/ts/monthly/{{ ts_num_years }}yr" |
There was a problem hiding this comment.
- Same as above, but for streamflow
| ts_rof_dir_source={{ reference_data_path_ts_rof }}/{{ ts_num_years_ref }}yr | ||
| ts_rof_dir_ref=ts_rof_ref | ||
| create_links_ts_rof ${ts_rof_dir_source} ${ts_rof_dir_ref} ${ref_Y1} ${ref_Y2} 8 | ||
| ts_rof_dir_ref={{ reference_data_path_ts_rof }}/{{ ts_num_years_ref }}yr |
There was a problem hiding this comment.
- Same as above, but for streamflow's model-vs-model ref
There was a problem hiding this comment.
@forsyth2 I can confirm that with the second commit, the missing 36 figures based on monthly time-series data are created now. The code change also looks okay for me.
The miss-matched figures mostly due to small cosmetics change due to matplotlib updates, though there are a few cases are non-cosmetics and not directly related to zppy's code, I'm investigating if these are known behaviors documented during e3sm-diags refactor. To not delay e3sm-unified testing, I suggest to merge this PR for now.
forsyth2
force-pushed
the
issue-346-diags-post-refactor
branch
from
2af9bd7 to
dadc602
Compare
|
Rebased and renamed second commit for clarity/accuracy. Using merge commit since there are two important commits to include here. |
@chengzhuzhang There are some cases where the CDAT-based code had bugs, resulting in mismatches. Can you document the specific variables/files and I'll confirm which ones? Here are some clues:
if "MISRCOSP-CLDLOW_TAU1.3_9.4_MISR" not in f
and "MISRCOSP-CLDLOW_TAU1.3_MISR" not in f
and "MISRCOSP-CLDLOW_TAU9.4_MISR" not in f
and "MISRCOSP-CLDTOT_TAU1.3_9.4_MISR" not in f
and "MISRCOSP-CLDTOT_TAU1.3_MISR" not in f |
|
@tomvothecoder thank you! i was scratching my head to understand the mismatches. The list of documented issues can help a lot. I will try do document all diffs, to see if there are additional items need to be documented and addressed. |
|
Thank you for this PR @forsyth2! |
|
It is a headache to review 2k+ mis mached images and capture the meaningful signal. Most of the problem was cause by:
I think they can be further grouped, the MISRCOSP diff can be explained by a recent fix. Other than this, some figures missing the last longitude grid, some missing last latitude, and some have more mysterious diffs... @tomvothecoder maybe you can recognize if there are some diffs that are expected? |
Thank you for working on this @chengzhuzhang. I will take a closer look at those specific variables. |
Issue resolution
Select one: This pull request is...
1. Does this do what we want it to do?
Objectives:
zppyto call the CDAT-migrated E3SM DiagsRequired:
If applicable:
2. Are the implementation details accurate & efficient?
Required:
If applicable:
zppy/conda, not just animportstatement.3. Is this well documented?
Required:
4. Is this code clean?
Required:
If applicable: