fix(pdep): let the pdep parser read T3's own transitionState('TS2', 'qm/TS2.py') - #197
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #197 +/- ##
==========================================
- Coverage 82.73% 82.71% -0.02%
==========================================
Files 72 72
Lines 11720 11722 +2
Branches 2526 2527 +1
==========================================
Hits 9696 9696
- 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:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a producer/consumer mismatch within T3’s pdep hybrid workflow by teaching the pdep parser to accept Arkane’s required two-positional transitionState(label, path) form that T3 itself writes into hybrid network files.
Changes:
- Allow
_call_keywordsto maptransitionState('TS2', 'qm/TS2.py')into{'label': ..., 'path': ...}while continuing to refuse other positional-argument shapes. - Add regression tests asserting the real hybrid artifact parses and that both label and path are recovered.
- Vendor the real crashed hybrid network file plus provenance documentation under
tests/data/.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
t3/pdep/parser.py |
Adds a narrowly-scoped exception so transitionState(label, path) positional form is parsed into keywords. |
tests/test_pdep/test_parser.py |
Adds regression tests covering the real crashed artifact and the narrowness of the exception. |
tests/data/pdep_hybrid/positional_ts/README.md |
Documents provenance and the load-bearing line in the vendored artifact. |
tests/data/pdep_hybrid/positional_ts/network0_reduced.py |
Adds the byte-for-byte hybrid network fixture that previously triggered the crash. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if call_name == 'transitionState' and len(call.args) == 2 and not call.keywords: | ||
| return {'label': call.args[0], 'path': call.args[1]} |
|
|
||
| def test_real_hybrid_file_parses(self): | ||
| """Verifier 1: the real crashed artifact drives through the pdep parser and parses.""" | ||
| network = parse_pdep_network_file(HYBRID_POSITIONAL_TS_FILE) |
…qm/TS2.py')
The PES loop writes a hybrid network in round 0 and reads it back in round 1.
When a transition state is adopted from quantum chemistry, the writer
(t3/pdep/hybrid.py) rewrites it to transitionState('<label>', 'qm/<label>.py')
-- two positional arguments -- because that is the ONLY spelling Arkane accepts
to load a stat-mech file from a path (arkane/input.py:241, path-loading guarded
by `len(args) == 1 and len(kwargs) == 0`; the keyword spelling silently disables
it). The reader's _call_keywords refused every positional argument, so T3 crashed
on the very file it had written minutes earlier.
Widen the refusal by exactly one form: transitionState(label, path) with two
positionals and no keywords maps to {'label', 'path'}. Every other positional
shape, and this same shape for any other call name, still falls through to the
refusal -- the guard caught a real producer/consumer inconsistency and is not
relaxed generally. The writer is left untouched; it is correct for Arkane.
Vendored fixture is the byte-for-byte hybrid file from the r001_m1-cho2-pilot run
that actually crashed. A test drives it through parse_pdep_network_file and
asserts both TS2's label and its path are recovered -- not merely that no
exception is raised.
723c7ae to
92f10be
Compare
What this fixes
T3's PES exploration loop crashed in round 1 on a hybrid network file T3 itself wrote in round 0, during the first run of the loop against real quantum chemistry:
t3/pdep/hybrid.pyemitstransitionState('TS2', 'qm/TS2.py')when it vendors a QM-adopted transition state;t3/pdep/parser.py::_call_keywordsrefuses every positional argument. Producer and consumer are both inside T3 and they disagreed.Why the fix is on the reader's side
The obvious fix — make the writer use keywords — does not work.
arkane/input.py:241isdef transitionState(label, *args, **kwargs), and its load-a-statmech-file-from-a-path branch is guarded byif len(args) == 1 and len(kwargs) == 0. Arkane offers no keyword spelling for path-loading, and Arkane reads this same file, sotransitionState(label=..., path=...)would silently stop loading the QM statmech file.The guard's premise is what was wrong. Its comment justifies refusing positionals by surveying "zero of the 156 parseable files under RMG-Py's
examples/andarkane/data/". That survey is real and its conclusion does not hold here, because the corpus never contained T3's own writer's output.The change
One narrow exception in
_call_keywords, ahead of the unchanged refusal: when the call istransitionStatewith exactly two positional arguments and no keywords, mapargs[0] -> label,args[1] -> path. Every other positional shape — one or three-plus positionals, positionals mixed with keywords, and this same two-positional shape forspecies/reaction— still falls through to the refusal, which is deliberate: mapping positions to names generally would mean duplicating Arkane's signatures and keeping them in step forever.Both
_call_keywordscall sites that can see atransitionStatenode (parse_pdep_network_textandparse_pdep_network_e0_text) passcall_nameand are therefore covered.Testing
The real crashed artifact is vendored at
tests/data/pdep_hybrid/positional_ts/network0_reduced.pywith a provenance README, since the run directory it came from is scratch.{'label': 'TS2', 'path': 'qm/TS2.py'}) — not merely "no exception raised".tests/test_pdep: 1902 passed.Not covered, deliberately
arkane/input.pyline ~130 givesspecies(label, *args, **kwargs)the identical path-loading shape, so the same trap is latent there. T3's writer never emits a positionalspecies(...)— the hybrid writer's species channel is keyword-form — so covering it now would widen the guard to a spelling nothing writes, and would need a fabricated fixture. Until a writer path does emit it, the parser refuses loudly rather than mis-parsing silently, which is the safe failure direction.