Address Copilot review: curl retries, missing config vars, GCC install - #3
Closed
cenamiller wants to merge 91 commits into
Closed
Address Copilot review: curl retries, missing config vars, GCC install#3cenamiller wants to merge 91 commits into
cenamiller wants to merge 91 commits into
Conversation
- Add build-mpas and run-mpas composite actions - Add validate-logs action with compare_logs.py - Add test-case configs and reference logs for 240km and 120km - Add coverage.yml workflow for Codecov integration - Add test-ga-nogpu.yml and CIRRUS workflow updates - Download test case archives from NCAR/mpas-ci-data at CI runtime Co-authored-by: Cursor <cursoragent@cursor.com>
- Run 240km test case with both 1 and 4 MPI ranks in test-ga-nogpu.yml - Add decomposition test: compare 4-proc output against 1-proc output to verify results are independent of domain decomposition - Add --decomposition-test and --filter flags to compare_logs.py - Update validate-logs action with log-filter input Co-authored-by: Cursor <cursoragent@cursor.com>
Based on Price-Broncucia et al. (2025), doi:10.5194/gmd-18-2349-2025 - Add ect-test.yml: runs 3 perturbed 120km members and validates against a pre-built ensemble summary file using PyCECT. Intended for code changes not expected to alter science. - Add ect-ensemble-gen.yml: generates 200 perturbed ensemble members (batched 10/job) and produces a summary file via pyEnsSumMPAS. Manual-only, for major version/science changes. - Add perturb-ic action: applies O(10^-14) perturbation to theta field in IC file. Placeholder until native MPAS-A perturbation code is re-added. - Add ECT parameters to 120km config.env Co-authored-by: Cursor <cursoragent@cursor.com>
The tar tzf | head -1 pipeline triggers SIGPIPE (exit 141) under bash -o pipefail when head closes the pipe. Add || true and a fallback directory detection, matching the pattern already used in run-mpas/action.yml. Co-authored-by: Cursor <cursoragent@cursor.com>
The AlmaLinux 9 container does not have pip on PATH. Bootstrap it with python3 -m ensurepip, then use python3 -m pip for installs. Co-authored-by: Cursor <cursoragent@cursor.com>
AlmaLinux 9 containers only provide python3 on PATH. Co-authored-by: Cursor <cursoragent@cursor.com>
The run job was skipped entirely when any build matrix entry failed because it lacked an `if: !cancelled()` condition. Now it proceeds for whichever builds succeeded, with continue-on-error on the artifact download so individual entries gracefully skip when their build artifact is missing. The validate job also handles the case where no logs are available. Matches the pattern already used in CIRRUS_build_run240.yml. Co-authored-by: Cursor <cursoragent@cursor.com>
gfortran exits with code 2 on IEEE_UNDERFLOW/DENORMAL warnings even when the run completes successfully. Wrap mpirun with set +e/set -e so the loop can continue, and use the presence of a history file as the real success indicator instead of the exit code. Co-authored-by: Cursor <cursoragent@cursor.com>
The output stream in streams.atmosphere had output_interval="none", so no history files were ever written. Add sed to set the output stream interval to ECT_HISTORY_INTERVAL (30 min), producing 4 time slices in the 2-hour run (needed for ECT_TSLICE=3). The apparent exit-code-2 crash was actually from `ls history.*.nc | tail -1` failing under set -e + pipefail when no files existed. Fix with || true. Co-authored-by: Cursor <cursoragent@cursor.com>
Show clear markers for when perturbation starts, model run starts, and model run finishes, with wall-clock timestamps for each. Co-authored-by: Cursor <cursoragent@cursor.com>
After generating the ensemble summary, push it to NCAR/mpas-ci-data along with ect_summary_metadata.json containing MPAS version, branch, commit, resolution, ensemble size, date, and workflow run link. Requires MPAS_CI_DATA_TOKEN secret (PAT with repo scope). Gracefully skips if the secret is not configured. Co-authored-by: Cursor <cursoragent@cursor.com>
- Create .github/test-cases/ect-120km/config.env with all ECT parameters (run settings, PyCECT config, perturbation settings, summary file naming) in one place. - Remove ECT parameters from .github/test-cases/120km/config.env. - Version summary files: archived as ect-summaries/<prefix>_v<ver>_<date>.nc in mpas-ci-data; current copy kept as <prefix>.nc for ect-test.yml. - Both ECT workflows now source from ect-120km/config.env. Co-authored-by: Cursor <cursoragent@cursor.com>
The EOF heredoc terminator at column 1 inside a YAML block scalar was parsed as a YAML key, breaking the workflow file. Replace the heredoc with string concatenation. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
OpenMPI refuses to run more processes than available CPU slots without --oversubscribe. GitHub Actions runners may expose fewer cores than requested ranks. Also adds CPU/RAM diagnostics to help debug the separate gcc/mpich3 heap corruption issue. Co-authored-by: Cursor <cursoragent@cursor.com>
Both combinations crash with heap corruption during parallel mesh bootstrapping (glibc malloc assertions). This appears to be a container library incompatibility rather than an MPAS code issue. 1-proc runs for these combinations continue to work fine. Co-authored-by: Cursor <cursoragent@cursor.com>
Let gcc/mpich3 and nvhpc/openmpi 4-proc runs fail visibly rather than hiding them from the matrix. Co-authored-by: Cursor <cursoragent@cursor.com>
gcc/mpich3 crashes with heap corruption at 4 MPI ranks. gcc/openmpi works reliably with --allow-run-as-root --oversubscribe flags. Co-authored-by: Cursor <cursoragent@cursor.com>
…ble size pyEnsSumMPAS exits 0 even when it fails (e.g. too few members for the number of variables). Add file existence check after generation and raise the minimum ensemble count to 63 (matching the ~63 output variables PyCECT needs to analyze). Co-authored-by: Cursor <cursoragent@cursor.com>
Remove old workflows and shell scripts that have been replaced by
the current composite actions and workflow structure:
Deleted workflows:
- smoketest.yml (hardcoded feature-ci branch)
- build_run240.yml (hardcoded feature-ci branch)
- CIRRUS_build_run240.yml (replaced by test-cirrus-nvhpc.yml)
Deleted shell scripts (replaced by composite actions):
- build_mpas.sh -> actions/build-mpas
- run_mpas_240km.sh -> actions/run-mpas
- run_mpas.sh -> actions/run-mpas
- build_mpas_coverage.sh (unused)
- interrogate_env.sh (only used by deleted workflows)
- build_common.cfg (only sourced by deleted scripts)
Other cleanup:
- Remove 240km.tar.gz from tracking (belongs in mpas-ci-data)
- Remove duplicate reference log from validation/ (canonical
copy is in test-cases/240km/)
- Update coverage.yml branch triggers (feature-ci -> develop)
- Add .DS_Store, test archives, editor dirs to .gitignore
- Add AGENT_GUIDE.md for AI assistant context
Remaining active workflows:
- test-ga-nogpu.yml (GitHub Actions build/run)
- test-cirrus-nvhpc.yml (CIRRUS NVHPC GPU/CPU)
- ect-ensemble-gen.yml (ECT ensemble generation)
- ect-test.yml (ECT validation)
- coverage.yml (code coverage)
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Three combined optimizations to shrink history files from ~556MB to an estimated ~50-80MB per member (with compression): 1. Reduce time slices: set output_interval = run_duration so MPAS writes only at t=0 and t=2h instead of every 30 minutes (5 -> 2). 2. Remove 22 unneeded variables: Ertel PV diagnostics (16 expensive 3D fields), PV tendency terms, edge velocity, integers, and time metadata. List maintained in ect_excluded_vars.txt. 3. Compress + extract: trim_history.py extracts only the target time slice and applies NetCDF4 deflation before artifact upload. At ~70MB per member, 200 members = ~14GB, fitting within the ~30GB free disk on standard GitHub Actions runners. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid any risk of data alteration in the ECT validation pipeline. Deflation is technically lossless but adds unnecessary complexity. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Centralizes test case download + extraction into a reusable action at .github/actions/download-testcase/. Replaces duplicated curl/tar/mv blocks in run-mpas, ect-ensemble-gen, and ect-test workflows. Also adds RESOLUTION and DATA_REPO to 240km/config.env for consistency with the ect-120km config. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…d_mpas.sh The build_mpas.sh script was removed during workflow cleanup but build-mpas/action.yml still referenced it. Inlines the compiler-to-make-target mapping and make command directly in the action. Co-authored-by: Cursor <cursoragent@cursor.com>
Each ect-validate matrix job now writes its result (PASSED/FAILED/ SKIPPED/ERROR) to a small artifact. A new ect-summary job collects all results and renders a single markdown table to the GitHub Actions job summary, showing all compiler/MPI/IO/rank combinations at a glance. Made-with: Cursor
Add actions/cache to avoid re-downloading large test case archives
(120km ~280MB, 240km ~77MB) from GitHub LFS on every job. The cache
key is testdata-v1-{archive} — bump the v1 prefix to bust the cache
when test data is updated. On cache hit, only the fast local tar
extraction runs.
Made-with: Cursor
All workflow_dispatch workflows now accept mpas-repository and mpas-ref inputs, allowing CI to build and test MPAS source from any public fork (e.g. MPAS-Dev/MPAS-Model) using this repos CI infrastructure. Build jobs use a two-step checkout: MPAS source first, then overlay .github/ from MPAS-Model-CI so composite actions resolve correctly. Made-with: Cursor
Adds repository layout tree, branch structure, workflow overviews, MPI matrix values and mapping logic, ECT job structure details, test data caching, CLI examples, development history, and expanded pitfalls list including numpy compat and NVHPC portability. Made-with: Cursor
Centralizes the PyCECT version in ect-120km/config.env as PYCECT_TAG so all four ECT workflows clone the same pinned release instead of HEAD. Prepares for more frequent upstream PyCECT updates. Made-with: Cursor
Layer 2 — ECT Modularity: - Create validate-ect action encapsulating PyCECT install, clone, summary download, validation, and enriched result file output - Create ect-summary action with auto-discovered columns from key=value dimension metadata in result files - Refactor ect-validate/ect-summary jobs in test-ga-nogpu.yml, test-cirrus-nvhpc.yml, and ect-test.yml to use new actions (net -240 lines of duplicated inline logic) Layer 1 — Docs Generator Rewrite: - Restructure generate_ci_docs.py for contributor audience: lead with what CI checks and why, resolve matrix values into human- readable counts, add "How to read CI results" section, move developer details into collapsible reference - Filter template expressions from container image output - Use MkDocs admonitions and collapsible sections Update AGENT_GUIDE.md with new actions and repo layout. Made-with: Cursor
Draft materials for sharing current MPAS CI status with NOAA/GSL collaborators and for the upcoming ISS talk. Made-with: Cursor
MPAS writes an initial-state time slice before the main loop for cold-start (non-restart) runs. The old hardcoded tslice=0 grabbed that un-integrated state instead of the 6-hour forecast, causing PyCECT to see near-zero ensemble variability. The action now auto-detects the last time slice in each history file and passes it to trim_history.py, which also gains support for negative indexing (e.g. -1 = last). Made-with: Cursor
Adapted from ESCOMP/CAM-SIMA MPAS dynamical core CI. Runs fortitude-lint on src/ with --exit-zero (non-blocking) to establish a baseline. Excludes src/external and WRF physics. Triggered on PRs and pushes that touch src/ or linting config. Made-with: Cursor
Adapted from ESCOMP/CAM-SIMA MPAS dynamical core CI. Sets up: - CMake build system for testable MPAS procedures (tests/) - pFUnit test framework with GCC 12/13/14 matrix - Starter test suite for mpas_spline_interpolation: linear interpolation, cubic spline coefficients, node reproduction, accuracy against sin(x), and integration of linear/sine functions The test library builds only standalone modules with minimal dependencies (currently mpas_kind_types + mpas_spline_interpolation). More source files and tests can be added incrementally. Made-with: Cursor
gfortran treats .F as fixed-form Fortran, but MPAS sources are free-form with CPP directives. Add -cpp -ffree-form (GNU) and -fpp -free (Intel) to the testable_procedures library. Also update fortitude linting to use reviewdog for PR-scoped annotations (only flags issues on changed lines). Made-with: Cursor
Reverts fortran-linting.yml and fortitude_config.toml to their original versions; reviewdog integration was not desired. Made-with: Cursor
Prepend docker.io/ to all ncarcisl container image references. Required by upcoming Cirrus cluster upgrade that no longer assumes docker.io as the default registry. Made-with: Cursor
Move the restart file push from the generate-summary job to the spinup job so it persists even if later steps (e.g. pyEnsSumMPAS) fail. Also cache the restart with actions/cache so ect-test and cirrus workflows can skip the 1.2 GB download when a cache hit is available. Made-with: Cursor
MPICH4 F08 bindings call CFI_is_contiguous at runtime, which crashes with unsupported array rank on NVHPC versions before 2024 due to incomplete CFI support. Force MPAS_MPI_F08=0 so MPAS uses the standard mpi module instead, bypassing the broken code path entirely. See: pmodels/mpich#6505 Made-with: Cursor
in prep for PR to master branch Preparing for PR for master branch
MPICH_GPU_SUPPORT_ENABLED=0 alone is insufficient to prevent yaksa from probing CUDA during MPI_Init, causing SIGSEGV on CUDA runners. MPIR_CVAR_ENABLE_GPU=0 disables GPU initialization at a lower level. OpenACC GPU computation is unaffected. See: pmodels/mpich#6523 Made-with: Cursor
The nvhpc-mpich-cuda and nvhpc-mpich3-cuda container images do not set LD_LIBRARY_PATH in config_env.sh, unlike the OpenMPI variant. Without it the NVHPC runtime cannot find libcuda.so injected by the Kubernetes NVIDIA device plugin, causing cuInit to fail with error 34. Set a fallback LD_LIBRARY_PATH when the container does not provide one. Made-with: Cursor
The O(1e-14) perturbation magnitude from Price-Broncucia et al. (2025) is below single-precision machine epsilon (~1.2e-7), causing all ensemble members to produce identical results. Build MPAS with PRECISION=double for all ECT workflows so perturbations survive the float representation. Shorten ECT member runs from 6h (30 steps) to 2h36m (13 steps at the 12-min default dt), just past the ~12 step normality transition the paper identifies. The Cirrus workflow gets a separate build-ect job so regular 240km runs remain single precision. Made-with: Cursor
- Add compiler and mpi-impl workflow_dispatch inputs (default: nvhpc/mpich) - Derive container image from inputs so all jobs use consistent toolchain - Fix metadata to record actual MPAS source repo/branch/commit instead of workflow trigger context (GITHUB_REF_NAME/GITHUB_SHA) - Remove dead ECT_RAW_TSLICE variable - Use dynamic ECT_SUMMARY_PREFIX based on RESOLUTION variable - Update perturbation magnitude to 1e-7 for testing - Clarify run duration and output interval comments in config.env Made-with: Cursor
The env context is not available in job-level container.image or name fields. Replace env.CONTAINER_IMAGE, env.COMPILER, and env.MPI_IMPL with inline inputs expressions. Made-with: Cursor
…n failure - Add validation step that checks each history file with netCDF4 before passing to pyEnsSumMPAS, removing corrupted files from artifact download - Split cleanup into two steps: always delete build artifacts, but only delete ensemble history artifacts after generate-summary succeeds. This allows re-running the summary job after transient download failures. Made-with: Cursor
Record compiler, MPI implementation, precision, run duration, perturbation magnitude, and perturbation variable in both the ect_summary_metadata.json pushed to mpas-ci-data and the workflow summary log. Filenames stay stable for consumers; the metadata JSON provides full traceability of how each summary was generated. Made-with: Cursor
Made-with: Cursor # Conflicts: # .github/test-cases/240km/reference_log.atmosphere.0000.out
- Add --retry 5 --retry-delay 5 to curl in validate-ect (consistency with other download steps) - Add RESOLUTION and DATA_REPO to 120km/config.env (required by download-testdata action) - Add GCC toolchain install step to unit-tests.yml (gcc-14 not guaranteed on ubuntu-latest) Made-with: Cursor
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
1 participant
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
Follow-up fixes from Copilot review comments on PR #2. Addresses 3 of 4 suggestions (the 4th — changing
ECT_PERTURB_MAGNITUDEto 1e-14 — was intentionally left as-is since the current value of 1e-5 is correct for single-precision runs).validate-ect/action.yml: Add--retry 5 --retry-delay 5to the curl download of the ensemble summary file, consistent with all other download steps in the CI.120km/config.env: Add missingRESOLUTION=120kmandDATA_REPO=NCAR/mpas-ci-datavariables required by thedownload-testdataaction.unit-tests.yml: Addapt-get installstep for the GCC toolchain sincegcc-14/gfortran-14are not guaranteed onubuntu-latest.Test plan
unit-tests.ymlpasses across GCC 12/13/14 after mergeMade with Cursor