Enhance robust pcmdi - #49
Draft
zhangshixuan1987 wants to merge 24 commits into
Draft
Conversation
Contributor
Author
|
@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
- 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).
- Use shutil.move for cross-filesystem-safe file moves - Normalize ENSO group parsing by stripping whitespace - Make figure, metric, and diagnostic output replacement explicit - Add destination directory guards before overwriting outputs - Write metrics JSON through a temporary file before replacing destination - Improve output validation with return-code checks and clearer directory checks
- Make output collection more robust across filesystems - Add explicit replacement handling for figures, metrics, and diagnostics - Strip whitespace from ENSO group inputs - Validate command return codes and reported output directories - Support prefix-style NetCDF filenames when symlinking alternative variables - Extend existing alias treatment consistently for wind stress inputs
zhangshixuan1987
force-pushed
the
enhance_robust_pcmdi
branch
from
May 15, 2026 20:27
151329f to
97b6ce8
Compare
Correct the coupled modes variability viewer so MOV_compose EOF links use the expected EOF mode instead of incorrectly pointing to CBF figures. Coupled SST modes are now mapped as: AMO/PDO -> eof1 and NPGO -> eof2. Mode names are normalized from configuration input so lowercase or whitespace-padded values still produce the expected filenames and labels. Improve viewer robustness: - default EMoV viewer modes now use atmospheric PSL modes, not coupled SST modes - atmospheric EMoV mode names are normalized before filename generation - output directories are created before writing viewer HTML files - string-valued config lists are preserved instead of joined character by character - image glob matches are sorted for deterministic link selection Harden mean-climate synthetic portrait plotting by filtering variables per region/stat/season before dataframe indexing. This prevents missing variables in region-specific dataframes from raising pandas KeyError and instead skips unavailable variables with a warning. The shared drop_vars helper now also removes requested variables that are absent from the dataframe. Extract the mean-climate portrait variable preparation into a helper to keep mean_climate_plot_driver below the flake8 complexity threshold. Add regression tests for coupled EOF link generation, config normalization, EMoV defaults, viewer output handling, and mean-climate missing-variable handling.
Contributor
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.
Comment on lines
69
to
+73
| if return_code != 0: | ||
| # Terminate any remaining running processes in the batch | ||
| for _, remaining_proc in procs: | ||
| if remaining_proc.poll() is None: | ||
| remaining_proc.terminate() |
Comment on lines
+433
to
+436
| Path(cfg("out_dir", ".")).mkdir(parents=True, exist_ok=True) | ||
| out_path = os.path.join(cfg("out_dir", "."), "methodology.html") | ||
| Path(out_path).write_text(rendered_html) | ||
| print(f"HTML file written to: {cfg('out_dir')}") | ||
| logger.info(f"HTML file written to: {cfg('out_dir')}") |
Collaborator
There was a problem hiding this comment.
i.e., logger.info(f"HTML file written to: {out_path}")
Comment on lines
+637
to
+640
| Path(cfg("out_dir", ".")).mkdir(parents=True, exist_ok=True) | ||
| out_path = os.path.join(cfg("out_dir", "."), "diag_data.html") | ||
| Path(out_path).write_text(output_html) | ||
| print(f"HTML file written to: {cfg('out_dir')}") | ||
| logger.info(f"HTML file written to: {cfg('out_dir')}") |
Collaborator
There was a problem hiding this comment.
i.e., logger.info(f"HTML file written to: {out_path}")
Comment on lines
88
to
+92
| for json_path in model_files: | ||
| with open(json_path) as ff: | ||
| data_json = json.load(ff) | ||
|
|
||
| old_key = list(data_json["RESULTS"]["model"].keys())[0] | ||
| results_block = data_json.get("RESULTS", {}) |
forsyth2
reviewed
Jun 19, 2026
forsyth2
left a comment
forsyth2
left a comment
Collaborator
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 |
|
|
||
| assert captured["region"] == "ocean" | ||
| assert captured["var_list"] == ["pr"] | ||
| assert all(values.shape == (1, 2) for values in captured["data_dict"].values()) |
Collaborator
There was a problem hiding this comment.
From Claude:
The shape (1, 2) is implicit — a brief comment explaining what the dimensions represent (e.g. 1 model row, 2 columns?) would help future readers understand what's being asserted.
Contributor
Author
There was a problem hiding this comment.
this is also a smoke test, so maybe it is not a problem?
Comment on lines
+433
to
+436
| Path(cfg("out_dir", ".")).mkdir(parents=True, exist_ok=True) | ||
| out_path = os.path.join(cfg("out_dir", "."), "methodology.html") | ||
| Path(out_path).write_text(rendered_html) | ||
| print(f"HTML file written to: {cfg('out_dir')}") | ||
| logger.info(f"HTML file written to: {cfg('out_dir')}") |
Collaborator
There was a problem hiding this comment.
i.e., logger.info(f"HTML file written to: {out_path}")
Comment on lines
+637
to
+640
| Path(cfg("out_dir", ".")).mkdir(parents=True, exist_ok=True) | ||
| out_path = os.path.join(cfg("out_dir", "."), "diag_data.html") | ||
| Path(out_path).write_text(output_html) | ||
| print(f"HTML file written to: {cfg('out_dir')}") | ||
| logger.info(f"HTML file written to: {cfg('out_dir')}") |
Collaborator
There was a problem hiding this comment.
i.e., logger.info(f"HTML file written to: {out_path}")
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.