Conversation
HYDROGYM_ENGINEERING_AUDIT_v2.md: full audit + design + task list (findings, target architecture, Safety Protocol, phased Task List, Definition of Done). HYDROGYM_AUDIT_V2_AGENT_PROMPT.md: orientation prompt for the coding agent implementing the audit on this branch — pins the per-backend validation matrix (MAIA/JAX/JAX-Fluids in GPU containers, Firedrake/ Nek5000 in CPU containers), the identical-results requirement (interface-only change), and the commit/continuous-testing discipline. Co-Authored-By: Claude Code <noreply@anthropic.com> (cherry picked from commit 20165b82efee4261ff640280f8d2a27d2dd5d25a)
Phase 0 safety net, Task 0.2: `PDEBase`/`TransientSolver`/`FlowEnv` had zero direct test coverage (every existing test exercises them only through the full Firedrake PDE stack). This adds test/test_core.py, which exercises the shared core abstraction with trivial mock implementations: - MockFlow(PDEBase): 1-D scalar state, q <- q + dt*u, objective q^2 - MockSolver(TransientSolver), MockActuator(ActuatorBase), RecordingCallback - 49 tests covering: construction (mesh/init/reset ordering), restart (string / list / invalid type), reset semantics, set_control, advance_time, solve() in both num_steps and t_span modes (incl. the exactly-one-of ValueError), collect_rewards=True, callback invocation + interval + close(), FlowEnv construction/spaces, deprecated num_sim_substeps_per_actuation and reward_aggreation_rule warnings, num_substeps/reward_aggregation validation errors, single- and multi-substep step() rewards (mean/sum/median, hand-computed values), truncation vs max_steps, terminated always False, multi-checkpoint initial-state handling with seeded reset selection, and render/close delegation. Validation evidence (Safety Protocol rule 3): Bare venv: Python 3.12.3, gymnasium 1.3.0, numpy 2.5.2, pytest 9.1.1 ONLY (no Firedrake/MPI/GPU) -- acceptance criterion for this task. $ PYTHONPATH=. python -m pytest test/test_core.py -v => 49 passed in 0.15s No existing code touched; purely additive. (cherry picked from commit 49a047725309ad35e04881c8bfd4681f24861f33)
…r (audit Task 0.1)
Finding 4: no GitHub Actions workflow ever ran `pytest` -- build.yml only
does `poetry build`. This adds .github/workflows/test.yml, the Phase 0
regression net from HYDROGYM_ENGINEERING_AUDIT_v2.md: nothing else from
the implementation plan merges before it is green.
Design:
- devcontainer up --config .devcontainer/firedrake-test.devcontainer.json
(the repo's own petsc+firedrake features, the SAME definition used for
the local validation containers -- not the stale third-party Docker
Hub images README points at; that mismatch is Task 1.3's problem)
- devcontainers/cli pinned at 0.88.0 (same version as the local
validation runs)
- checkout with lfs: true -- meshes (*.msh, *.re2, ...) are LFS
- disk-cleanup step first: the base image carries the NVHPC/CUDA stack
(~17GB+) and hosted runners start disk-constrained
- timeout-minutes: 360; heavy source builds (PETSc 3.24 + OpenBLAS +
Firedrake at pinned commits) mean 1.5-3h/run is expected for the first
iteration -- per the audit this is acceptable to start with; the
obvious future optimization is publishing the built image to GHCR and
pulling it here instead of rebuilding
- job runs `python -m pytest . -x --durations=10` from /workspace/test
inside the built container
ACCEPTANCE CRITERIA STATUS (not silently skipped -- deferred by decision):
The audit's break-a-test validation (push a commit, watch CI go red,
revert, watch it go green) is DEFERRED at the user's explicit request
("we don't need to push so far... the tests are just lint, ruff, etc
tests"). The workflow is instead validated locally to the extent
possible without pushing: the firedrake-test container builds and runs
the suite locally from the same definition (Task 0.3 baseline, run in
this same container definition), and the devcontainer CLI invocation
mirrors the local build_all_containers.sh usage. Push validation should
happen whenever this branch is first pushed.
(cherry picked from commit d9bef299ed45622477e378cb89feae7588af4a3a)
…rom CI (audit Task 0.3) Task 0.3: record the pre-change state of the test/ suite in the firedrake-test container (the same definition CI uses). Baseline (2026-09-03, commit 7c08bc5 tree + grad tests excluded): pytest test/ (minus grad tests): 33 failed, 54 passed, 5 skipped grad tests (test_cavity_grad/test_pinball_grad/test_step_grad): 3 COLLECTION ERRORS -- ModuleNotFoundError: firedrake_adjoint Full log: .devcontainer/test-logs/firedrake-baseline-2026-09-03.txt Failure characterization (spot-checked with --tb=long): the failing tests (cavity/cyl/pinball/step) die inside Firedrake internals at `to_element.dual_evaluation(fn, coordinate_mapping)` -> "ValueError: too many values to unpack (expected 2)" during FiredrakeFunctionSpace setup. Consistent with version skew in the pinned install (firedrake 2025.11.0.dev0 vs firedrake-fiat 2026.5.0.dev0 -- the Fiat build is a much newer dev snapshot than the Firedrake build). PRE-EXISTING at baseline, not caused by any audit change; the audit's Phase 0 intent is precisely to surface this instead of leaving it invisible. Fixing these 33 failures is NOT part of this branch's task list; they now have a recorded reference point. test/README.md has said "The gradient tests are currently not run" all along -- the baseline confirms they cannot even be collected in the current Firedrake venv. DISCREPANCY HANDLED: the audit's literal CI command (plain `pytest . -x`) would make every CI run red on this pre-existing condition, so the committed workflow now passes --ignore=test_cavity_grad.py --ignore=test_pinball_grad.py --ignore=test_step_grad.py, mirroring the documented pre-existing state. If firedrake_adjoint is later provided to the venv, drop the --ignore flags. IMPORTANT BASELINE IMPLICATION FOR CI: with the grad tests excluded, CI would still be RED today (33 pre-existing failures). That is the honest pre-change state the audit asked to record; the phase-0 net's job is to keep that number from growing, not to pretend it is zero. Validation evidence: baseline run command + full pytest output captured in .devcontainer/test-logs/firedrake-baseline-2026-09-03.txt (951s wall in the container). Post-change spot checks all reproduce the same baseline numbers: no new failures introduced by Tasks 2.13/2.14/2.15/ 2.10-2.12 (their new tests pass: 9 + 5 + 4 + 8 + 3; unit regressions 53 core/lazy-loader + 9 nek pass; the 33 baseline failures are untouched Firedrake-solver code paths). (cherry picked from commit 015a038dfe3ee222e404fe09b9652d49780bdaee)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
devcontainer CLI 0.88.0 rejects --config pointing at any file not literally named "devcontainer.json" or ".devcontainer.json" -- "firedrake-test.devcontainer.json" doesn't match, so every single run of this workflow has been failing at container creation: Error: Filename must be devcontainer.json or .devcontainer.json This went unnoticed because every local verification this session used --override-config (via build_all_containers.sh), which has no such restriction -- confirmed by reproducing the exact failure locally with --config, then confirming --override-config starts the build cleanly with the identical CLI version. --override-config is also already the flag used for full-cpu-stack/full-gpu-stack local validation throughout this branch's work, so this brings CI in line with what was actually tested, not just documented as tested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…gpu needs them Every devcontainer config sharing base/Dockerfile.base -- including firedrake-test, petsc-test, nek5000-test, maia-cpu-test and full-cpu-stack -- was unconditionally building a full NVHPC/HPCX-toolchain copy of Parallel-NetCDF, Parallel HDF5, and FFTW3 into /opt/maia_deps, in addition to the GNU-toolchain copies into /opt/maia_deps_gnu right after it. Checked which features actually load which copy: petsc/install.sh, firedrake/install.sh, and maia-cpu/postCreateCommand.sh all export HDF5_HOME/LD_LIBRARY_PATH pointing exclusively at /opt/maia_deps_gnu; only DEVCONTAINER.cmake (materialized by maia-gpu/postCreateCommand.sh for the real NVHPC-toolchain GPU build) references the plain /opt/maia_deps paths. So every CPU-only config was building an entire unused library stack from source on every run -- roughly half of the ~1h+ Firedrake CI build time, confirmed by comparing the "Firedrake test suite" job's step timings against the from-source build blocks below -- and, worse, a source of outright CI flakiness: one run's NVHPC-toolchain HDF5 build crashed with `nvc-Fatal- .../llc TERMINATED by signal 6` (github.com/dynamicslab/hydrogym/actions/ runs/34091138163/job/101644668541), unrelated to any test/solver code and entirely avoidable since that build was never needed for this config. Fixed: added ARG BUILD_GPU_LIBS=false, wrapped the three NVHPC-toolchain build RUN blocks in `if [ "$BUILD_GPU_LIBS" = "true" ]`, and set BUILD_GPU_LIBS=true only in the three configs that use ./features/maia-gpu (maia-gpu-test, full-gpu-stack, the top-level devcontainer.json). The GNU-toolchain copies (used by every CPU-only config) are unaffected. Verified: `docker build` syntax-checked via `bash -n` on the extracted conditional shell blocks; every *.devcontainer.json re-parsed with python's json module to confirm the added build arg didn't break any config; full ruff/isort/codespell pass clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
2 participants
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.
First of a sequential stack of draft PRs splitting up the
hydrogym-audit-v2engineering audit branch into reviewable chunks. Each PR in the stack targets the previous one (this one targetsdevcontainer-update, #266).Scope (Phase 0 — safety net before making any changes)
HYDROGYM_ENGINEERING_AUDIT_v2.md— the audit plan this whole stack implementshydrogym/core.py's shared contract (PDEBase/TransientSolver/FlowEnv), Task 0.2Verified
ruff check .cleantest/test_core.pyparses and its assertions were exercised live in the devcontainer during the audit (see the audit doc's own verification sections)🤖 Generated with Claude Code