feat(explorer): accept the modelChemistry LevelOfTheory call form in the source guard - #198
Conversation
723c7ae to
92f10be
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the Arkane explorer/network source guard to accept the real-world modelChemistry = LevelOfTheory(...) / CompositeLevelOfTheory(...) call form (while keeping the existing “literal-only top-level assignment” policy for everything else), aligning the explorer reader with T3/ARC’s writer behavior.
Changes:
- Allow
modelChemistryassignments whose value is a validatedLevelOfTheory(...)/CompositeLevelOfTheory(...)AST call (via the existing_validate_model_chemistry_expressionstructural checker). - Add targeted tests covering accepted modelChemistry call forms and ensuring computed/non-call values remain refused.
- Ignore
docs/contracts/via.gitignore.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
t3/pdep/explorer/input_file.py |
Adds a narrow exception for modelChemistry assignments to permit validated call expressions while preserving the literal-only rule elsewhere. |
tests/test_pdep/test_explorer_input_file.py |
Adds regression tests for accepting valid modelChemistry call forms and refusing malformed/computed variants. |
.gitignore |
Adds docs/contracts/ to ignored paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| raise ValueError( | ||
| f"Refusing to use '{source_path}' as an Arkane explorer/network source: line " | ||
| f"{node.lineno} assigns a 'modelChemistry' value that fails structural validation " | ||
| f"({_source_snippet(node, text)!r}): {e}. This source's text is spliced verbatim into " |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #198 +/- ##
==========================================
- Coverage 82.73% 82.72% -0.01%
==========================================
Files 72 72
Lines 11722 11729 +7
Branches 2527 2528 +1
==========================================
+ Hits 9698 9703 +5
- Misses 1433 1434 +1
- Partials 591 592 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ce guard `_validate_source_statements` refuses any top-level assignment that is not `ast.literal_eval`-able, so a legitimate `modelChemistry = LevelOfTheory(...)` (or `CompositeLevelOfTheory(...)`) -- the bare-call form ARC and T3's own hybrid writer emit and Arkane exec's at load time -- was refused as non-literal. Teach that one branch a narrow exception: when the target is `modelChemistry` and the value is a genuine `LevelOfTheory`/`CompositeLevelOfTheory` call node, validate it with the same structural checker T3 uses when it emits this directive (`t3.pdep.hybrid._validate_model_chemistry_expression`), bridging the AST/string gap via `ast.unparse`. The gate is on a real call node because that checker accepts any non-call string as a plain label, so a computed `modelChemistry` value must keep falling through to refusal. Every other target and every other non-literal assignment is refused exactly as before.
Stacked on #197 — based on
i016-ts-positional, so this PR's diff shows only its own change. GitHub retargets it tomainautomatically when #197 merges. Review #197 first.What this fixes
T3's PES loop writes a hybrid network in round 0 and reads it back in round 1. #197 fixed the first defect on that handoff. With #197 in place round 1 gets further and dies again, in a different reader, on line 1 of every hybrid T3 writes:
_validate_source_statementscallsast.literal_evalon every top-level assignment's value. The line is T3's own and deliberate: the source network has nomodelChemistryat all,t3/pdep/hybrid.pyintroduces it, and that module namesLevelOfTheory/CompositeLevelOfTheoryas the two call forms an ArkanemodelChemistrydirective may legitimately take. ARC's independently generated Arkane inputs carry the identical spelling. Arkaneexecs these files, so the call resolves there.The change
The literal-only rule stays, and so do its siblings (refusing
**unpacking, refusing to rebind names Arkane defines). One exception: when the target ismodelChemistryand the value is a genuineast.Callto one of the two known names, validate it witht3.pdep.hybrid._validate_model_chemistry_expression— the same structural checker T3 uses when it emits this directive — instead of refusing it as non-literal.Reusing that checker rather than growing a second allowlist is deliberate: it already type-checks each keyword against Arkane's real
LevelOfTheoryschema, narrowed below it on purpose, and two copies of that rule would drift silently.Gating on a real call node matters and is not incidental: the checker treats any non-call string as a plain label and only injection-checks it, so routing an arbitrary value there would accept
modelChemistry = 1 + 2. Computed values keep falling through to the refusal. Pinned bytest_a_computed_non_call_model_chemistry_value_is_still_refused.Testing
tests/test_pdep/test_explorer_input_file.py: 266 passed. The three accept-cases were red before the fix ("assigns something that is not a literal") and green after; the refusal cases were green throughout.tests/test_pdep: 1915 passed, 134 s.test_hybrid+test_energy_settings137 passed;test_main_wiring+test_capture+test_pes_qm215 passed. No circular import (hybridimportsparserandwriter, neverinput_file).write_arkane_explorer_input_file— verified end to end, no cluster, no quantum chemistry. It then stops further along at an unrelated seed-label mismatch, tracked separately.