Move input-only checks to case_validator, delete dead params_tests, enforce the split - #1717
Open
sbryngelson wants to merge 10 commits into
Open
Move input-only checks to case_validator, delete dead params_tests, enforce the split#1717sbryngelson wants to merge 10 commits into
sbryngelson wants to merge 10 commits into
Conversation
The Python validator and src/simulation/m_checker.fpp had drifted into double entry: reactive_burn (#1670) added nine constraints to both in the same PR, and ib_state_wrt, chemistry+bubbles_euler, and dt <= 0 were each enforced twice. Delete the Fortran copies of the checks that depend only on input parameters, and complete their Python counterparts: - reactive_burn num_fluids / gamma / pi_inf / qv pairing (new in Python) - chemistry operator-split sub-stepping, 4 checks (new in Python) - many_ib_patch_parallelism requires ib (new in Python) - bf_spatial_support 2D-only (new in Python) - dt <= 0, ib_state_wrt, chemistry + bubbles_euler/qbmm (already in Python; drop the Fortran duplicates) s_check_inputs_time_stepping had no remaining body and is removed. Fortran defaults are materialized where an unset value is meaningful: chem_params%reaction_substeps{,_max} and fluid_pp(i)%qv all default to 0, so Python treats unset as 0 to match. gamma and pi_inf default to the dflt_real sentinel, which f_approx_equal reports as equal, so Python skips the comparison when either is unset. nv_uvm_igr_temps_on_gpu stays in Fortran. Its default is 3 and the check is "== 3 .and. igr_iter_solver == 2", so it fires precisely when the user has not set the parameter; in Python an unset value reads as None and the check would never fire. It is also inside #ifdef __NVCOMPILER_GPU_UNIFIED_MEM, which the validator cannot see. The remaining Fortran checks depend on runtime or compiler state that is unavailable at validation time: MPI decomposition (s_check_total_cells, s_check_inputs_fft), per-rank m/n/p (WENO/MUSCL stencil widths), and compiler conditionals (s_check_amd, rdma_mpi). Add toolchain/mfc/test_case_validator.py. case_validator.py had no pytest coverage of its constraint checks, and validating the example cases only exercises configurations meant to pass, so a check that stopped firing would go unnoticed -- which now matters more, since these no longer have a Fortran backstop. Reverting case_validator.py fails 15 of the 24 tests. src/simulation/m_checker.fpp: 225 -> 162 lines.
…plit
Three follow-ons to the previous commit, which moved the input-only
simulation checks into case_validator.
Migrate the last input-only Fortran checks
------------------------------------------
- particle_cloud packing_method (2 checks) and the muscl_order/int_comp
check were already duplicated in case_validator; drop the Fortran.
- synthetic-turbulence forcing zones (3 checks) had no Python
counterpart; add check_synthetic_turbulence, covering
num_turbulent_sources bounds and per-zone turb_pos / synth_L over
d = 1..num_dims, matching the Fortran loop so trailing components of a
lower-dimensional case stay optional.
s_check_inputs_particle_clouds and s_check_inputs_synthetic_turbulence
are removed along with their call sites, and m_helper_basic and
muscl_order_first_order are no longer used by m_checker.
src/simulation/m_checker.fpp: 162 -> 113 lines (225 before this branch).
Delete the params_tests CLI island
----------------------------------
coverage.py, inventory.py, mutation_tests.py, negative_tests.py,
runner.py, and snapshot.py: 1651 lines that collect zero pytest tests.
Each is imported only by runner.py, which nothing imports and which
appears in no workflow, script, doc, or CMake file. Its entire history is
incidental edits from unrelated feature PRs -- #1713 had to update
mutation_tests.py to drop model_eqns = 4 earlier today.
It could not be promoted as-is: negative_tests decides whether the right
error fired with a "half the key terms appear" substring heuristic, and
runner verify needs a data/ baseline that is gitignored. The sibling
test_*.py files, which supply all 171 real tests in the package, stay.
Enforce the split going forward
-------------------------------
Add check_checker_input_constraints to lint_source. A @:PROHIBIT in
src/**/m_checker*.fpp is allowed only inside a subroutine listed in
RUNTIME_CHECKER_SUBROUTINES -- the ones that depend on the MPI
decomposition, per-rank grid extents, the active compiler, or a
Cantera-populated value -- or on a line marked
"! lint: runtime-check <reason>".
This is what stops the next feature PR from re-creating the double entry
this branch removed: reactive_burn added nine constraints to both
languages in a single PR.
sbryngelson
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Moves input-parameter-only simulation constraints out of Fortran m_checker*.fpp and into toolchain/mfc/case_validator.py, reducing duplicate logic and enforcing rules earlier (before binaries run).
Changes:
- Added Python-side constraint checks for synthetic turbulence, chemistry substepping, reactive-burn fluid pairing, IB/body-force flags, and removed matching Fortran duplicates.
- Added a dedicated unit test module covering the migrated Python-only constraints.
- Added a source-lint rule to prevent reintroducing input-only
@:PROHIBITconstraints into Fortran checker files; removed the old params test “safety net” tooling.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| toolchain/mfc/test_case_validator.py | New unit tests for migrated Python-only constraint checks. |
| toolchain/mfc/params_tests/snapshot.py | Removed legacy snapshot-based validation regression tooling. |
| toolchain/mfc/params_tests/runner.py | Removed CLI runner for legacy params test tooling. |
| toolchain/mfc/params_tests/negative_tests.py | Removed legacy negative-test generator (superseded by new unit tests). |
| toolchain/mfc/params_tests/mutation_tests.py | Removed legacy mutation test tool. |
| toolchain/mfc/params_tests/inventory.py | Removed parameter inventory export tool. |
| toolchain/mfc/params_tests/coverage.py | Removed constraint coverage analysis tool. |
| toolchain/mfc/params_tests/init.py | Updated package docstring to reflect remaining test purpose. |
| toolchain/mfc/params_tests/.gitignore | Removed ignores for deleted generated “data/” outputs. |
| toolchain/mfc/lint_source.py | Added lint guard to keep input-only constraints out of Fortran m_checker*.fpp. |
| toolchain/mfc/case_validator.py | Added/expanded Python checks replacing the removed Fortran counterparts. |
| src/simulation/m_checker.fpp | Removed input-only @:PROHIBIT checks and dead subroutines now enforced in Python. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+82
to
+89
| def errors_for(self, params) -> str: | ||
| """Return all simulation-stage validation errors for params, joined.""" | ||
| validator = CaseValidator(dict(params)) | ||
| try: | ||
| validator.validate("simulation") | ||
| except Exception as exc: # CaseConstraintError | ||
| return str(exc) | ||
| return "" |
Comment on lines
+91
to
+99
| def assertRejects(self, params, expected: str): | ||
| """params must fail validation with expected in the message.""" | ||
| errors = self.errors_for(params) | ||
| self.assertIn(expected, errors) | ||
|
|
||
| def assertAccepts(self, params, unexpected: str): | ||
| """params must not trip the check identified by unexpected.""" | ||
| errors = self.errors_for(params) | ||
| self.assertNotIn(unexpected, errors) |
Every ./mfc.sh run called ensure_vscode_settings(), which edited
.vscode/settings.json in the user's checkout whether or not they use VS
Code. It did so by string surgery -- rfind("}"), then splice in a block
and guess whether a comma is needed -- on a file that is hand-maintained,
committed, and legally contains comments, so a malformed result was a
plausible outcome.
Remove toolchain/mfc/ide.py entirely. Its two functions,
ensure_vscode_settings and update_vscode_settings, were near-duplicates
of each other, called from main.py (every invocation) and from
generate --json-schema respectively.
Also drop generate_vscode_settings() from json_schema_gen.py: a third
copy of the same settings blob, called by nothing, and disagreeing with
ide.py on both the fileMatch patterns and the schema URL.
.vscode/settings.json keeps all 32 of its settings, including the
json.schemas/yaml.schemas association -- it is a useful set of defaults
for anyone working on MFC. The block is now plain committed config rather
than a region the toolchain rewrites, so the "auto-generated, do not
edit" markers are replaced with a comment pointing at ./mfc.sh generate,
which still writes toolchain/mfc-case-schema.json.
user_guide.py: 710 -> 315 lines. interactive_mode (~145 lines) ./mfc.sh interactive presented a numbered menu whose seven handlers each shelled back out to ./mfc.sh new/validate/build/run/test/clean. The command is removed from the CLI schema, main.py, and the README. Tips (~78 lines) A class of five contextual hint methods. Only after_build_failure was ever called (twice, from build.py); after_case_error, after_test_failure, after_run_failure, and suggest_validate had no callers at all. Markdown help scraper (~173 lines) MARKDOWN_HELP_FILES, _extract_markdown_section, _load_markdown_help, and _generate_markdown_help re-read docs/documentation/*.md at runtime, stripped Doxygen syntax with regexes, and re-rendered four topics (gpu, batch, debugging, performance) in the terminal -- a second, lossy presentation of pages that already exist on the docs site. With those four topics gone, HELP_TOPICS held a single dynamic entry, so the topic-dispatch layer (print_topic_help, print_help_topics, and the `topic` positional on the help command) collapses into print_clusters_help. ./mfc.sh help now prints the cluster table directly, through the same Panel it always rendered in. Kept: the cluster table itself, which derives from toolchain/modules and so cannot go stale, and print_help / print_command_help, which back ./mfc.sh --help and ./mfc.sh <command> --help.
The cluster listing existed twice: a hand-written coloured menu in
toolchain/bootstrap/modules.sh, shown by ./mfc.sh load, and 163 lines of
Python in user_guide.py that re-derived the same list from
toolchain/modules for ./mfc.sh help.
Keep the shell menu and delete the Python. That removes CLUSTER_ORGS,
SLUG_ORG_OVERRIDE, SLUG_NAME_OVERRIDE, ORG_ORDER, ORG_COLORS (which mapped
all eight organisations to "yellow"), _parse_modules_file,
_get_cluster_short_name, _generate_clusters_content, and
print_clusters_help. The cluster table was the only content ./mfc.sh help
had left, so the help command goes with it.
user_guide.py: 315 -> 144 lines (710 at the start of this branch).
Fix the drift the second copy had been masking. The menu offered Summit,
which was decommissioned and has no entry in toolchain/modules, so
selecting it loaded nothing; and it omitted Phoenix IFX (pifx) and Santis
(san), both of which have module sets that users could not discover.
Add check_cluster_menu_slugs to lint_source so the hand-written menu
cannot drift again: the slugs it advertises must match the cluster
definitions in toolchain/modules, in both directions. Module-list lines
(<slug>-{all,cpu,gpu}[-unload]) are excluded -- p-gpu-unload is an unload
list, not a cluster.
Three places still offered Summit as a machine you could target:
- modules.sh show_help listed "Summit (s)". This was a second, staler
copy of the cluster list inside the same file -- it also omitted
Tuolumne, Santis, Phoenix IFX, Anvil, HiPerGator, and Turing. Replace
the list with a pointer to the interactive menu so the file holds one
list, the one check_cluster_menu_slugs already validates.
- running.md's "Example Runs" passed -c summit. For ./mfc.sh run, -c
names a batch template in toolchain/templates, and summit.mako does
not exist, so the documented command could not have worked. Point it
at Frontier.
- running.md described LSF as "e.g., Summit". LSF support stands; drop
the example.
Historical references are deliberately kept: the V100 weak-scaling
results and figure in expectedPerformance.md, the gallery entries in
docs/index.html recording where each simulation ran, the allocation
acknowledgement and scaling record in the README, and the search
keywords. Those describe what was done, not what users can target.
|
Claude Code Review Head SHA: 21ca34b Files changed:
Findings:
|
errors_for caught bare Exception and returned str(exc), so a regression in the validator that raised KeyError or TypeError would be stringified and could satisfy an assertion instead of failing the test. Catch only CaseConstraintError; anything else propagates. assertAccepts checked that one message was absent, which a case that broke for an unrelated reason would still satisfy. It now asserts full validity (no violations at all) and takes only the params. Every call site already uses a fully valid configuration, verified individually, so no test needed relaxing to accommodate the stronger form. assertRejects gained a check that validation failed at all, so a case that is wrongly accepted reports "expected validation to fail with X" rather than the less obvious "X not found in ''". Verified the new assertions catch what the old ones missed: injecting a KeyError into CaseValidator.validate now errors the test instead of passing; assertAccepts on a fixture with an unrelated violation (dt <= 0) now fails; assertRejects on a case that validates cleanly now fails. The 2D synthetic-turbulence z-component test folded into test_accepts_fully_specified_zone -- with assertAccepts asserting full validity, accepting a fixture that sets only d = 1, 2 is itself the proof that turb_pos(1,3) and synth_L(1,3) are not required. 32 -> 31 tests.
Member
Author
|
Thanks — went through all four findings. Two were valid and are fixed in 94b8565; two are false positives, with evidence below. Fixed — Copilot,
|
| Claim | Where it lives on master |
|---|---|
rburn%ta / fluid_pp(1)%cv |
case_validator.py:1571 onward, in check_reactive_burn |
muscl_order / int_comp |
case_validator.py:431 |
git diff master...HEAD -- toolchain/mfc/case_validator.py contains no +/- line touching rburn%ta, %cv, or int_comp.
Confirmed by running the validator on this branch:
rburn%ta < 0 -> CAUGHT
rburn%ta > 0 with fluid_pp(1)%cv = 0 -> CAUGHT
rburn%ta > 0 with cv unset -> CAUGHT
muscl_order = 1 with int_comp = 1 -> CAUGHT
Message for the second: reactive_burn with rburn%ta > 0 requires fluid_pp(1)%cv > 0 (the reactant temperature needs a physical heat capacity; cv = 0 silently disables the Arrhenius factor) — the exact constraint the review reported as dropped.
Worth noting these two are only covered by check_reactive_burn / check_interface_compression, not by the new tests, since they predate this PR. Happy to add cases for them if that's wanted, though it widens the scope past the migration.
Self-review of this branch found that three checks I moved to Python are
weaker than the Fortran they replaced, all from the same cause: Python's
`is not None` / `_is_numeric` guards no-op on an absent key, while the
deleted Fortran compared against the dflt_real / dflt_int sentinels, so an
unset parameter was itself a violation. Each turned a startup abort into a
silently wrong run.
- reactive_burn now requires both fluid_pp(1)%{gamma,pi_inf} and
fluid_pp(2)%{gamma,pi_inf} to be set. Previously, omitting fluid 2's
EOS was accepted and the product phase ran with gamma = pi_inf =
dflt_real = -1e6, a negative stiffened-gas EOS. The earlier reasoning
only covered both-unset, where f_approx_equal(dflt, dflt) is true.
- reactive_burn now requires num_fluids and model_eqns to be set, not
merely correct-if-present. Omitting both was accepted outright.
- dt is required whenever stepping is not CFL-driven. The adap_dt
exemption had no counterpart in the Fortran, which checked
`if (.not. cfl_dt) dt <= 0` regardless. Cases that genuinely need no
dt use cfl_adap_dt (2D_lagrange_rising_bubble), which is unaffected.
Four tests cover the gaps; all four fail against the previous validator.
TestTimeStepPositivity became TestTimeStep, since it previously only
exercised a rule that predates this branch.
lint_source.py fixes, all in code this branch added:
- The runtime-check marker was consumed by the next physical line, so a
blank line or Fypp directive between it and the @:PROHIBIT silently
dropped the exemption. It now persists until the next @:PROHIBIT.
- The subroutine regex accepted only impure/pure, so `recursive
subroutine` reported "at module scope" and told the developer to add
`None` to the allowlist. It now accepts the full prefix set and gives
scope-appropriate advice.
- s_check_inputs_weno and s_check_inputs_muscl left the allowlist. They
mix runtime and input-only checks -- the muscl_order/int_comp check
this branch deletes lived in one of them -- so allowlisting the
subroutines would have let a replacement back in unnoticed. Their six
grid-extent checks now carry explicit markers instead, and an
int_comp-style addition to either is caught.
- check_cluster_menu_slugs keyed on the literal strings "Select a
system:" and "read u_c", so rewording the prompt disabled it and
editing the read broke precheck repo-wide. It now keys on explicit
cluster-menu-begin/end markers, tolerates rewording, still catches
slug drift, and fails loudly if the markers go missing.
Also drops HELP_TOPICS from commands.py and the help_topics field from
CLISchema: 25 lines describing five help topics that no longer exist and
that nothing reads, left behind when the help system was removed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1717 +/- ##
==========================================
- Coverage 61.21% 61.20% -0.01%
==========================================
Files 84 84
Lines 21601 21572 -29
Branches 3195 3192 -3
==========================================
- Hits 13223 13204 -19
+ Misses 6209 6202 -7
+ Partials 2169 2166 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…alidator Master's s_check_inputs_hll_non_conservative and s_check_inputs_hypo_branch are the double entry this branch removes: nearly all of their PROHIBITs already exist verbatim in case_validator.py. Dropped both, and migrated the four that had no Python counterpart (HLL Method 2 3D cylindrical, alt_soundspeed with HLL in 2D axisymmetric and 3D cylindrical, alt_soundspeed with HLLD without hypoelasticity), with tests.
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.
Description
Three related changes. Net −1,363 lines (409 added, 1,772 removed).
1. Move input-only checks out of Fortran
case_validator.pyandsrc/simulation/m_checker.fpphad drifted into double entry —reactive_burn(#1670) added nine constraints to both, in the same PR.reactive_burnnum_fluids / gamma / pi_inf / qvcheck_synthetic_turbulence)many_ib_patch_parallelism requires ibbf_spatial_support2D-onlydt <= 0,ib_state_wrt,chemistry + bubbles_euler/qbmmparticle_cloud%packing_method(2),muscl_order/int_compsrc/simulation/m_checker.fpp: 225 → 113 lines.s_check_inputs_time_stepping,s_check_inputs_particle_clouds, ands_check_inputs_synthetic_turbulenceare gone entirely, along with the now-unusedm_helper_basicimport andmuscl_order_first_order.Fortran defaults are materialized where an unset value is meaningful.
chem_params%reaction_substeps{,_max}andfluid_pp(i)%qvdefault to0, so unset is treated as0.gamma/pi_infdefault to thedflt_realsentinel, whichf_approx_equalreports as equal, so the comparison is skipped when either is unset.check_synthetic_turbulenceloopsd = 1..num_dimsto match the Fortran, so trailing components of a lower-dimensional case stay optional.nv_uvm_igr_temps_on_gpudeliberately stays in Fortran. Its default is3and the check is== 3 .and. igr_iter_solver == 2, so it fires precisely when the user has not set the parameter. In Python an unset value reads asNoneand the check would silently never fire — moving it would look like a migration and act like a deletion. It is also inside#ifdef __NVCOMPILER_GPU_UNIFIED_MEM.What remains in Fortran depends on state unavailable at validation time: MPI decomposition (
s_check_total_cells,s_check_inputs_fft), per-rankm/n/p(WENO/MUSCL stencils), compiler conditionals (s_check_amd,rdma_mpi), andnum_species, which Cantera fills in at runtime.2. Delete the
params_testsCLI island — −1,651 linescoverage.py,inventory.py,mutation_tests.py,negative_tests.py,runner.py,snapshot.py. Together they collect zero pytest tests. Every one is imported only byrunner.py, which nothing imports and which appears in no workflow, script, doc, or CMake file. Its whole history is incidental edits from unrelated feature PRs — #1713 had to updatemutation_tests.pyto dropmodel_eqns = 4earlier today.It could not be promoted as-is:
negative_testsdecides whether the right error fired with amatches >= len(key_terms) * 0.5substring heuristic, andrunner verifyneeds adata/baseline that is gitignored. The siblingtest_*.pyfiles, which supply all 171 real tests in the package, are untouched.3. Enforce the split —
check_checker_input_constraintsNew check in
lint_source.py. A@:PROHIBITinsrc/**/m_checker*.fppis allowed only inside a subroutine listed inRUNTIME_CHECKER_SUBROUTINES, or on a line marked! lint: runtime-check <reason>. Otherwise it fails with a pointer tocase_validator.py.This is what stops the next feature PR from re-creating the double entry this one removes.
Type of change
Testing
./mfc.sh precheck— all 7 stages pass (full pytest suite + all 155 example cases), run repeatedly including via the pre-commit hookpre_process/simulation/post_process— clean, no new warnings./mfc.sh test --percent 4— 24 passed, 0 failed, on both commitstoolchain/mfc/test_case_validator.py— 32 tests, every migrated check in both directions@:PROHIBITwith the right message; the! lint: runtime-checkescape hatch suppresses itcheck_synthetic_turbulenceverified against the realexamples/2D_synthetic_turbulencecase — accepts it, and rejects it oncesynth_L(1,2)is removedcase_validator.pyhad no pytest coverage of its constraint checks, and validating the example cases only exercises configurations meant to pass, so a check that stopped firing would go unnoticed. That matters more now that these have no Fortran backstop. Revertingcase_validator.pyfails 15 of the original 24 tests; the 9 that survive are exactly those covering checks that already existed in Python.Checklist
physics_constraints.md/case_constraints.mdare generated fromcase_validator.pyat docs-build time. APHYSICS_DOCSentry was added forcheck_synthetic_turbulence—lint_docs.pycaught its absence, which is the doc-coverage gate working as intended.GPU changes (expand if you modified
src/simulation/)Validation-only; no kernel, data-movement, or numerics changes. The one GPU-conditional check (
nv_uvm_igr_temps_on_gpu, under#ifdef __NVCOMPILER_GPU_UNIFIED_MEM) is deliberately left in place, ands_check_inputs_nvidia_uvmis on the lint rule's runtime allowlist.