feat: make use_parallel_embedding reachable instead of deleting the code behind it - #134
Merged
Merged
Conversation
…ode 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.
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.
Resolves the decision left open by #132/#133: connect the feature rather than delete it.
The gap
Parallel conformer embedding existed as a constructor argument on the isomer engine with no route from
Auto3DOptions. Nomain()orsmiles2molsrun could turn it on, soisomers/parallel_embed.pywas reachable only from tests — which is why audit M53 listed it as ~138 lines of dead code. The factory already accepted all three parameters; only the config layer and the two call sites were missing.Three fields, not one
Wiring only the boolean would have half-plumbed it.
isomer_engine.pygates onuse_parallel_embedding and len(smi_name_tuples) >= parallel_embedding_threshold, so 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 from anywhere.use_parallel_embeddingFalseparallel_workers4parallel_embedding_threshold10They flow
Auto3DOptions→CLIConfig→ bothIsomerEngineFactory.createsites.CLIConfigis not optional here:test_cliconfig_covers_all_auto3doptions_fieldsrequires every user-facingAuto3DOptionsfield to be reachable from the CLI/YAML layer, which is also what makes these settable from a config file rather than Python only.Default stays off. Enabling it changes a run's resource profile, and that should be the caller's choice rather than something they discover.
A self-review catch worth recording
I first declared the bounds as
Field(default=4, ge=1)on the pydantic model._check_bounds' own docstring warns that a second hand-maintained constraint set is exactly the drift it exists to prevent — it enforcesAuto3D.config.FIELD_BOUNDSprecisely so the two schemas cannot disagree. TheFieldconstraints were removed and the single table extended instead.Testing
The tests assert what the factory is called with, not what the dataclass holds.
Auto3DOptions(use_parallel_embedding=True).use_parallel_embedding is Truewould pass with the plumbing still entirely missing — it tests the dataclass, not the wiring. That is the trap this codebase has hit repeatedly, so the test stubsIsomerEngineFactory.create, drivessmiles2mols, and asserts the three values arrived.Mutation-verified both ways:
smiles2molscall site → the reachability test failsFIELD_BOUNDSentries → both bounds cases failPlus a test that the default is still serial, so this cannot silently become opt-out.
1297 passed, 9 skipped; ruff clean.
Consequence for the deletion list
M53's "test-only" claim for
isomers/parallel_embed.pyno longer holds, and the module is off that list — recorded in the plan doc. #132's diagnostics fix now protects a path a user can actually take.