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