From dda8d77b30dc48462ecf39c18349c74c3913c0dc Mon Sep 17 00:00:00 2001 From: isayev Date: Mon, 3 Aug 2026 12:26:41 -0400 Subject: [PATCH] fix: make two quiet paths name the molecules they dropped 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. --- .../follow-ups-after-4.0.0-remediation.md | 27 ++++- src/Auto3D/batch_opt/batchopt.py | 21 +++- src/Auto3D/isomers/parallel_embed.py | 27 ++++- tests/test_workflow.py | 106 ++++++++++++++++++ 4 files changed, 172 insertions(+), 9 deletions(-) diff --git a/docs/superpowers/follow-ups-after-4.0.0-remediation.md b/docs/superpowers/follow-ups-after-4.0.0-remediation.md index d5c13304..50f9235f 100644 --- a/docs/superpowers/follow-ups-after-4.0.0-remediation.md +++ b/docs/superpowers/follow-ups-after-4.0.0-remediation.md @@ -32,9 +32,30 @@ durable record. correctness tests that assert unchanged results; the number comes from a maintainer-run benchmark. -**Version:** 4.0.0, not 3.5.0. `pyproject.toml` still reads `3.5.0` and must -move; the `v3.5.0` tag points at `0d21094`, many merges behind, and is retired -rather than moved. +**Version: 3.0.0** (revised 2026-08-03 from 4.0.0). No 3.x release ever reached a +package channel — **PyPI's latest is 2.3.1 and conda-forge's is 2.3.0** — so +skipping to 4.0.0 would leave users wondering where 3.x went. `2.3.1 → 3.0.0` is a +plain major bump and correctly signals the breaking changes. + +That collides with existing artifacts, and the resolution needs one approval +before anything destructive happens: + +- The `v3.0.0` and `v3.5.0` git tags exist but correspond to no published + artifact. They must be deleted and `v3.0.0` re-created at the release commit; + leaving `v3.0.0` pointing at old code while publishing a *different* 3.0.0 would + be worse than deleting it. Deleting pushed tags is outward-facing, so it waits + for an explicit go-ahead. +- The CHANGELOG's `[3.0.0] - 2026-01-02` and `[3.5.0] - 2026-06-13` sections + describe development milestones that never shipped. Recommendation: relabel them + as never-published rather than delete them, then retitle the unreleased `[4.0.0]` + section `[3.0.0]`. Preserves the record, removes the collision, tells the truth. +- **The CHANGELOG and the published record diverge in both directions**, found + while checking the above: the CHANGELOG has a `[2.2.10]` that was never + published, and is missing `2.3.0` and `2.3.1`, which were. The last CHANGELOG + entry corresponding to a real release is `2.2.9`. Worth fixing in the same pass. + +`pyproject.toml` still reads `3.5.0` and moves to `3.0.0` as part of the release +step. **Order:** correctness leftovers -> the *subset* of test-quality findings covering files the architecture work will move -> dead-code deletion -> one model contract diff --git a/src/Auto3D/batch_opt/batchopt.py b/src/Auto3D/batch_opt/batchopt.py index 22dc4a03..6aae893f 100644 --- a/src/Auto3D/batch_opt/batchopt.py +++ b/src/Auto3D/batch_opt/batchopt.py @@ -272,10 +272,23 @@ def run(self): logger.warning(f"Input file {self.in_f} is empty. Skipping optimization.") return - mols = list(Chem.SDMolSupplier(self.in_f, removeHs=False)) - - # Filter out None molecules (failed to parse) - mols = [m for m in mols if m is not None] + # Name every record that could not be parsed, not just the case where all + # of them failed. The all-failed warning below was the only signal, so a + # single bad record among a thousand left the output file shorter than the + # input with nothing said about which one -- for `opt_geometry` that is an + # output SDF with fewer records, the path returned and exit 0, and the only + # trace is RDKit's own C++ parse error on stderr, which names a file offset + # rather than a molecule. `SPE.calc_spe` and `ASE/thermo`'s + # `iter_thermo_records` both log per-record for the identical situation; + # this was the one reader that did not. + mols = [] + for index, mol in enumerate(Chem.SDMolSupplier(self.in_f, removeHs=False)): + if mol is None: + logger.warning( + "Skipping molecule at index %d: failed to parse", index + ) + continue + mols.append(mol) if not mols: logger.warning("No valid molecules in input file. Skipping optimization.") diff --git a/src/Auto3D/isomers/parallel_embed.py b/src/Auto3D/isomers/parallel_embed.py index f937fc82..544c8ff5 100644 --- a/src/Auto3D/isomers/parallel_embed.py +++ b/src/Auto3D/isomers/parallel_embed.py @@ -47,6 +47,13 @@ def _embed_single( # Validate SMILES first to avoid unpicklable Boost.Python errors mol_noh = Chem.MolFromSmiles(smi) if mol_noh is None: + # Same message the serial path emits (isomer_engine._run_serial_embedding). + # This branch returned [] in silence, so a molecule dropped for an + # unparseable SMILES was reported by the parallel path and not by the + # serial one -- a switch documented as a performance option decided how + # much the user was told. The parent also warns on an empty result, which + # is the guaranteed signal; this one adds the reason. + logger.warning(f"Skipping molecule {name!r}: failed to parse {smi!r}") return [] mol = Chem.AddHs(mol_noh) @@ -121,8 +128,9 @@ def embed_conformers_parallel( # order is deterministic and matches the serial path; all futures are # already running concurrently, so this costs no parallelism. for future in futures: + smi, name = futures[future] try: - yield from future.result() + conformers = future.result() except BrokenProcessPool: # A worker died (e.g. OOM-killed): the pool is broken and EVERY # remaining future will also raise this. Surface it loudly -- @@ -134,5 +142,20 @@ def embed_conformers_parallel( # RDKit's Boost.Python.ArgumentError, which is a TypeError and so # escaped the previous narrow except) must not abort the whole # batch and silently drop every remaining molecule. - smi, name = futures[future] logger.warning(f"Failed to embed {name}: {type(e).__name__}: {e}") + continue + if not conformers: + # The counterpart to the serial path's `n_written == 0` warning, + # which this path had no equivalent of. A species that embeds + # nothing -- unparseable SMILES, or every conformer rejected by + # clash relief -- is absent from the output and never reaches + # ranking, so not even "No structure converged" appears for it. + # Warned here, in the parent, because a message from a + # ProcessPoolExecutor worker depends on that child's logging + # configuration, while this one does not. + logger.warning( + f"{name!r} produced no conformers; this species is absent " + "from the output." + ) + continue + yield from conformers diff --git a/tests/test_workflow.py b/tests/test_workflow.py index a7096eb4..9f4d533f 100644 --- a/tests/test_workflow.py +++ b/tests/test_workflow.py @@ -950,3 +950,109 @@ def spy(smi_path, sdf_path): assert ethanol_id not in missing_ids assert len(missing_ids) == 1 assert any(missing_ids[0] in r.message for r in caplog.records) + + +class TestQuietPathsNameWhatTheyDropped: + """Two readers dropped molecules more quietly than their siblings. + + Both are the same defect: 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. + """ + + def test_the_optimizer_names_each_record_it_could_not_parse( + self, tmp_path, caplog, monkeypatch + ): + """`optimizing` logged only the all-records-failed case. + + A single bad record among a thousand left the output SDF shorter than the + input with nothing said about which one -- for `opt_geometry`, that is a + short file, the path returned, and exit 0. The only trace was RDKit's own + C++ parse error, which names a file offset rather than a molecule. + `SPE.calc_spe` and `ASE/thermo`'s `iter_thermo_records` both log + per-record for exactly this; this reader did not. + """ + from types import SimpleNamespace + + import torch + from rdkit import Chem + from rdkit.Chem import AllChem + + from Auto3D.batch_opt.batchopt import optimizing + + monkeypatch.setattr( + "Auto3D.batch_opt.batchopt.create_model", + lambda *a, **k: SimpleNamespace(coord_pad=0.0, species_pad=-1), + ) + + mol = Chem.AddHs(Chem.MolFromSmiles("CCO")) + AllChem.EmbedMolecule(mol, randomSeed=1) + mol.SetProp("_Name", "mol_a") + block = Chem.MolToMolBlock(mol).splitlines() + block[3] = "!! corrupted counts line !!" + # Every record unparseable, so this returns before any model is needed -- + # the per-record warning under test happens while reading the file. + bad_sdf = tmp_path / "bad.sdf" + bad_sdf.write_text("\n".join(block) + "\n$$$$\n") + + config = { + "opt_steps": 100, "opttol": 0.003, "patience": 100, + "batchsize_atoms": 1024, + } + optimizer = optimizing( + str(bad_sdf), str(tmp_path / "out.sdf"), "AIMNET", + torch.device("cpu"), config, + ) + + with caplog.at_level(logging.WARNING): + optimizer.run() + + assert "index 0" in caplog.text, ( + "the unparseable record was dropped without being named; only the " + f"all-failed case was reported. Log was: {caplog.text!r}" + ) + + def test_the_parallel_embed_path_names_a_species_it_produced_nothing_for( + self, caplog + ): + """The serial path warns twice here; the parallel path warned not at all. + + `_embed_single` returned `[]` for an unparseable SMILES in silence, and + `_run_parallel_embedding` had no counterpart to the serial path's + `n_written == 0` warning. So `use_parallel_embedding` -- documented as a + performance option -- decided whether a lost species was reported. + + The warning asserted here is the parent-side one, which is the guaranteed + signal: a message logged inside a ProcessPoolExecutor worker depends on + that child's logging configuration, and this one does not. + """ + from Auto3D.isomers.parallel_embed import embed_conformers_parallel + + with caplog.at_level(logging.WARNING): + results = list( + embed_conformers_parallel( + [("this-is-not-a-smiles", "bad_mol")], + n_conformers=1, + n_workers=1, + ) + ) + + assert results == [], "test premise: an unparseable SMILES embeds nothing" + assert "bad_mol" in caplog.text, ( + f"a species that produced no conformers was absent from the output " + f"with nothing logged. Log was: {caplog.text!r}" + ) + + def test_a_species_that_embeds_normally_is_not_warned_about(self, caplog): + """The new branch must not fire for a molecule that worked.""" + from Auto3D.isomers.parallel_embed import embed_conformers_parallel + + with caplog.at_level(logging.WARNING): + results = list( + embed_conformers_parallel( + [("CCO", "ethanol")], n_conformers=2, n_workers=1 + ) + ) + + assert results, "test premise: ethanol should embed" + assert "produced no conformers" not in caplog.text