Repository navigation
Conversation
|
@copilot Please implement the following fix directly on this PR’s branch. Fix the When upstream The last job depends on the blocked PCMDI jobs. The framework keeps waiting until its four-hour timeout. Update the existing polling loop to cancel this run’s jobs reporting Once none of the run’s jobs remain queued or running, continue through failure reporting and print the output/log locations. The run should still report failure. Keep the fix small and reuse existing scheduler helpers. Add focused mocked tests for direct cancellation, cascading cancellation, and preservation of healthy/unrelated jobs. Run the relevant tests and summarize the changes. |
Implemented in |
|
@copilot Please make sure tests don't require external directories. It appears your latest commit, 7a151c1, has introduced a |
Fixed in |
|
Saw a banner message on #886 pointing to https://docs.github.com/en/pull-requests/get-started/about-stacked-prs. Perhaps that might be useful for breaking this PR into reviewable chunks. |
|
@copilot Please implement the following fix directly on this PR’s branch. In tests/complete_run, make configuration file-driven and add dependency pinning and Unified options.
Tests must go through the real CLI (automation.main([...])). Update |
Implemented in |
|
The latest Copilot commit, f88183f, addresses these points:
|
|
@copilot Please implement the following fix directly on this PR’s branch. Make every zppy task independently selectable and visible in results, not only plot-producing ones. Today
Add tests and update the docs and AGENTS.md. |
|
The latest Copilot commits (e4163a7, 314e34b, 060bafb) addresses these points:
|
|
@copilot Please implement the following fix directly on this PR’s branch. Make expected results configurable per cfg and per task, keeping the "a run is its own
Add tests and update update_expected_results.rst, automated_test.rst, and the AGENTS.md |
|
The latest Copilot commits (8fa4546, 1e060a0, 91bdf51) addresses these points:
|
Add Claude-generated rst file documenting the internal workings of the new testing framework.
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
b179786 to
2e8b63a
Compare
|
Rebased off the latest |
|
Currently running this code configured to match the 2026-10-09 E3SM Unified 1.14.0rc3 Chrysalis test, for comparison. |
E3SM Unified 1.14.0rc3 Chrysalis test -- using refactored test frameworkTrying to follow the 2026-10-09 E3SM Unified 1.14.0rc3 Chrysalis test as close as possible. Set up the testcd ~/ez/zppy
git status
# On branch streamline-complete-run-test-updated
# nothing to commit, working tree clean
git log --oneline | head -n 37
# b1797863 Mark report test as Unified-backed
# ...
# 80035514 Replace the bash complete run driver with tests/complete_run
# 64bffc9f Merge pull request #815 from E3SM-Project/zppy_pcmdi_enhancement
# Good, matches the 36 commits of https://github.com/E3SM-Project/zppy/pull/883/commits
# BUT we do need to rebase off the latest `main` to get to v3.3.0rc1
git fetch upstream main
git rebase upstream/main
git push -f upstream streamline-complete-run-test-updated # Update the branch on GitHub
# Good, GitHub Actions pass
# The launching env only needs Python and mache. The rc3 Unified env has both.
bash
source /lcrc/soft/climate/e3sm-unified/test_e3sm_unified_1.14.0rc3_chrysalis.sh
python -m tests.cfg_suite.promote --machine chrysalis showThese expected results differ from what we used for the other test. We'd need to update the baseline at the same time for both to actually compare diffs properly. But for now, this will do. cp ~/ez/zppy/tests/cfg_suite/cfg-suite.example.cfg ~/ez/zppy_cfg_suite_rc3.cfg
emacs ~/ez/zppy_cfg_suite_rc3.cfg
# Update the following:
# [cfg_suite]
# machine = chrysalis
# account = e3sm
# label = unified_rc3
# repo_root = /lcrc/group/e3sm/ac.forsyth2/zppy_pr876_test_dirs/
# branch = zppy=streamline-complete-run-test-updated
# env_type = zppy=unified,e3sm_diags=unified,e3sm_to_cmip=unified,mpas_analysis=unified,zppy_interfaces=unified
# unified_load_script = /lcrc/soft/climate/e3sm-unified/test_e3sm_unified_1.14.0rc3_chrysalis.sh
cd /lcrc/group/e3sm/ac.forsyth2/zppy_pr876_test_dirs/zppy
git status
# On branch streamline-complete-run-test-updated
# nothing to commit, working tree clean
git fetch upstream streamline-complete-run-test-updated
git reset --hard upstream/streamline-complete-run-test-updated
screen
ulimit -s unlimited # needed for MPAS-Analysis inside screen
cd /lcrc/group/e3sm/ac.forsyth2/zppy_pr876_test_dirs/zppy
cat ~/ez/zppy_cfg_suite_rc3.cfg # Looks good
time python -m tests.cfg_suite.automation --config ~/ez/zppy_cfg_suite_rc3.cfg \
2>&1 | tee cfg_suite_20261013_run1.log
# Ctrl-A D to detach
screen -ls
# 3735093.pts-24.chrlogin1 (Detached)
tail -f cfg_suite_20261013_run1.log
# Picking back up the next day
cd /lcrc/group/e3sm/ac.forsyth2/zppy_pr876_test_dirs/zppy
tail cfg_suite_20261013_run1.logscreen -Rexit # Exit screen
ls -t /lcrc/group/e3sm/public_html/zppy_complete_run/runs | head -n 3
# Tag is: 20261013_174427_unified_rc3
cat /lcrc/group/e3sm/public_html/zppy_complete_run/runs/20261013_174427_unified_rc3/cfg-suite-report.md
# Pasted below in the "Generated Markdown Report" section.
ls /lcrc/group/e3sm/public_html/zppy_complete_run/runs/20261013_174427_unified_rc3/index.html
# Good, the web page existsWeb page is visible at: https://web.lcrc.anl.gov/public/e3sm/zppy_complete_run/runs/20261013_174427_unified_rc3/ Generated Markdown Reportzppy cfg suite test -- 20261013_174427_unified_rc3
What was tested
Baseline by cfg/task
Changes since the baselineThe listed zppy commits describe the worktree under test; they may not be included in the E3SM-Unified release used to run zppy.
|
| Test | Result |
|---|---|
tests/integration/test_last_year.py |
pass |
tests/integration/test_bash_generation.py |
FAIL |
tests/integration/test_campaign.py |
FAIL |
tests/integration/test_defaults.py |
FAIL |
tests/integration/test_bundles.py |
FAIL |
Image checker
- SLURM job:
1308020 - Terminal state:
FAILED - Exit code:
1:0 - Summary source:
final
| Severity | Images | Reviewable |
|---|---|---|
| IDENTICAL | 40 | no |
| NEGLIGIBLE | 14734 | no |
| MINOR | 1549 | yes |
| MODERATE | 529 | yes |
| MAJOR | 519 | yes |
| STRUCTURAL | 4 | yes |
| MISSING | 157 | yes |
2758 images need review.
Full summary: /lcrc/group/e3sm/public_html/zppy_complete_run/runs/20261013_174427_unified_rc3/test_images_summary_20261013_174427_unified_rc3.md
Review page, with a table across every package: /lcrc/group/e3sm/public_html/zppy_complete_run/runs/20261013_174427_unified_rc3/index.html
Next steps
Failures above are for human review. A cfg suite never promotes expected results; that remains the separate, explicit update_*_expected_files_<machine>.sh step.
Image check summary
cat /lcrc/group/e3sm/public_html/zppy_complete_run/runs/20261013_174427_unified_rc3/test_images_summary_20261013_174427_unified_rc3.mdSummary of test results
| Test name | Total images | Correct images | Identical | Cosmetic only | Missing | Needs review (excluding missing) | Total needing attention | Severity |
|---|---|---|---|---|---|---|---|---|
| bundles_e3sm_diags | 1762 | 1703 | 3 | 1700 | 0 | 59 | 59 | 28 moderate, 31 minor, 1700 negligible, 3 identical |
| bundles_global_time_series | 3 | 3 | 0 | 3 | 0 | 0 | 0 | 3 negligible |
| bundles_ilamb | 388 | 144 | 0 | 144 | 0 | 244 | 244 | 42 major, 44 moderate, 158 minor, 144 negligible |
| comprehensive_v2_e3sm_diags | 3806 | 3697 | 3 | 3694 | 1 | 108 | 109 | 1 missing, 31 major, 56 moderate, 21 minor, 3694 negligible, 3 identical |
| comprehensive_v2_mpas_analysis | 856 | 613 | 4 | 609 | 0 | 243 | 243 | 63 moderate, 180 minor, 609 negligible, 4 identical |
| comprehensive_v2_global_time_series | 12 | 12 | 0 | 12 | 0 | 0 | 0 | 12 negligible |
| comprehensive_v2_ilamb | 776 | 282 | 0 | 282 | 0 | 494 | 494 | 84 major, 88 moderate, 322 minor, 282 negligible |
| comprehensive_v3_e3sm_diags | 5369 | 5056 | 3 | 5053 | 0 | 313 | 313 | 227 major, 53 moderate, 33 minor, 5053 negligible, 3 identical |
| comprehensive_v3_mpas_analysis | 1280 | 903 | 6 | 897 | 0 | 377 | 377 | 71 moderate, 306 minor, 897 negligible, 6 identical |
| comprehensive_v3_global_time_series | 1404 | 1398 | 0 | 1398 | 0 | 6 | 6 | 6 minor, 1398 negligible |
| comprehensive_v3_ilamb | 837 | 348 | 12 | 336 | 0 | 489 | 489 | 99 major, 104 moderate, 286 minor, 336 negligible, 12 identical |
| comprehensive_v3_livvkit | 392 | 303 | 8 | 295 | 0 | 89 | 89 | 7 moderate, 82 minor, 295 negligible, 8 identical |
| comprehensive_v3_pcmdi_diags | 647 | 312 | 1 | 311 | 156 | 179 | 335 | 156 missing, 4 structural, 36 major, 15 moderate, 124 minor, 311 negligible, 1 identical |
Legend: Missing means an expected image was not produced. Needs review excludes missing images. Total needing attention includes missing images and other reviewable differences.
Test approach comparison 3: E3SM Unified 1.14.0rc3 testing
Below is Claude's analysis based on the output links above (and text pasted from https://web.lcrc.anl.gov/public/e3sm/zppy_complete_run/runs/20261013_174427_unified_rc3/). I need to spend more time reviewing this. Comparison: current vs. experimental/refactored test framework (E3SM-Unified 1.14.0rc3, Chrysalis)This compares the current test framework (the one actually in use) with the experimental/refactored test framework, both run against Unified 1.14.0rc3. Known differences between the two runs
Things the current framework does that the refactored one doesn't
Things the refactored framework does that the current one doesn't
Discrepancies in how the two handle the same thingImages needing review (current framework: "Needs review"; refactored: "Total needing attention", which includes missing images):
"No environment export or package listing for this run; dependency changes could not be compared"This is probably related to testing a Unified RC, but the reports alone don't confirm the cause.
Does the refactored framework's web page match its markdown report?Where they overlap, yes. The totals agree: 17,532 images, 157 missing, 2,601 needing review excluding missing, and 2,758 total needing attention. The per-test rows in But the web page is not just a prettier version of the markdown report, because each contains content the other doesn't:
The markdown report's image section only has aggregate severity totals, so the per-cfg/task breakdown exists only on the web page. The web page content was pasted as plain text, so links, images and styling weren't checked. |
See #871 (comment) |
This PR should not be merged directly (I've made it a draft PR for easy review). It consists of my suggested updates to #876. Once Jill and I agree on any changes here, we can cherry-pick the relevant commits to that PR.
List of suggested features from #876:
Noted in #876 (comment):
DependencyNeverSatisfiedjob handling -- e.g., cancelling if a job will literally never be able to run (and only looking at jobs related to this run)e3sm_to_cmipfailing due tosrunerror #875 is affectinge3sm_to_cmip, so we don't really need to run anything beyond that).Noted in Test approach comparison 1: NCO error in e3sm_to_cmip:
python -m tests.complete_run.promote --machine chrysalis show), theshafield is useful, but perhaps asha_datefield would be good as well, so it's easy to immediately identify how old it is.main_branch_testing,complete_runaren't always reflective of what we're actually testing. The test suite is for testing various configurations.DependencyNeverSatisfiedjobs. It needs to review its list of job IDs and cancel any that haveDependencyNeverSatisfied. In this case, the one remaining job that has onlyDependencyis because it's actually waiting onDependencyNeverSatisfiedjobs. That is, it's a second-order dependency never satisified situation. So the test also has to keep cancelling jobs until none of the jobs it requested are in the queue. Then, it should continue on to print info about where to find output.e3sm_to_cmip, so there's not much of a need to run any of the plotting tasks. As mentioned above, "climo/ts/e3sm_to_cmip/tc_analysis aren't independently selectable"Noted in #876 (comment):
conda installitself automatically. Something likePIN_DEPENDENCIES: "nco<5.4.0,"This also would have been helpful for addressing the issue we saw in August re: PMP's version (Integration testing strategy for `pcmdi_diags` #848). To clarify, I do not mean testing with frozen environments which we already decided against; I mean being able to temporarily pin an old version when we've already determined it's the dependency, and not our packages, causing the issue.Noted in Test approach comparison 2: main branch testing using NCO 5.3.9:
In summary, suggestions for PR 876:
promote --machine chrysalis showand the report (Compared against baseline 20260918_unified113) both show one global baseline used for every package except zppy's own dev env. PR 871's config exposesDIAGS_EXPECTED_RESULTS_DATE,MPAS_EXPECTED_RESULTS_DATE, etc. for exactly this reason. Until this is ported, none of the diff counts between the two approaches are apples-to-apples.zppyande3sm_to_cmipare invisible in the "By package" results table. Onlye3sm_diags,ILAMB,LIVVkit,MPAS-Analysis, andzppy-interfacesget a row. There's no result/status indicator at all for the two tasks that don't produce plots.Environment<details>box on the per-check pages defaults to open (<details class='env' open>), burying the image diff grid under a long dependency list. Should default to closed.zppy_interfacesrannco 5.4.0;conda list -n test-zppy_interfaces-main-20260929_run5 ncoshows it's actually5.3.9.e3sm_to_cmipwasn't even included in the comparison (Not compared for: e3sm_to_cmip), but its live env is also correctly5.3.9. So the "This run" column looks like a snapshot taken early (likely right afterconda env create, before the manualnco<5.4.0pins were applied) that never gets refreshed before the final report is written. This needs a fix at the source — capture the environment manifest at or after task execution, not once during setup — ande3sm_to_cmipshould be included in the comparison rather than excluded, since it currently hides exactly this class of bug. (The onencorow that may genuinely be 5.4.0 iszppyitself, which doesn't call NCO, so that particular mismatch has no functional consequence — but it's a coincidence, not evidence the underlying bug is benign elsewhere.)test_images_summary_*.md's per-check "Needs review" column excludes missing images (tracked separately);index.html's "By package" table's "Images needing review" includes them. The underlying numbers are internally consistent, but this will read as a discrepancy to anyone cross-referencing the two files.From #876 (comment):
/lcrc/group/e3sm/public_html/zppy_complete_run/runshas multiple runs, and it'd be very easy for two testers to accidentally choose the same test name. I think the username should be inserted into the path so it's something likeruns/$USER/...to prevent this.$USERto avoid name collisions.From #876 (comment):
Newer features in the stack #887:
zppyitself