Report the two RDKit failures ARC currently swallows - #1013
Open
calvinp0 wants to merge 2 commits into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1013 +/- ##
==========================================
- Coverage 64.60% 64.58% -0.02%
==========================================
Files 119 119
Lines 39785 39786 +1
Branches 10307 10307
==========================================
- Hits 25703 25696 -7
- Misses 11105 11114 +9
+ Partials 2977 2976 -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:
|
calvinp0
force-pushed
the
fix_rdkit_failure_diagnosability
branch
from
August 23, 2026 08:56
5f3fc7c to
87376e4
Compare
``to_rdkit_mol()`` caught ``AtomValenceException`` from ``Chem.SanitizeMol()`` with a bare ``pass``. It then returned an unsanitized RDMol, which travels on to its callers, and the same valence problem re-emerges much later inside ``EmbedMultipleConfs`` where it is attributed to embedding. The evidence for the original failure was destroyed at the point it was cheapest to read. Keep the swallow: all three callers of ``to_rdkit_mol()`` (``rdkit_conf_from_mol``, ``conformers.embed_rdkit`` and the goflow_ts TS adapter) use the returned molecule, and returning ``None`` here would be a behaviour change with a blast radius well beyond this fix. Only make the failure visible. The log level was chosen from a measurement, not from the comment the code carried. Instrumenting the except and running ``arc/species/``, ``arc/molecule/`` and ``arc/reaction/`` (936 tests) produced zero firings. Running ``arc/job/adapters/ts/linear_test.py`` produced two, and neither is the ``[C-]#[O+]`` case the removed comment named: both are singlet biradicals (multiplicity 1, no formal charges, two lone-pair carbons) reached through ``determine_chirality`` -> ``embed_rdkit`` while computing a reaction atom map. Carbon monoxide never reaches this code in a normal species flow, since ARC short-circuits conformer generation for diatomics. The exception is therefore rare, is not dominated by a known-benign case, and every occurrence means an unsanitized molecule is being handed onward - so a plain ``logger.warning`` is both affordable and warranted. The message names the exception class and its text so the next occurrence self-identifies rather than needing this investigation repeated.
``EmbedMultipleConfs`` does not only raise on failure - for some strained species it returns normally having embedded zero conformers. ``embed_rdkit`` only guarded the raising path, so in that case it returned an RDMol with no conformers and logged nothing at all. Every consumer of that object then reads an empty list, and the first caller to index it gets ``IndexError: list index out of range`` with no record of where the molecule came from. Reproduced on ``C1#CC1``, ``C1#CCC1`` and ``C1#CC#CC#C1``. Treat zero conformers the same as an exception: log a warning naming the species and return ``None``, which is what the ``RDMol | None`` return annotation already promised and what the exception path already did. All four callers already handle ``None`` and none of them is made worse by receiving it: ``get_force_field_energies`` guards with ``if rd_mol is not None``, ``get_force_field_energies_of_conformers`` returns early on ``None``, ``determine_chirality`` skips on ``rd_mol is None or not rd_mol.GetNumConformers()``, and ``species.get_cheap_conformer`` passes it to ``rdkit_force_field``, which returns empty lists for ``None``. A conformer-less molecule and ``None`` therefore produce the same downstream result, minus the crash and plus a log line.
calvinp0
force-pushed
the
fix_rdkit_failure_diagnosability
branch
from
August 23, 2026 12:14
87376e4 to
90ef064
Compare
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.
Two RDKit failures that ARC currently absorbs without a trace.
converter.to_rdkit_molswallowed a valence exceptionIt caught
AtomValenceExceptionwith a barepassand returned an unsanitized molecule — no log line, no signal to the caller that sanitization had failed. Everything downstream treated it as a normal RDKit molecule.It now logs a warning naming the formula, the multiplicity and the exception, so the failure appears in
arc.logat the point it happens rather than as a puzzling result later.conformers.embed_rdkitcould return zero conformers without raisingRDKit's embedder can fail without an exception —
C1#CC1is one such case. The function's own signature documentsRDMol | None, but it returned the conformer-less molecule instead ofNone, so callers proceeded to read conformers that were not there.It now checks
rd_mol.GetNumConformers()and returnsNonewith a warning naming the species, matching the contract the docstring already stated.Why this is standalone
The only PR with any file overlap is #921 (
crest_adapter_clean), whoseconverter.pychange adds areorder_xyz_stringhelper — a different function in a different layer fromto_rdkit_mol. Folding these in would have made a 24-file TS-search adapter PR larger and buried an unrelated diagnostics fix.Verification
Rebased from 27 behind onto current
main; the changed-line set is identical in both directions against the old merge-base, so nothing was dropped.140 passedacrossarc/species/conformers_test.pyandarc/species/converter_test.py. Two commits, four files, each file touched by exactly one commit. ~31 added lines.🤖 Generated with Claude Code