Skip to content

fix: make two quiet paths name the molecules they dropped - #132

Merged
isayev merged 1 commit into
mainfrom
phase17/quiet-paths
Aug 3, 2026
Merged

fix: make two quiet paths name the molecules they dropped#132
isayev merged 1 commit into
mainfrom
phase17/quiet-paths

Conversation

@isayev

@isayev isayev commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Cluster A continued (plan: #130). Both findings are one defect in two places: a code path that loses a molecule and says less about it than another path doing the identical thing, so how much the user is told depends on which door they came through.

M1 — the optimizer's SDF reader logged only the all-failed case

mols = [m for m in mols if m is not None]   # a bad record vanishes here
if not mols:
    logger.warning("No valid molecules in input file. ...")   # only this fired

A single bad record among a thousand left the output file shorter than the input with nothing said about which one. Through opt_geometry that is a short SDF, the path returned, and exit 0 — with RDKit's own C++ parse error as the only trace, and that names a file offset, not a molecule.

SPE.calc_spe and ASE/thermo's iter_thermo_records both log per-record for exactly this situation. This was the one reader that did not.

M2 — the parallel embedding path was quieter than the serial one, twice

serial parallel (before)
unparseable SMILES logs the SMILES it could not parse return [], silent
species embeds nothing n_written == 0 warning nothing

A species that embedded nothing was absent from the output, never reached ranking, and did not even produce "No structure converged". use_parallel_embedding is documented as a performance option; it decided whether a lost molecule was reported.

The empty-result warning is raised in the parent, not the worker, and that placement is the point. A message logged inside a ProcessPoolExecutor worker depends on that child's logging configuration; the parent's does not. It also covers both causes at once — unparseable SMILES and every-conformer-clash-rejected both arrive as an empty result — while the child still logs the parse failure, which adds the reason the parent cannot know.

Verification

  • Both mutation-verified: restoring the silent filter fails the optimizer test; removing the parent-side warning fails the embed test.
  • A third test asserts a molecule that embeds normally is not warned about, so neither new branch can fire on success.
  • The optimizer test uses an all-unparseable SDF deliberately, so it returns before any model is needed — the per-record warning under test happens while reading the file.
  • 1284 passed, 9 skipped; ruff clean.

Version decision recorded

The next release is 3.0.0, not 4.0.0. No 3.x ever reached a package channel — PyPI's latest is 2.3.1, conda-forge's is 2.3.0 — so skipping to 4.0.0 would leave users wondering where 3.x went, while 2.3.1 → 3.0.0 is a plain major bump that still signals the breaking changes.

Three collisions follow, written down rather than acted on:

  1. The v3.0.0 and v3.5.0 tags correspond to no published artifact and need deleting, with v3.0.0 re-created at the release commit. Deleting pushed tags is outward-facing, so it waits for an explicit go-ahead.
  2. The CHANGELOG's [3.0.0] and [3.5.0] sections describe milestones that never shipped — recommend relabelling them as never-published rather than deleting, then retitling the unreleased [4.0.0] section.
  3. Found while checking the above: the CHANGELOG and the published record diverge in both directions. It carries a [2.2.10] that was never published, and is missing 2.3.0 and 2.3.1, which were. The last CHANGELOG entry matching a real release is 2.2.9.

Cluster A of the debt plan, continued. Both findings are one defect in two places:
a code path that loses a molecule and says less about it than another path doing
the identical thing, so how much the user is told depends on which door they came
through.

M1 -- batchopt's SDF reader logged only the case where EVERY record failed to
parse. A single bad record among a thousand left the output file shorter than the
input with nothing said about which one; through opt_geometry that is a short SDF,
the path returned, and exit 0, with RDKit's own C++ parse error -- which names a
file offset, not a molecule -- as the only trace. SPE.calc_spe and ASE/thermo's
iter_thermo_records both log per-record for exactly this situation. Now so does
this reader.

M2 -- the parallel embedding path was quieter than the serial one in two ways.
_embed_single returned [] for an unparseable SMILES in silence where the serial
path logs the SMILES it could not parse, and _run_parallel_embedding had no
counterpart to the serial path's `n_written == 0` warning, so a species that
embedded nothing was absent from the output, never reached ranking, and did not
even produce "No structure converged". use_parallel_embedding is documented as a
performance option; it decided whether a lost molecule was reported.

The empty-result warning is raised in the PARENT rather than the worker, and that
placement is the point: a message logged inside a ProcessPoolExecutor worker
depends on that child's logging configuration, while the parent's does not. It
also covers both causes at once -- unparseable SMILES and every conformer
rejected by clash relief both arrive as an empty result. The child still logs the
parse failure, which adds the reason the parent cannot know.

Both mutation-verified: restoring the silent filter fails the optimizer test,
removing the parent-side warning fails the embed test. A third test asserts a
molecule that embeds normally is not warned about, so neither branch can fire on
success.

Also records the version decision. The next release is 3.0.0, not 4.0.0: no 3.x
ever reached a package channel -- PyPI's latest is 2.3.1, conda-forge's is 2.3.0
-- so skipping to 4.0.0 would leave users wondering where 3.x went, while
2.3.1 -> 3.0.0 is a plain major bump that still signals the breaking changes.

Three collisions follow from that and are written down rather than acted on: the
v3.0.0 and v3.5.0 tags correspond to no published artifact and need deleting,
which is outward-facing and waits for an explicit go-ahead; the CHANGELOG's
[3.0.0] and [3.5.0] sections describe milestones that never shipped and should be
relabelled rather than deleted; and -- found while checking -- the CHANGELOG and
the published record diverge in BOTH directions, carrying a [2.2.10] that was
never published while missing 2.3.0 and 2.3.1, which were.

Verified: 1284 passed, 9 skipped; ruff clean.
@isayev
isayev merged commit ad7671d into main Aug 3, 2026
8 checks passed
@isayev
isayev deleted the phase17/quiet-paths branch August 3, 2026 18:38
isayev added a commit that referenced this pull request Aug 3, 2026
…ode behind it (#134)

Parallel conformer embedding existed as a constructor argument on the isomer
engine with no route from Auto3DOptions, so no main() or smiles2mols run could
turn it on and isomers/parallel_embed.py was reachable only from tests -- which is
why audit M53 listed that module as ~138 lines of dead code. Given the choice
between deleting a working feature and connecting it, this connects it.

Three fields, not one. Wiring only the boolean would have half-plumbed it:
parallel_workers and parallel_embedding_threshold are read by the same code path
(isomer_engine.py gates on `use_parallel_embedding and len(mols) >= threshold`) and
would have stayed at their constructor defaults, leaving the worker count and the
batch-size gate untunable from anywhere.

They flow Auto3DOptions -> CLIConfig -> both IsomerEngineFactory.create sites.
CLIConfig is not optional here: test_cliconfig_covers_all_auto3doptions_fields
requires every user-facing Auto3DOptions field to be reachable from the CLI/YAML
layer, which is also what makes these settable from a config file rather than
Python only.

Bounds live in FIELD_BOUNDS, not in Field(ge=1) on the pydantic model. I wrote the
Field constraints first; _check_bounds' own docstring warns that a second
hand-maintained constraint set is exactly the drift it exists to prevent, so they
were removed and the single table extended instead.

Default stays off: enabling it changes a run's resource profile, which should be
the caller's choice rather than something they discover.

The tests assert what the factory is CALLED with, not what the dataclass holds --
`Auto3DOptions(use_parallel_embedding=True).use_parallel_embedding is True` would
pass with the plumbing still missing, since it tests the dataclass and not the
wiring. Mutation-verified both ways: un-wiring the call site fails the reachability
test, and dropping the FIELD_BOUNDS entries fails both bounds cases.

M53's "test-only" claim for isomers/parallel_embed.py no longer holds and the
module is off that deletion list; #132's diagnostics fix now protects a path a
user can actually take.

Verified: 1297 passed, 9 skipped; ruff clean.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant