Enhance robust pcmdi - #49
zhangshixuan1987 wants to merge 34 commits into
Conversation
|
@forsyth2: Hi Ryan, I am submitting this pull request with several bug fixes identified during the code review process while addressing the reported issues in E3SM-Project/zppy#807. I also included a few fixes related to the ENSO code path that is planned to be activated in the near future. I decided to address them now because the relevant code is already part of the current code base, so it would be better to resolve these issues before the functionality becomes fully enabled and more widely used. |
zhangshixuan1987
added
the
semver: small improvement
151329f to
97b6ce8
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves the robustness of the PCMDI diagnostics interface by tightening input validation, making viewer/collector output generation more deterministic, and improving failure diagnostics (logging and error handling) across ENSO, mean-climate, and variability-modes workflows.
Changes:
- Hardened viewer generation and synthetic plot handling (directory creation, deterministic file selection, safer config coercion) and added/expanded unit coverage.
- Improved job execution reliability and error observability (better logging, clearer failures, basic batch cleanup on parallel failures).
- Enhanced ENSO / modes collectors with stricter argument validation, safer file moves/renames, and more defensive parsing of produced outputs.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| zppy_interfaces/pcmdi_diags/viewer.py | Adds logger usage, safer output-dir creation, deterministic glob selection, and mode normalization for viewer tables. |
| zppy_interfaces/pcmdi_diags/utils.py | Improves parallel/serial subprocess failure reporting and validates worker count. |
| zppy_interfaces/pcmdi_diags/synthetic_plots/synthetic_metrics_plotter.py | Fixes ENSO collection extraction per-stat and improves mean-climate portrait handling when variables/seasons are missing. |
| zppy_interfaces/pcmdi_diags/synthetic_plots/enso_metrics_reader.py | Adds input validation and safer JSON structure checks while collecting ENSO metric JSON paths. |
| zppy_interfaces/pcmdi_diags/pcmdi_variability_modes.py | Strengthens parameter validation, improves warnings for missing outputs, and moves from prints to logging. |
| zppy_interfaces/pcmdi_diags/pcmdi_synthetic_plots.py | Uses safer bool parsing, guards missing logo copy, and improves argument parsing for debug. |
| zppy_interfaces/pcmdi_diags/pcmdi_setup.py | Adds required-argument checks, improves error handling for unexpected file formats, and standardizes logging. |
| zppy_interfaces/pcmdi_diags/pcmdi_mean_climate.py | Adds required-argument checks, fixes CLIM path typo, improves missing-output warnings, and hardens filename parsing. |
| zppy_interfaces/pcmdi_diags/pcmdi_enso.py | Enables ENSO flow (removes early exit), adds catalogue normalization, hardens file collection/moves, and improves output validation. |
| zppy_interfaces/pcmdi_diags/link_observation.py | Uses context manager for reading JSON alias file. |
| tests/unit/pcmdi_diags/test_viewer.py | Adds targeted tests for coupled-mode EOF tagging, config normalization, and out_dir creation behavior. |
| tests/unit/pcmdi_diags/test_synthetic_metrics_plotter.py | Adds tests for drop_vars behavior and mean-climate portrait variable consistency. |
| tests/unit/pcmdi_diags/test_pcmdi_variability_modes.py | Updates expectations for quoted reference_data_path. |
| tests/unit/pcmdi_diags/test_pcmdi_mean_climate.py | Fixes import path typo in test. |
| pyproject.toml | Fixes console script entrypoint for mean-climate module name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
forsyth2
left a comment
forsyth2
left a comment
There was a problem hiding this comment.
I'm reviewing this PR and the corresponding zppy PR together. I had both Copilot and Claude do initial reviews. I then did a visual inspection based on their comments. (I will post a similar review for that PR).
Once these comments are addressed, I can run zppy's integration tests on it. I'm also adding zppy-interfaces docs in #51, so I'll want to update those to reflect important changes from this PR.
Claude's summary
These two PRs were made in conjunction and should be reviewed together. At a high level, they extend the zppy/zppy-interfaces pipeline with MPAS ocean and sea-ice component support, while also cleaning up configuration, fixing bugs, and expanding test coverage.
High-Level Summary
| Area | zppy |
zppy-interfaces |
|---|---|---|
| New feature | MPAS ocean/sea-ice time-series processing | — |
| Config changes | New MPAS parameters; removal of vertical remap params from [ts] |
Default EMOV modes list updated |
| Bug fixes | enso set handling; dependency wiring for e3sm_to_cmip |
Typos in module names, paths, and variable names |
| Robustness | Multi-subsection dependency support; richer error codes in shell templates | Output dir auto-creation; Jinja2 template validation; sorted glob; normalized mode inputs |
| Logging | print → logger.debug in pcmdi_diags.py |
print → logger.info in viewer.py |
| Tests | — | New tests for synthetic_metrics_plotter, expanded test_viewer.py |
zppy-interfaces PR
1. Typo fixes (bugfixes)
Three typos were silently breaking things:
pcmdi_mean_cimate→pcmdi_mean_climatein bothpyproject.toml(CLI entry point) andtest_pcmdi_mean_climate.py(import). Would have caused thezi-pcmdi-mean-climatecommand to fail at install time.CLIM_patttern→CLIM_patterninMeanClimateTableBuilder. Would have produced broken figure links.seasonsvariable renamed toregionsinmap_regions(). No behavior change, but removes a misleading name.
2. EOF compose fix in CMVARGroupBuilder
A copy-paste bug caused "EOF(Yearly)" and "EOF(Monthly)" entries to use the cbf tag instead of the appropriate eof tag. The fix introduces a map_coupled_mode_to_eof_tag() static method that maps NPGO → eof2 and all other modes → eof1. Mode names are now stripped and uppercased before lookup.
3. Default EMOV modes list changed
generate_emovs_table changed from ["PDO", "NPGO", "AMO"] to ["NAM", "PNA", "NPO", "NAO", "SAM", "PSA1", "PSA2"]. This is presumably intentional — PDO/NPGO/AMO now belong to the coupled modes table — but it is easy to miss buried among other changes. This should be clearly documented in a changelog or release note, and any existing user configs that relied on the default should be reviewed.
4. Robustness improvements in viewer.py
setup_jinja_env()now raisesFileNotFoundErrorif the template directory is missing (fail-fast).generate_methodology_html(),generate_data_html(), andgenerate_viewer_html()all callPath(...).mkdir(parents=True, exist_ok=True)before writing output, preventing crashes on missing directories.glob.glob()results are now sorted before selecting the first match, ensuring deterministic behavior across filesystems.join_list()now passes strings through directly if the config value is already a string. Minor concern: this silently absorbs a likely misconfiguration. A warning log would be more appropriate.- All
print(...)calls replaced withlogger.info(...). Consistent with standard Python logging.
5. run_serial_jobs error logging improvements
Error messages now include the job index, return code, and full stdout/stderr — a meaningful debuggability improvement. The stdout.strip() / stderr.strip() are now applied once before the error check and stored in results. Verify no downstream consumers relied on trailing newlines in those values.
6. Test coverage expansion
- New
test_synthetic_metrics_plotter.pyadds unit tests fordrop_varsandmean_climate_plot_driver. Well-structured, but could add edge cases for all-missing or none-missing variables. The shape assertionvalues.shape == (1, 2)would benefit from a comment explaining the dimensions. test_viewer.pygrows from 2 tests to ~12+, coveringCMVARGroupBuilder,generate_cmvar_table,generate_emovs_table, and others. Tests usetmp_pathandmonkeypatchappropriately.
7. Variability modes test: quoted refpath
Expected CLI strings now wrap --reference_data_path in quotes ("refpath"). shell=True, quoting strategy inside command strings can be fragile and platform-dependent. Confirm the quoting is applied consistently in the actual implementation.
Summary of Concerns
| Severity | Location | Issue |
|---|---|---|
zppy-interfaces/viewer.py |
Default EMOV modes list changed — behavioral breaking change for users relying on defaults | |
zppy-interfaces |
Quoted refpath in variability modes tests — confirm quoting strategy is robust with shell=True |
|
zppy-interfaces/viewer.py |
join_list string passthrough absorbs misconfiguration silently — consider a warning log |
|
| 💡 Suggestion | zppy-interfaces tests |
test_synthetic_metrics_plotter.py could cover more edge cases |
- pcmdi_setup.py: raise ValueError in _extract_metadata() when filename parts are missing, instead of logging and falling through to IndexError; fix typo "dervied" -> "derived" in log message - link_observation.py: use context manager for obs_alias_file to ensure file handle is closed - enso_metrics_reader.py: guard nested RESULTS.model dict access with .get() and raise a descriptive KeyError when structure is missing, instead of an opaque KeyError at runtime - utils.py: replace Popen(shell=True) with shlex.split() + shell=False in run_parallel_jobs() and run_serial_jobs() to eliminate shell injection risk; add shlex import - pcmdi_enso.py, pcmdi_synthetic_plots.py: replace bare json.load(open()) calls with context managers to prevent file descriptor leaks All 7 unit tests pass (zi-pcmdi-diags-20260430).
- ENSOParameters: validate enso_groups is not None at construction time instead of crashing with AttributeError on .split() downstream - EnsoDiagnosticsCollector.__init__: validate model_name_parts has exactly 4 elements before tuple unpack, giving a descriptive error - collect_figures: guard os.listdir(fdir) with os.path.isdir() before calling it; guard error-log dir listing the same way to prevent FileNotFoundError inside the logger call itself - collect_figures: warn when model/relm marker is absent from filename before splitting, preventing silent wrong output filenames - collect_metrics / collect_diags: replace bare os.listdir() inside logger error f-strings with isdir-guarded dir_contents variable - main(): check obs_dict is non-empty before [0] index to avoid IndexError on empty obs_catalogue.json - check_enso_input: guard both os.symlink() calls with os.path.exists() to prevent FileExistsError on re-run/retry - check_vars: add re.DOTALL flag to list_variables regex so multi-line driver stdout does not produce a false "no variable list found" failure All 7 unit tests pass (zi-pcmdi-diags-20260430).
- rename pcmdi_mean_cimate.py -> pcmdi_mean_climate.py (correct spelling); update entry point in pyproject.toml and import in test file - MeanClimateParameters: validate --regions is not None before .split() to prevent AttributeError on missing CLI argument - MeanClimateMetricsCollector.__init__: validate model_info has exactly 4 elements before tuple unpack, giving a descriptive error - _collect_figures: fix output directory key typo "CLIM_patttern" -> "CLIM_pattern"; add logger.warning when no figures are found for a var/region/season combination instead of silently skipping - _collect_metrics: guard parts[1] access with len(parts) < 2 check and log+skip on unexpected filename format instead of IndexError - main(): re-raise RuntimeError from job runners instead of swallowing it with print(), preventing silent wrong results after job failure; replace all print() calls with logger.info() - generate_mean_clim_cmds: add logger.warning when a variable is not found in obs_dic instead of silently omitting its command All 7 unit tests pass (zi-pcmdi-diags-20260430).
- VariabilityModesParameters: validate --var_modes and --vars are not None at construction time instead of crashing with AttributeError or KeyError: None downstream - main(): validate model_name has exactly 4 dot-separated parts before index access to prevent IndexError - main(): re-raise RuntimeError from job runners instead of swallowing it with print(), preventing silent wrong results after job failure; replace all print() calls with logger.info() - _collect_figures: add logger.warning when no files match a mode/season combination instead of silently skipping - _classify_output_name: add logger.warning when filename does not match any known pattern and suffix falls back to "unknown" - generate_varmode_cmds: quote refpath in command string so paths containing spaces are handled correctly by shlex.split - test_generate_varmode_cmds: update expected strings to reflect quoted refpath All 7 unit tests pass (zi-pcmdi-diags-20260430).
- SyntheticPlotsParameters: replace all bare args["x"].split(",") calls
with None-safe guards so missing optional list arguments (clim_vars,
clim_regions, mova_modes, mova_vars, movc_modes, movc_vars, enso_vars)
no longer raise AttributeError
- SyntheticPlotsParameters: replace all str(args["x"]).lower() in (...)
boolean checks with str2bool(args.get("x", False)) to use the existing
helper consistently; previously str("None") silently evaluated to
False,
masking missing viewer flags (clim_viewer, mova_viewer, movc_viewer,
enso_viewer, save_all_data)
- main(): guard shutil.copy for e3sm_pmp_logo.png with os.path.exists
check; log a warning and skip instead of raising FileNotFoundError
when pcmdi_external_prefix is misconfigured
- _get_args(): change --debug to type=str2bool with default=False and
simplify check to `if args.debug:`; previously only "true" was
recognised, silently ignoring "1", "yes", etc.
All 7 unit tests pass (zi-pcmdi-diags-20260430).
with logger
- CoreParameters: validate num_workers and multiprocessing are not None
before int() and .lower() calls to prevent cryptic TypeError/
AttributeError on missing CLI arguments
- CoreParameters: guard --vars with args.get() before .split(",") to
prevent AttributeError on missing argument
- set_up(): split model_name once into model_name_parts and validate
len >= 2 before index access; eliminates IndexError on malformed
model name and removes two redundant .split() calls in input_template
construction
- set_up(): validate model_name_ref is non-None and has >= 2 parts in
model_vs_model branch to prevent None.split() and IndexError
- _process_group(): replace print() warning with logger.warning() so
missing catalogue warning respects log-level control
- _generate_mask(): replace both print() calls with logger.info() so
mask method info is captured in log output
- derive_missing_variable(): replace print() with logger.info() for
derived variable write confirmation
- Fix typo "assigining" -> "assigning" in two log messages
All 7 unit tests pass (zi-pcmdi-diags-20260430).
- run_parallel_jobs: raise ValueError if num_workers < 1 instead of
silently degrading to serial execution
- run_parallel_jobs: rename inner loop variables cmd/proc to
batch_cmd/batch_proc to eliminate shadowing of the outer cmd variable
- run_parallel_jobs: terminate all remaining running batch processes
before raising RuntimeError on job failure, preventing orphaned
subprocesses from running indefinitely after an error
- run_parallel_jobs: improve batch log message from misleading
"Running {count_child_processes()} subprocesses" (counted before
launch) to "Running batch of {len(procs)} subprocesses"
All 7 unit tests pass (zi-pcmdi-diags-20260430).
- MeanClimateTableBuilder.map_regions(): rename local variable from `seasons` to `regions` throughout the method; copy-paste bug caused no runtime error but was misleading and error-prone - build_table(): update figure path from "CLIM_patttern" to "CLIM_pattern" to match the corrected output directory name from pcmdi_mean_climate.py; viewer was silently finding no files - setup_jinja_env(): add os.path.isdir() check before creating the Jinja2 environment; previously a missing template directory produced an unhelpful TemplateNotFound error with no indication the directory itself was absent - Add module-level `import logging` and `logger = logging.getLogger` at the top of the file; previously the file had no logger - generate_methodology_html(), generate_data_html(), generate_viewer_html(): replace print() calls with logger.info() so HTML write confirmations respect log-level control and appear in log output All 7 unit tests pass (zi-pcmdi-diags-20260430).
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved ENSO setup, catalogue, case-handling, and subprocess-cleanup issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
zppy_interfaces/pcmdi_diags/pcmdi_enso.py:183
- These lines treat any falsy metric value as incomplete. A valid ENSO metric can be numeric
0, so it will be removed from the copied JSON before the collector moves it, causing zero-valued results to disappear. Check for a missing/None/empty metric payload rather than truthiness so valid zeroes are preserved.
incomplete = [
m for m, v in value_block.items() if not v.get("metric")
]
zppy_interfaces/pcmdi_diags/pcmdi_enso.py:611
- This new underscore-style filename branch is not supported by the shared setup:
DataCatalogueBuilderstill searches only*.{var}.*.nc(pcmdi_setup.py:91-97and316-318). Such a file is symlinked here but produces no model catalogue metadata, so the subsequent ENSO driver cannot use the accepted input. Extend catalogue discovery to the same convention before treating it as supported.
found_nc_file = glob.glob(
os.path.join(ts_dir, f"*.{cmip_var_name}.*.nc")
) + glob.glob(os.path.join(ts_dir, f"{cmip_var_name}_*.nc"))
zppy_interfaces/pcmdi_diags/pcmdi_setup.py:293
- All three command entry points document and validate
model_nameasmip.exp.model.relm, but this shared setup now accepts any value with at least two parts. A 3- or 5-part value can perform catalogue or derived-file work and only fail later in a collector, so reject non-4-part names here.
if len(model_name_parts) < 2:
raise ValueError(
f"model_name must have at least 2 dot-separated parts, "
f"got: {parameters.model_name}"
)
zppy_interfaces/pcmdi_diags/pcmdi_variability_modes.py:25
var_modesis later used directly byVariabilityMetricsCollector, whereasgenerate_varmode_cmdsstrips each mode. With a value such as--var_modes 'NPO, PDO', the PDO job runs but collection searches for a directory named' PDO'and leaves its outputs uncollected. Normalize and strip the list here as well.
self.var_modes: List[str] = var_modes.split(",")
- Files reviewed: 18/18 changed files
- Comments generated: 7
- Review effort level: Lite
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Fixed in 2fd895b. |
forsyth2
left a comment
forsyth2
left a comment
There was a problem hiding this comment.
I had Claude summarize the changes in this PR, since it is adding nearly 2,000 lines of code. I'm pasting its individual file diff summaries as part of this review.
(This is just a summary review; no action items)
forsyth2
left a comment
forsyth2
left a comment
There was a problem hiding this comment.
Now for actual items from Claude
|
@zhangshixuan1987 Can you address Claude's notes in my review above. They seem to be mostly confirming design decisions -- so if you could just confirm the decisions were intentional (or note if the code should be fixed). Once that is done, I'll do a test run of just the |
Hi @forsyth2 : thank you for putting these together. I will follow the comments and attempt to review and adjust as needed. I will notify you once I push the changes to the PR. |
|
Great, thanks @zhangshixuan1987! |
@forsyth2 Hi Ryan, I followed up on the review comments and confirmed or refined the code further where needed. I have pushed the latest changes to this PR. In addition, I ran a standalone test for ENSO using the E3SM data I had collected and tested previously, with the final revised code in this PR together with the corresponding changes in the zppy branch. The standalone test completed successfully on my side. Could you please take another look and test it on your side to see whether any additional issues arise? If so, I would be happy to help further refine the implementation. |
|
Thanks @zhangshixuan1987. Chrysalis isn't available today, so I'll do a visual inspection, and once it's back up I'll run the test suite to see if any issues come up. Also, Xylar let us know the release candidate (RC) deadline for E3SM-Unified 1.14.0 is Oct. 5. That means we need to have this PR and E3SM-Project/zppy#815 merged by then, so I can make a |
2026-09-14 zppy pcmdi_diags ENSO support testI just updated the expected results as noted here Using the test scriptA. Set up the test scriptcd ~/ez/zppy
git status
# On branch partial-update-expectations
# nothing to commit, working tree clean
git fetch upstream main
git checkout -b test-zppy-pr815-20260914 upstream/main
git log --oneline | head -n 1
# 5232fe30 Rank image check failures by severity (#865))
# Good, matches https://github.com/E3SM-Project/zppy/commits/main
# Now, copy the test script and cfg from the zppy repo into the directory
# that you'll be running the test script from.
mkdir -p ~/ez/zppy_main_branch_tests/test_20260914_run2
cd ~/ez/zppy_main_branch_tests/test_20260914_run2
cp ~/ez/zppy/tests/main_branch_testing/run_integration_test.bash .
cp ~/ez/zppy/tests/main_branch_testing/zppy_test.cfg .
# Now, edit the test cfg as needed
emacs zppy_test.cfgB. Set up the test cfgC. Run the test script# NOTE: This part is not listed in the docs, as the script should check this.
# To be on the safer side though, it's a good idea to check that these directories
# don't have uncommitted changes.
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/e3sm_to_cmip && git status
# nothing to commit, working tree clean
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/e3sm_diags && git status
# nothing to commit, working tree clean
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/MPAS-Analysis && git status
# nothing to commit, working tree clean
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/zppy-interfaces && git status
# nothing to commit, working tree clean
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/zppy && git status
# nothing to commit, working tree clean
cd ~/ez/zppy_main_branch_tests/test_20260914_run2
screen # Use `screen`` so that even if the terminal connection is interrupted, the script will keep running.
ulimit -s unlimited # This is necessary for MPAS-Analysis to work inside `screen`
cd ~/ez/zppy_main_branch_tests/test_20260914_run2
cat zppy_test.cfg # Make sure changes are there
time ./run_integration_test.bash --config zppy_test.cfg 2>&1 | tee integration_test_run2.log
# [2026-09-14 22:17:18] Starting zppy integration test automation
# Ctrl-A D to detach from screen
screen -ls # See what screen sessions you have
# 1792866.pts-29.chrlogin1 (Detached) # For this run
# 1735354.pts-35.chrlogin1 (Detached) # From run 1
tail -f integration_test_run2.log
# Jobs remaining: 29 (elapsed: 600s / max: 14400s)[2026-09-14 23:35:42] ✗ Jobs with DependencyNeverSatisfied:
# 1287157 debug pcmdi_di ac.forsy PD 0:00 1 (DependencyNeverSatisfied)
# Jobs remaining: 7 (elapsed: 1200s / max: 14400s)
# Jobs remaining: 29 (elapsed: 600s / max: 14400s)[2026-09-14 23:35:42] ✗ Jobs with DependencyNeverSatisfied:
# 1287157 debug pcmdi_di ac.forsy PD 0:00 1 (DependencyNeverSatisfied)
# Jobs remaining: 1 (elapsed: 3000s / max: 14400s)
# Jobs remaining: 29 (elapsed: 600s / max: 14400s)[2026-09-14 23:35:42] ✗ Jobs with DependencyNeverSatisfied:
# 1287157 debug pcmdi_di ac.forsy PD 0:00 1 (DependencyNeverSatisfied)
# Jobs remaining: 1 (elapsed: 3600s / max: 14400s)Looks like the script isn't recognizing there's only one job left and it has "DependencyNeverSatisfied". We'll have to terminate that job ourselves. D. Review the output# CTRL C # Exit tail
scancel 1287157
screen -R 1792866.pts-29.chrlogin1
# CTRL C -- no point letting it run now
# real 125m23.824s
# user 4m30.471s
# sys 1m15.671s
exit # Exit screen
cd ~/ez/zppy_main_branch_tests/test_20260914_run2
# First, the unit tests.
grep -n "Running zppy-interfaces unit tests..." integration_test_run2.log # Line 1183
grep -n "zppy-interfaces unit tests passed" integration_test_run2.log # Line 1212
grep -n "Running zppy unit tests..." integration_test_run2.log # Line 2138
grep -n "zppy unit tests passed" integration_test_run2.log # Line 2158
# Second, the output directories status.
grep -n "Checking all status files..." integration_test_run2.log # No matches
grep -n "All status files clean!" integration_test_run2.log # No matches
# By cancelling early, we accidentally skipped the directory status checking steps
# We'll have to check manually.
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_legacy_3.1.0_comprehensive_v3_output/zppy_main_branch_test_20260914_run2/v3.LR.historical_0051/post/scripts
grep -v "OK" *status # Good, no errors
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v3_output/zppy_main_branch_test_20260914_run2/v3.LR.historical_0051/post/scripts
grep -v "OK" *status
# pcmdi_diags_enso_model_vs_obs_1985-1994.status:ERROR (9)
# pcmdi_diags_synthetic_plots_model_vs_obs.status:WAITING 1287157
emacs pcmdi_diags_enso_model_vs_obs_1985-1994.o1287156
# ncrcat: ERROR No records lay within specified hyperslab
# Let's at least continue on to the image checker test.Image checker testcd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/zppy
git status
# Good: On branch test_zppy_20260914_run2
# The image checker test, which we'll run from a compute node:
salloc --nodes=1 --partition=debug --time=02:00:00 --account=e3sm
source /home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh
conda activate test-zppy-zppy_pcmdi_enhancement-20260914_run2
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/zppy
pytest tests/integration/test_images.py
# Paste `Captured stdout call` below
cat test_images_summary.md
# Paste below
exit # Exit compute noteOutputComplete summary table
Results analysis:167 images are now missing. Claude notes this typo fix: If we look at the list of missing images above, we can see they're misnamed. That just means we need to update the expected results now that the code is fixed. Are ENSO plots showing up now?It doesn't look like it. https://web.lcrc.anl.gov/public/e3sm/diagnostic_output//ac.forsyth2/zppy_weekly_comprehensive_v3_www/zppy_main_branch_test_20260914_run2/v3.LR.historical_0051/pcmdi_diags/model_vs_obs/ shows: Indeed, From Claude: Yes — the combined with: From the log, What needs changing: drop the CERES-only radiation variables from ( Conclusion@zhangshixuan1987 So, we're still not quite ready to merge.
We need to either fix the ENSO code or, perhaps more likely, fix what data I'm asking for in the test cfg. |
|
@forsyth2 Hi Ryan, I think this test failure is related to the observational dataset configuration rather than the One vulnerability in the current workflow is that, by default, we use the observations best for model evaluation, say CERES EBAF for rlds, rsds, rlus, rsdt, which have limited coverage. This is perfectly fine for mean climate evaluation, but it is not good for diagnostics such as ENSO. To better address this in the workflow, I proposed to use a separate setup for the observations for each diagnostic section in zppy-pcmdi. I’ve updated the configuration in PR #815 with the following changes:
If possible, could you also share the testing setup you used? I’d be happy to run the same test on my side using the same setup so that we can compare results and troubleshoot any remaining issues. After that, I could hand it to you for the final test. |
Great, thanks I'll review the diff of the latest commits.
I used a test cfg based off You can see the output directories here:
I'm not entirely sure that's possible at the moment. I've been making a lot of improvements to the testing process (notably E3SM-Project/zppy#774 and now E3SM-Project/zppy#871). While the goal is for anyone to run this updated test script, I'm not sure we're there yet. (I'm mainly just trying to 100% automate a test run for myself before I have others try it out). That said, you could still run a zppy cfg with a matching ENSO section. |
|
@forsyth2 Hi Ryan, I wanted to follow up on the testing I did on my side for the code changes associated with this PR and the companion zppy PR #815. Based on my testing, I think the failure you encountered is mainly related to several issues in the zppy workflow addressed in PR #815, rather than to the zppy-interface changes in this PR.
With these changes in PR #815, my test on the same E3SM dataset is now able to proceed successfully through the PCMDI workflow. See
So at this point, I think it would be useful to rerun your test with the latest version of PR #815 together with the current zppy-interface PR. If there are still failures after that, I would be happy to compare the logs and continue debugging from there. |
2026-09-17 zppy pcmdi_diags ENSO support testI'm going to use the test improvements of E3SM-Project/zppy#871 here. Set up and run the testSet up the zppy branchcd ~/ez/zppy
git status
# nothing to commit, working tree clean
git checkout zppy_pcmdi_enhancement
git checkout -b zppy_pcmdi_enhancement_backup20260917
git checkout zppy_pcmdi_enhancement
git fetch upstream zppy_pcmdi_enhancement
git reset --hard upstream/zppy_pcmdi_enhancement
git log --oneline | head -n 17
# e36096c0 Fix PCMDI diags observation date extraction and period clamping
# ...
# fc2df2d8 One authoritative production path per case; nest development by user (#868)
# Good, has the 16 commits from https://github.com/E3SM-Project/zppy/pull/815/commits
lcrc_conda # Activate conda
rm -rf build
conda clean --all --y
conda env create -f conda/dev.yml -n zppy-pcmdi-test-20260917
conda activate zppy-pcmdi-test-20260917
pre-commit run --all-files
python -m pip install .
git checkout -b combined-pcmdi-test
git fetch upstream issue-869-improve-test-automation
git rebase upstream/issue-869-improve-test-automation # Add the 24 commits from PR 871
git log --oneline | head -n 42
# 8f3724a2 Fix PCMDI diags observation date extraction and period clamping
# ...
# 00216ed6 Enable experimental PCMDI ENSO diagnostics
# 83d5644d One authoritative production path per case; nest development by user (#868)
# c754ea47 Split zppy-interfaces and rename e3sm_to_cmip/zppy expected-results date tracking
# ...
# 827cb4c6 Automate image checker, add per-task env descriptions, and auto-generate test report
# 5232fe30 Rank image check failures by severity (#865)
# Now, we have the PCMDI Diags PR (#815) built on top of the improved test script (#871)Set up the test scriptcd ~/ez/zppy
git status
# On branch combined-pcmdi-test
# nothing to commit, working tree clean
# NOTE: For `ZPPY_BASE_BRANCH="combined-pcmdi-test"` to work,
# we need to push that branch to GitHub so the script can fetch that branch.
# It doesn't need a PR made though.
git push upstream combined-pcmdi-test
# Now, copy the test script and cfg from the zppy repo into the directory
# that you'll be running the test script from.
mkdir -p ~/ez/zppy_main_branch_tests/test_20260917_run1
cd ~/ez/zppy_main_branch_tests/test_20260917_run1
cp ~/ez/zppy/tests/main_branch_testing/run_integration_test.bash .
cp ~/ez/zppy/tests/main_branch_testing/zppy_test.cfg .
# Now, edit the test cfg as needed
emacs zppy_test.cfgSet up the test cfgls -lt /lcrc/group/e3sm/public_html/zppy_test_resources/expected_comprehensive_v3/
# Sep 14 21:11 pcmdi_diags
# Sep 4 11:55 mpas_analysis
# Sep 4 11:53 e3sm_diags
# Aug 14 17:01 global_time_series
# May 20 13:52 livvkit
# May 20 13:51 ilambshows the date results were promoted to be official expected results. We can look at the testing log to see what the run before that date was.
Run the test script# NOTE: This part is not listed in the docs, as the script should check this.
# To be on the safer side though, it's a good idea to check that these directories
# don't have uncommitted changes.
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/e3sm_to_cmip && git status
# nothing to commit, working tree clean
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/e3sm_diags && git status
# nothing to commit, working tree clean
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/MPAS-Analysis && git status
# nothing to commit, working tree clean
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/zppy-interfaces && git status
# nothing to commit, working tree clean
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/zppy && git status
# Has uncommitted changes
git add -A
git commit -m "Testing" --no-verify
git status
# nothing to commit, working tree clean
cd ~/ez/zppy_main_branch_tests/test_20260917_run1
screen # Use `screen`` so that even if the terminal connection is interrupted, the script will keep running.
ulimit -s unlimited # This is necessary for MPAS-Analysis to work inside `screen`
cd ~/ez/zppy_main_branch_tests/test_20260917_run1
cat zppy_test.cfg # Make sure changes are there
time ./run_integration_test.bash --config zppy_test.cfg 2>&1 | tee integration_test_run1.log
# [2026-09-17 12:27:18] Starting zppy integration test automation
# Ctrl-A D to detach from screen
screen -ls # See what screen sessions you have
tail -f integration_test_run1.log
# Note: already started run, but in general it's faster
# to set the branches you're not testing to use Unified.
# That way a dev env doesn't have to be built.
# 14:30 CT:
# Jobs remaining: 9 (elapsed: 600s / max: 14400s)[2026-09-17 14:08:52] ✗ Jobs with DependencyNeverSatisfied:
# 1289577 debug pcmdi_di ac.forsy PD 0:00 1 (DependencyNeverSatisfied)
# Jobs remaining: 1 (elapsed: 2400s / max: 14400s)
# Let's cancel that job. In another tab:
scancel 1289577
# We'll now wait for the script to continue.D. Review the output# CTRL C # Exit tail
screen -R
# [2026-09-17 15:00:43] ✓ Markdown report written: /home/ac.forsyth2/ez/zppy_main_branch_tests/test_20260917_run1/test_report_20260917_run1.md
# real 153m25.620s
# user 9m52.678s
# sys 2m36.317s
exit # Exit screenCopying the auto-generated AUTO-GENERATED REPORTAuto-generated report20260917 zppy testBelow, I follow the steps of the automated testing docs page. Step 1: Determine what the current expected results arePromotion date for each cfg/task under
Step 2: Review changes since expected results were updatedCommits merged on each repo's expected-results baseline branch (see the "Branch tested" column when it differs from the branch this run actually tested) since that dependency's expected results were actually produced (for
Environment descriptionsAn
Automated test script results
TODO: one or more steps above did not pass -- review the full log captured from this script's stdout (for example, Step 8: Run Python testsThe image checker (
Complete summary tableSummary of test results
Summary table -- only failing image-check tests, sorted by task
Results analysisTODO: fill in analysis of any failures above (expected vs. unexpected, whether expected results should be updated, etc.). Manual review of results
The sample of cosmetic diffs and the grid of bigger diffs appear to only have small diffs, however some of those do involve plots looking slightly different. The list of missing images was previously identified to be simply because the expected results need to be updated to point to the new file names. Did ENSO plots get produced?No, https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_weekly_comprehensive_v3_www/zppy_main_branch_test_20260917_run1/v3.LR.historical_0051/pcmdi_diags/model_vs_obs/metrics_data/ does not contain ENSO plots. Did any jobs fail?cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_legacy_3.1.0_comprehensive_v3_output/zppy_main_branch_test_20260917_run1/v3.LR.historical_0051/post/scripts
grep -v "OK" *status
# Good, no errors
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v3_output/zppy_main_branch_test_20260917_run1/v3.LR.historical_0051/post/scripts
grep -v "OK" *status
# pcmdi_diags_enso_model_vs_obs_1985-1994.status:ERROR (12)
# pcmdi_diags_synthetic_plots_model_vs_obs.status:WAITING 1289577
cat pcmdi_diags_enso_model_vs_obs_1985-1994.o1289576gives: Synthetics plots can't start because ENSO failed, because @zhangshixuan1987 It looks like we need to remove |
|
@forsyth2: Hi Ryan, unfortunately, the hfls is mandatorily needed by pcmdi-enso to generate a complete list of metrics data and figures. Simply removing hfls from enso_vars will not be preferred and will lead to errors in the subsequent enso metrics processing step. I think the error here is simply because you did not process the hfls data, as I did not find the data file under the following directory: To trace back the cause, in your This is correct as hfls is indeed included in the cmip_vars. However, when we further trace back to your [ts] section You did not add LHFLX as part of the time series processing. So the LHFLX data from e3sm is never produced; therefore, [e3sm_to_cmip] will not produce hfls as there is no upstream LHFLX passed in. Here is what I used in my setup: A comment from me: I think it is a good idea to make the [ts], [e3sm_to_cmip], and diagnostic sections, including [e3sm_diags] and [pcmdi_diags], fully independent. This would make the workflow more flexible and modular. However, this also places greater responsibility on the user. In particular, the user needs to understand which variables are required by each diagnostic package and make sure those variables are properly included in both the [ts] and [e3sm_to_cmip] sections. To me, it may be worth thinking about a good "default" setup in the example cfg file to ensure that a default list of variables required by each set of diagnostics supported by zppy can be processed anyway. |
|
Thanks @zhangshixuan1987, I've replied on the zppy PR. |
|
@forsyth2: Hi Ryan, as your AUTO-GENERATED REPORT seems to perform a comparison with some existing pcmdi diagnostic results you perhaps generated before. If the target is to do a fair comparison with the old results like what you showed in the table above, please consider following extra modifications in your cfg file (/lcrc/group/e3sm/ac.forsyth2/zppy_weekly_legacy_3.1.0_comprehensive_v3_output/zppy_main_branch_test_20260917_run1/v3.LR.historical_0051/post/scripts/provenance.20260917_184844_958693.cfg)
Simply remove "Tropics,NHEX,SHEX" so that we do not process it for this test to save time. Also, for sample run, we do not need these regions.
This change is due to the update in the reference dictionary which now assign a unique key for each observations for each variable. By default, HadISST2 observations is used, and is now referred to as alternatd4 (previously is alternate1). We note that the use of HadISST2 is to make the portrait and parallel coordinate metrics figure more meaningful because the CMIP6 reference datasets for comparison also used the HadISST2 observations,
Similar to what we described in 2, the changes here is also due to the change in reference dictionray. By default, NOAA-20C observations is used, and is now referred to as alternate4 (previously is alternate1). We note that the use of NOAA-20C is to make the portrait and parallel coordinate metrics figure more meaningful because the CMIP6 reference datasets for comparison also used the NOAA-20C,
I checked the directory :
Note that: [[ atm_monthly_180x360_aave ]] does not explicitly specify the vars in the test. So it may not include enought variables for downstream [e3sm_to_cmip] process (bold variables in list above are madatorily required for enso diagnostics). Specfically, [e3sm_to_cmip] requires (1) LHFLX to process hfls; (2) FLDS and FLNS to process rlds and rlus; (3) FSDS to process rsds, and (4) SOLIN to process rsdt; (5) PSL, PRECT, TS, THREFHT, SHFLX, TAUX, TAUY for psl,pr,ts,tas,hfss, tauu, and tauv, respectively.
|
|
Thanks @zhangshixuan1987, responded here. |
Summary
This pull request enhances the robustness of the PCMDI diagnostics utilities by fixing bugs related to ENSO metric extraction and synthetic plot handling, improving logging clarity and parallel computing reliability, and making incremental enhancements to the viewer and utility modules.
Objectives:
Issue resolution:
This pull request is
Small Change
1. Does this do what we want it to do?
-Product Management: I have confirmed with the stakeholders that the objectives above are correct and complete.
-Testing: I have considered likely and/or severe edge cases and have included them in testing.
Note: Testing was performed together with another bug fix change to address .
2. Are the implementation details accurate & efficient?
3. Is this well documented?
4. Is this code clean?
-All the pre-commits checks have passed.