From b829f29dbb1c361b349e66651f0c921eb62a2ad0 Mon Sep 17 00:00:00 2001 From: isayev Date: Mon, 3 Aug 2026 16:46:46 -0400 Subject: [PATCH] feat: make use_parallel_embedding reachable instead of deleting the code behind it 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. --- CHANGELOG.md | 18 +++++ .../follow-ups-after-4.0.0-remediation.md | 10 ++- src/Auto3D/auto3D.py | 3 + src/Auto3D/cli/config_schema.py | 13 ++++ src/Auto3D/config.py | 24 ++++++ src/Auto3D/workflow_workers.py | 3 + tests/test_config.py | 78 +++++++++++++++++++ 7 files changed, 146 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0af4c72..7e2b1878 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1279,6 +1279,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 into every CLI command (`run`, `energy`, `optimize`, `thermo`, `tautomers`, `models test`, and the legacy YAML path). +- **`use_parallel_embedding` is now reachable.** 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 the + module behind it was reachable only from tests — which is why an audit listed + `isomers/parallel_embed.py` as dead code. Three fields now flow from + `Auto3DOptions` through `CLIConfig` (so they work from a YAML config too) to + both isomer-engine construction sites: `use_parallel_embedding`, + `parallel_workers`, and `parallel_embedding_threshold`, the last of which keeps a + run serial below a given molecule count because spawning processes for a handful + of molecules costs more than it saves. + + Wiring only the boolean would have half-plumbed it: the other two are read by the + same code path and would have stayed at their constructor defaults, leaving the + worker count and the batch-size gate untunable. + + **Default is unchanged (off).** Enabling it changes a run's resource profile, + which should be the caller's choice rather than something they discover. + - **A monatomic molecule no longer crashes the ANI2xt thermochemistry path.** `aimnet_hessian_helper` built its species list with `numbers.squeeze()`, which collapses the `(1, 1)` tensor of a one-atom molecule to 0-d; `.tolist()` then 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 a6d5e99a..640a378b 100644 --- a/docs/superpowers/follow-ups-after-4.0.0-remediation.md +++ b/docs/superpowers/follow-ups-after-4.0.0-remediation.md @@ -94,9 +94,13 @@ diagnostics in `isomers/parallel_embed.py` — a module M53 lists for deletion. Re-verified: `use_parallel_embedding` is a constructor parameter of the isomer engine defaulting to `False`, with no plumbing from `Auto3DOptions` or the CLI, so no production path enables it and M53's "test-only" claim stands. The M2 fix is -correct but applies to a path no run takes. **Whether to delete the module is a -feature removal, not a dead-code cleanup, and needs a decision** — it is a public -constructor argument, so removing it changes a documented API. +correct but applies to a path no run takes. **RESOLVED 2026-08-03: wired through instead of deleted.** +`use_parallel_embedding`, `parallel_workers` and `parallel_embedding_threshold` now +flow from `Auto3DOptions` through `CLIConfig` to both isomer-engine construction +sites, so the option is reachable from Python and from a YAML config. M53's +"test-only" claim for `isomers/parallel_embed.py` no longer holds, and the module +is off that deletion list; the M2 diagnostics fix now protects a path a user can +actually take. **M53's inventory is partly stale — do not delete from it without re-checking.** Verified against current source: diff --git a/src/Auto3D/auto3D.py b/src/Auto3D/auto3D.py index 5d27964b..8442ca33 100644 --- a/src/Auto3D/auto3D.py +++ b/src/Auto3D/auto3D.py @@ -212,6 +212,9 @@ def smiles2mols(smiles: list[str], args: Auto3DOptions) -> list[Chem.Mol]: threshold=args.threshold, n_jobs=args.mpi_np, enumerate_isomers=args.enumerate_isomer, + use_parallel_embedding=args.use_parallel_embedding, + parallel_workers=args.parallel_workers, + parallel_embedding_threshold=args.parallel_embedding_threshold, ) isomer_engine.run() diff --git a/src/Auto3D/cli/config_schema.py b/src/Auto3D/cli/config_schema.py index 5201bd80..83a1fdb7 100644 --- a/src/Auto3D/cli/config_schema.py +++ b/src/Auto3D/cli/config_schema.py @@ -82,6 +82,16 @@ class CLIConfig(BaseModel): threshold: float = DEFAULT_RMSD_THRESHOLD batchsize_atoms: int = DEFAULT_BATCHSIZE_ATOMS + # Parallel conformer embedding. Mirrors Auto3DOptions, which + # test_cliconfig_covers_all_auto3doptions_fields requires -- and which is what + # makes these reachable from a YAML config rather than Python only. + # No Field(ge=1) here: the bounds live in Auto3D.config.FIELD_BOUNDS and are + # enforced by _check_bounds below. A second constraint declared here is the + # drift that validator's own docstring warns against. + use_parallel_embedding: bool = False + parallel_workers: int = 4 + parallel_embedding_threshold: int = 10 + # Resource settings memory: int | None = None capacity: int = DEFAULT_CAPACITY @@ -242,6 +252,9 @@ def to_auto3d_options(self, allow_missing_path: bool = False) -> Auto3DOptions: patience=self.patience, threshold=self.threshold, batchsize_atoms=self.batchsize_atoms, + use_parallel_embedding=self.use_parallel_embedding, + parallel_workers=self.parallel_workers, + parallel_embedding_threshold=self.parallel_embedding_threshold, memory=self.memory, capacity=self.capacity, allow_tf32=self.allow_tf32, diff --git a/src/Auto3D/config.py b/src/Auto3D/config.py index aa19464a..7edb7851 100644 --- a/src/Auto3D/config.py +++ b/src/Auto3D/config.py @@ -36,6 +36,8 @@ "patience": ("ge", 1), "threshold": ("gt", 0), "batchsize_atoms": ("ge", 1), + "parallel_workers": ("ge", 1), + "parallel_embedding_threshold": ("ge", 1), "memory": ("ge", 1), "capacity": ("ge", 1), "max_confs": ("ge", 1), @@ -297,6 +299,28 @@ class Auto3DOptions: memory: int | None = None """RAM size assigned to Auto3D in GB. None for automatic detection.""" + use_parallel_embedding: bool = False + """Embed conformers in parallel worker processes instead of serially. + + Off by default: parallel embedding spawns processes, so enabling it changes + a run's resource profile, and that should be the caller's choice rather than + something they discover. + + Until 3.0.0 this existed only as a constructor argument on the isomer engine + with no route from here, so no ``main()``/``smiles2mols`` run could reach it + and the code behind it was reachable only from tests. + """ + + parallel_workers: int = 4 + """Worker processes used when ``use_parallel_embedding`` is on.""" + + parallel_embedding_threshold: int = 10 + """Fewest molecules worth embedding in parallel. + + Below this count a run stays serial even with ``use_parallel_embedding`` on, + since spawning processes for a handful of molecules costs more than it saves. + """ + batchsize_atoms: int = DEFAULT_BATCHSIZE_ATOMS """Atoms per optimization batch, **per gigabyte** of detected GPU memory. diff --git a/src/Auto3D/workflow_workers.py b/src/Auto3D/workflow_workers.py index b9ba2e01..3870f9b0 100644 --- a/src/Auto3D/workflow_workers.py +++ b/src/Auto3D/workflow_workers.py @@ -169,6 +169,9 @@ def isomer_wrapper( n_jobs=mpi_np, enumerate_isomers=enumerate_isomer, mode=args.mode_oe if isomer_program == 'omega' else 'classic', + use_parallel_embedding=args.use_parallel_embedding, + parallel_workers=args.parallel_workers, + parallel_embedding_threshold=args.parallel_embedding_threshold, ) engine.run() diff --git a/tests/test_config.py b/tests/test_config.py index 43a21b4f..e8d1dc27 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -346,3 +346,81 @@ def test_a_real_k_is_unaffected(self): from Auto3D.config import Auto3DOptions assert Auto3DOptions(path="in.smi", k=1).k == 1 + + +class TestParallelEmbeddingIsReachable: + """The option must arrive at the isomer engine, not just exist on the config. + + Until 3.0.0 `use_parallel_embedding` was a constructor argument on the isomer + engine with no route from `Auto3DOptions`, so no `main()` or `smiles2mols` run + could reach it and `isomers/parallel_embed.py` was reachable only from tests -- + which is why an audit listed that module as dead code. + + Asserting `Auto3DOptions(use_parallel_embedding=True).use_parallel_embedding + is True` would pass with the plumbing still missing: it tests the dataclass, + not the wiring. These assert what the factory is actually called with. + """ + + def test_the_option_reaches_the_isomer_engine_factory(self, monkeypatch, tmp_path): + from Auto3D import auto3D as auto3D_mod + from Auto3D.config import Auto3DOptions + + seen = {} + + class _StubEngine: + def run(self): + raise RuntimeError("stop here: the factory call is what is asserted") + + def _capture(**kwargs): + seen.update(kwargs) + return _StubEngine() + + monkeypatch.setattr( + auto3D_mod.IsomerEngineFactory, "create", staticmethod(_capture) + ) + + smi = tmp_path / "in.smi" + smi.write_text("CCO ethanol\n") + options = Auto3DOptions( + # use_gpu=False: this box and CI are CPU-only, and check_gpu_requested + # is fatal for a GPU request with no visible device -- it would fire + # before the factory call under test. + path=str(smi), k=1, use_gpu=False, + use_parallel_embedding=True, + parallel_workers=3, + parallel_embedding_threshold=2, + ) + + with pytest.raises(RuntimeError, match="stop here"): + auto3D_mod.smiles2mols(["CCO"], options) + + assert seen.get("use_parallel_embedding") is True, ( + "use_parallel_embedding never reached the isomer engine: the field " + f"exists on the config but is not plumbed. Factory got: {sorted(seen)}" + ) + assert seen.get("parallel_workers") == 3, ( + "parallel_workers stayed at the constructor default, so enabling " + "parallel embedding could not control its worker count" + ) + assert seen.get("parallel_embedding_threshold") == 2, ( + "parallel_embedding_threshold stayed at its default, so the batch-size " + "gate could not be tuned" + ) + + def test_the_default_is_still_serial(self): + """Off by default: enabling it changes a run's resource profile.""" + from Auto3D.config import Auto3DOptions + + options = Auto3DOptions(path="in.smi", k=1) + assert options.use_parallel_embedding is False + + @pytest.mark.parametrize( + "field", ["parallel_workers", "parallel_embedding_threshold"] + ) + def test_a_count_below_one_is_rejected(self, field): + """Bounds come from FIELD_BOUNDS, so both entry points share them.""" + from Auto3D.config import Auto3DOptions + from Auto3D.exceptions import ConfigurationError + + with pytest.raises(ConfigurationError, match=field): + Auto3DOptions(path="in.smi", k=1, **{field: 0})