Uh oh!
There was an error while loading. Please reload this page.
JIT constants nested in wrapper expressions are live-rampable — reveal before substitute - #416
Merged
Merged
Conversation
… reveal before substitute (#302) A constant UWexpression listed in constants_manifest could still be baked into the generated C: the lowering substituted constants[] placeholders with a TOP-LEVEL xreplace and only then unwrapped, so a constant hidden inside a nested UWexpression never met the substitution and was folded to a literal by the unwrap. Since every Parameters.* value is template- wrapped in a UWexpression, this froze EVERY constant inside constitutive parameter expressions (broader than the issue's xi case — the reproducer shows even the 'delta pattern' freezes when it arrives via a parameter): manifest said live, C said literal, and only a full rebuild propagated new values. It also explains why rebuilds 'worked': the cache key baked the same values, so a ramp changed the hash and forced recompilation. The lowering is now three-phase, identical in codegen and cache hashing (shared _reveal_constants): 1. expand non-constant UWexpressions to fixpoint, KEEPING truly-constant atoms symbolic (the predicate is the one the manifest scan uses, so the revealed set equals the manifested set by construction); 2. xreplace manifested constants with their constants[] placeholders; 3. resolve remaining UW atoms to values. Two loud guards make this class impossible to reintroduce silently: a truly-constant atom with no manifest slot, or a manifested constant that survives to the final expression, is a RuntimeError — never a silently frozen parameter. Nested-constant ramps no longer invalidate the JIT cache (the placeholder is hashed, not the value). Regression tests (tests/test_0103_jit_rampable_constants.py): top-level and nested constants both ramp via real solves with no rebuild; the ramped solution equals a from-scratch rebuild at the same value to 1e-10; ramping back restores the base solution exactly. Level_1+tier_a gate: 445 passed with the guards active on every kernel. Unblocks the model-owned-time design recorded on #410 (time as a constants[] atom is now guard-protected against silent freezing). Closes#302 Underworld development team with AI support from Claude Code
…ponse) The adversarial review found prepare_for_cache_key swallowed reveal failures into a silent fallback that would hash constant VALUES — re-creating the recompile-per-ramp behaviour for the failing expression while codegen raised. Same input now behaves identically on both paths. Also from the review, filed separately: #415 (pre-existing Max/Min(UWexpression, number) construction crash). Underworld development team with AI support from Claude Code
lmoresi
commented
Jul 24, 2026
MemberAuthor
Adversarial review — round 1 (public record)Independent skeptic pass, live probes in the branch's own build. Instructed to REFUTE; could not. Attack 1 — sympy folding during the reveal phase: HELDEight nested-constant topologies, each ramped via .sym with REAL solves and checked against an independent literal-inlined rebuild (different kernel, separate compile): constant exponent over a field base; constant exponent with numeric base inside a wrapper; Integer*constant Mul auto-simplification; sqrt((T−1)²+δ²) regularisation; Piecewise on a field threshold; DOUBLE nesting; Max(constant, field); and Max(k1,k2) where the ramp switches the runtime branch (kernel evaluates fmax(constants[i],constants[j]) live). Every ramped answer matched truth to 1e−9. No silent bakes; the guards never needed to fire. Attack 2 — guard false positives: HELDtest_0103 + test_1018 (rotated free-slip, consistent-Newton power-law) + test_0817/0818 (ND) + test_0812 (units-active constants): 29 passed, zero guard errors. VEP stability subset: 2 passed. No spurious RuntimeError anywhere. Attack 3 — cache behavior: HELDFirst solve: exactly one new .so. Ramp nested constitutive constant: zero new .so, identical cache key, correct answer. Ramp a BC constant: same. Structural change: recompiles as it must. A fully-constant wrapper chain collapses onto the template constant's slot and inner ramps still propagate (values are re-packed per solve). Attack 4 — consistent Jacobian: HELDPower-law config with consistent_jacobian=True: ramping BOTH a nested amplitude and a nested EXPONENT (the historically-broken family), no rebuild — solution and Newton iteration count bit-identical to a fresh rebuild at the same values (vmean 0.00065017399 both, its=6 both). Attack 5 — performance: HELD20-deep nested chain (20 manifested constants): setup 0.10s / first solve 4.19s vs development's 0.26s / 4.24s. No regression. Attack 6 — BC and boundary kernels: HELDConstants in Dirichlet expressions and natural-BC fluxes are manifested, ramp live without recompile, and match truth (natural-flux ramp: exactly 3x on a linear problem). bd_residual/bd_jacobian slots get the same reveal. Findings → responses
Verdict: MERGE.Underworld development team with AI support from Claude Code |
lmoresi
commented
Jul 24, 2026
MemberAuthor
Post-rebase verification (onto development at #404): level_1+tier_a gate 449 passed, 0 failures on a clean rebuild — including #404's four new boundary-flux/topography tests running against this branch's lowering. (An intermediate gate run showed 4 failures that traced to a half-built environment from a timeout-killed build — old compiled extension without the #412 insert paired with post-rebase sources; Underworld development team with AI support from Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 27, 2026
lmoresi added a commit
that referenced
this pull request
Jul 28, 2026
… tangent, not the JIT From the adversarial review on the PR thread: - The setter docstring and __init__ comment claimed the JIT treated the wrapped flux components as independent constants. Probed false: the constants manifest was empty, the residual was never corrupted, and the identical pre-fix code solves under consistent_jacobian=True. The true mechanism is that the DEFAULT (Picard) tangent differentiates the flux without unwrapping (frozen-coefficient semantics) — an opaque wrapper holding the ENTIRE flux has zero derivative w.r.t. the unknown and its gradient, so G0-G3 vanish and the operator is structurally singular. Wording corrected so the record does not indict the JIT constants machinery (#416, exonerated). - Documented the side effect: with nothing left to freeze, GenericFluxModel's default tangent is effectively full Newton. - Test hygiene: removed the hand-wiring of Parameters._solver (the constitutive_model setter does it — hand-wiring masks a regression in that path) and the MUMPS solver pin (house policy avoids MUMPS; plain LU is used by the review probes and passes identically). Fix itself unchanged; all review probes pass (see thread). Underworld development team with AI support from Claude Code
lmoresi added a commit
that referenced
this pull request
Jul 28, 2026
…ies (#455) * Fix GenericFluxModel flux storage — preserve symbolic field dependencies GenericFluxModel.Parameters.flux setter (and __init__) wrapped each flux component in a UWexpression via validate_parameters(), creating intermediate symbols (q_0, q_1) that the JIT treated as independent constants, losing the dependency on the unknown field variable and producing a singular matrix. Fix: store raw sympy expressions directly, bypassing the UWexpression wrapper. Includes test test_1060_generic_flux_model_update.py verifying both the _force_setup and _solver-link propagation paths. Underworld development team with AI support from Claude Code * review response (wording + test hygiene): the mechanism is the Picard tangent, not the JIT From the adversarial review on the PR thread: - The setter docstring and __init__ comment claimed the JIT treated the wrapped flux components as independent constants. Probed false: the constants manifest was empty, the residual was never corrupted, and the identical pre-fix code solves under consistent_jacobian=True. The true mechanism is that the DEFAULT (Picard) tangent differentiates the flux without unwrapping (frozen-coefficient semantics) — an opaque wrapper holding the ENTIRE flux has zero derivative w.r.t. the unknown and its gradient, so G0-G3 vanish and the operator is structurally singular. Wording corrected so the record does not indict the JIT constants machinery (#416, exonerated). - Documented the side effect: with nothing left to freeze, GenericFluxModel's default tangent is effectively full Newton. - Test hygiene: removed the hand-wiring of Parameters._solver (the constitutive_model setter does it — hand-wiring masks a regression in that path) and the MUMPS solver pin (house policy avoids MUMPS; plain LU is used by the review probes and passes identically). Fix itself unchanged; all review probes pass (see thread). Underworld development team with AI support from Claude Code * review response (test hygiene): drop hand-wired Parameters._solver and the MUMPS pin The constitutive_model setter wires Parameters._solver — hand-wiring masked a regression in that path and weakened test_flux_update_via_solver_link. Plain LU replaces the MUMPS pin (house policy; review probes used LU throughout). Underworld development team with AI support from Claude Code --------- Co-authored-by: lmoresi <louis.moresi@anu.edu.au>
lmoresi added a commit
that referenced
this pull request
Aug 13, 2026
… with a constants-aware key (#417, supersedes #418) (#543) * Reuse the rotated free-slip solver workspace across solves (#417) Repeated transient rotated free-slip Stokes solves rebuilt the rotation Q, the PtAP'd operator, the fieldsplit Schur KSP/PC and the GAMG hierarchy on every timestep — an allocator high-water problem (RSS growth to OOM, issue #417) and roughly half the per-step cost of the Zhong A1 production runs. Cache the workspace on the solver between solves, re-derived from the original PR #418 (bec76bb) at the seam of the rewritten unified Newton loop: - geometry tier (Q/Qt, constrained rows, fault pair blocks, custom-FMG prolongation) reused while the boundary/fault registration and DM are unchanged; - structure tier (Ahat, Schur pmat, nullspaces, KSP/PC) reused as objects with values refreshed in place — the loop's own between-iteration pattern; - an iteration-0 fast path skips Jacobian assembly / ptap / PCSetUp entirely when the operator coefficient state counters match (RHS-only timesteps); - the cache is forfeited for direct-LU, prescribed-datum and fault interface-law solves, and torn down by _reset() and the _build() full rebuild before the SNES/DM are destroyed; - expose the existing time= argument through the Stokes.solve wrapper; an explicit time vetoes the fast path (petsc_t bypasses every counter). Unlike the original one-shot linear path, a wrong fast-path verdict here cannot return a stale solution: the loop measures the true residual at every iterate and reassembles from iteration 1 on. Regression: RHS-only reuse (same Q/Ahat/KSP handles), viscosity-field invalidation with in-place refresh, and the time= veto, in test_1018_rotated_freeslip. Production evidence on the original mechanism: 310 guarded Zhong A1 steps, flat RSS (PR #418 thread). Underworld development team with AI support from Claude Code * Close the rotated-workspace blind spot: rampable constants join the key, the verdict goes collective The PR #418 review's unresolved finding: the reuse key was built from MeshVariable._state counters, which are BLIND to rampable UWexpression constants — the #416 contract lets a constant change value with no state bump, so a 2x viscosity ramp between solves reported "unchanged" and (on the original one-shot path) returned a bit-identical stale solution while the matrix-probe safety net was disabled by the very verdict it was meant to check. The reworked loop already made a stale verdict non-fatal (the true residual is measured every iterate and iteration 1 onward always reassembles), but the verdict itself must still be honest: - the operator key now includes the packed constants[] values the kernels will actually assemble with, plus the JIT bundle key (covers an in-place kernel rewire). Measured on the ramp probe: the naive key reported workspace_reused=True for a 2x constant ramp; this key reports False and reassembles. Over-invalidation on RHS-only constant changes is accepted — reassembly is the safe default; - if the constants manifest or the coefficient enumeration cannot be read, the fast path is forfeited outright — correctness first; - the fast path additionally requires a self-measured linear hint (last solve converged in <= 1 increment): for a nonlinear model the cached operator is the previous solve's tangent, and the skip would only trade an assembly for a wasted increment; - the match verdict is allgathered and must be unanimous before it gates any collective PETSc call — state counters follow rank-local writes, and a rank-divergent verdict is a deadlock; - on a detected change the stored signature is poisoned before the in-place refresh, so an exception mid-refresh cannot leave a stale key that later matches half-updated values. Regression: test_rotated_workspace_constant_ramp_invalidates (fail-before validated on the naive key: the reused flag lies there) with its own armed- fast-path negative control and a fresh-solver control at the ramped viscosity; test_rotated_workspace_deform_invalidates re-proves the mesh.deform teardown on the reworked cache. Underworld development team with AI support from Claude Code * Fault contact composes with the rotated workspace cache: pair blocks cache, interface laws opt out solve_with_fault drives the same rotated Newton loop, so the cross-solve workspace decision had to be made explicitly for the fault machinery: - FRICTIONLESS pair blocks are geometry (coincident-node pairing + fault normals live in Q, keyed by the fault registration in the geometry signature) — they cache. A warm repeat reuses the rotation; a cold re-solve rides the iteration-0 fast path; both match a fresh-solver control on the same mesh. - INTERFACE-LAW solvers (viscous / Coulomb / rate-state) opt out entirely: the interface tangent is reassembled per iterate at the current slip rates and the reaction-fed normal stress is Picard-lagged solver state — neither is keyable registration state, so cache_allowed excludes them at the top of solve_rotated_freeslip. Regression: test_fault_repeat_solve_composes_with_workspace_reuse covers both arms, with a fresh-solver control for the cached arm and the absent- cache assertion for the opt-out. Underworld development team with AI support from Claude Code * Review response: persist the operator key only for values Ahat holds; null before destroy Two minors from the #543 review. (m2) The persisted operator_sig now requires that this solve either reassembled Ahat or rode a key-matched fast path - a poisoned solve exiting at iteration 0 without assembling can no longer store a fresh key against unrefreshed values (the window was bounded by the always-reassemble net; now it is closed). (m4) Both teardown sites null the cache attribute before destroying, so an exception mid-destroy leaves objects unreachable rather than arming a double-destroy. Underworld development team with AI support from Claude Code --------- Co-authored-by: Tyagi <gthyagi@gmail.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 freeto 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.
Closes#302. The prerequisite for the model-owned-time design recorded on #410.
The bug — broader than reported
A constant
UWexpressionlisted inconstants_manifestcould still be baked into the generated C as a literal. The lowering substitutedconstants[]placeholders with a TOP-LEVELxreplaceand only then unwrapped — so a constant hidden inside a nestedUWexpressionnever met the substitution, and the unwrap folded it to a number. Since everyParameters.*value is template-wrapped in aUWexpression, this froze every constant inside constitutive parameter expressions, not just the issue's ξ case: the reproducer shows even the 'δ pattern' freezes when it arrives through a parameter (ramping changed nothing; a rebuild changed the answer x10). It also explains why rebuilds 'worked': the cache key baked the same values, so a ramp changed the hash and forced the recompile a live constant exists to avoid.The fix
Three-phase lowering, identical in codegen and cache hashing (shared
_reveal_constants): (1) expand non-constantUWexpressions to fixpoint while KEEPING truly-constant atoms symbolic — the predicate is the same one the manifest scan uses, so the revealed set equals the manifested set by construction; (2)xreplacemanifested constants →constants[]placeholders; (3) resolve remaining atoms. Two RuntimeError guards make silent baking structurally impossible: a truly-constant atom with no manifest slot, or a manifested constant surviving to the final expression, refuses loudly. Nested-constant ramps no longer invalidate the JIT cache.Adversarial review (public, posted below) — all six attacks HELD
Eight nested topologies probed with real solves against independent literal-rebuild ground truths — constant exponents (the known sibling gotcha),
sqrt(f² + δ²),Piecewise,Maxincluding a ramp that switches the runtime branch, double nesting: every ramp matched truth to 1e-9, zero silent bakes. Guards: zero false positives across rotated free-slip consistent-Newton, ND poisson/stokes, units-active constants, and a VEP subset (31 tests green). Cache: ramp → 0 new .so, structural change → recompiles. Consistent-Jacobian: ramped nested amp AND exponent give bit-identical solution and iteration count vs a rebuild. Performance: 20-deep nesting, no measurable cost vs development. The review's one in-diff finding (a silent fallback in the cache-key path that would have re-created recompile-per-ramp) is fixed in the response commit; its pre-existing discovery is filed as #415 (Max/Min(UWexpression, number)construction crash).Verification
tests/test_0103_jit_rampable_constants.py): top-level and nested constants ramp via real solves with no rebuild; ramped == rebuilt to 1e-10; ramp-back restores base exactly.Manual close on merge: #302.
Underworld development team with AI support from Claude Code