From d250fe9cfcf2f6e2b3fd1183e7e7692cd787f9c7 Mon Sep 17 00:00:00 2001 From: isayev Date: Tue, 4 Aug 2026 00:56:08 -0400 Subject: [PATCH 1/4] test: give the isomers, utils, config and filtering tests real assertions These cover the code the next several refactors move, so they are hardened first: a test that asserts nothing cannot tell you whether a refactor preserved behavior, and hardening it afterwards proves nothing about the move. The substantive changes, rather than the mechanical ones: test_parallel_embed's dynamic-conformer test used ethane, which is degenerate and always collapses to a single conformer, so the assertion held for a reason unrelated to what it claimed. It now uses hexane and bounds the result against calculate_conformer_count. test_SDF2chunks compared only chunk counts; it now reparses each chunk and asserts the molecules match the source by name, order and atom count. test_config's chunk-meta test built its expectation by hand from the same literal it then compared against. It now derives from ChunkMeta's own annotations and required keys. Two tests are deleted rather than hardened. test_config_schema_exists cannot fail once the module imports, and no field on Auto3DOptions has a mutable default for test_immutable_default_list to guard. Two xfail(strict=True) tests pin defects found while deriving the correct assertions. Both fail today for the reason named in the marker: - filter_unique_optimized raises KeyError on a record with no E_tot property, while the legacy filter_unique tolerates it. The two filters were given a shared duplicate criterion but still disagree on malformed input, and the stricter one is the survivor. - opt_steps carries two minimums: FIELD_BOUNDS declares ("ge", 1) while validation.py hand-writes >= 10 in two places. The test asserts the two agree rather than picking a value, so whichever is authoritative can win. Verified: 1299 passed, 9 skipped, 2 xfailed. --- tests/test_cli_config_schema.py | 6 --- tests/test_cli_security.py | 11 ++++- tests/test_config.py | 75 +++++++++++++++++++++-------- tests/test_filtering.py | 74 +++++++++++++++++++++++++++- tests/test_isomer_engine.py | 28 +++++++++++ tests/test_isomers.py | 85 +++++++++++++++++++++++++++------ tests/test_parallel_embed.py | 26 ++++++++-- tests/test_utils_chemistry.py | 52 ++++++++++++++------ tests/test_utils_validation.py | 44 ++++++++++++----- 9 files changed, 326 insertions(+), 75 deletions(-) diff --git a/tests/test_cli_config_schema.py b/tests/test_cli_config_schema.py index 113ccd8..047c5df 100644 --- a/tests/test_cli_config_schema.py +++ b/tests/test_cli_config_schema.py @@ -5,12 +5,6 @@ from pathlib import Path -def test_config_schema_exists(): - """Config schema class should exist.""" - from Auto3D.cli.config_schema import CLIConfig - assert CLIConfig is not None - - def test_config_defaults(): """Config should have sensible defaults.""" from Auto3D.cli.config_schema import CLIConfig diff --git a/tests/test_cli_security.py b/tests/test_cli_security.py index 7350eeb..610519e 100644 --- a/tests/test_cli_security.py +++ b/tests/test_cli_security.py @@ -107,15 +107,22 @@ def test_a_whitespace_only_file_is_refused(self, tmp_path): load_yaml_config(cfg) def test_a_top_level_list_is_refused(self, tmp_path): + """``match=`` anchors on the phrase unique to THIS guard's message + ("...its top level is a {type}.") -- "must contain a YAML mapping" is + also a substring of the empty-file message (config_schema.py:341), so + that alone cannot tell this test apart from the empty-file guard + firing by mistake (e.g. if the not-a-mapping check were deleted and + the empty-file check's message merely happened to also match). + """ cfg = _write(tmp_path, "- k\n- window\n") - with pytest.raises(ConfigurationError, match="must contain a YAML mapping"): + with pytest.raises(ConfigurationError, match="top level is a list"): load_yaml_config(cfg) def test_a_top_level_scalar_is_refused(self, tmp_path): cfg = _write(tmp_path, "just a bare string\n") - with pytest.raises(ConfigurationError, match="must contain a YAML mapping"): + with pytest.raises(ConfigurationError, match="top level is a str"): load_yaml_config(cfg) def test_unparseable_yaml_is_refused(self, tmp_path): diff --git a/tests/test_config.py b/tests/test_config.py index e8d1dc2..27376a0 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -125,34 +125,33 @@ def test_gpu_idx_list(self): config = Auto3DOptions(gpu_idx=[0, 1, 2]) assert config.gpu_idx == [0, 1, 2] - def test_immutable_default_list(self): - """Test that default list values are not shared between instances.""" - config1 = Auto3DOptions() - config2 = Auto3DOptions() - - # If gpu_idx is a list, modifying one shouldn't affect the other - # But with default int value, this just verifies independence - config1["k"] = 5 - assert config2.k is False # Default is False, not None - class TestChunkMeta: """Tests for ChunkMeta TypedDict.""" def test_chunk_meta_structure(self): - """Test that ChunkMeta can be used as expected.""" + """Track the TypedDict's OWN declared keys, not a hand-copied literal. + + The prior version built its own 5-key dict and asserted back the + values it had just set -- a key added to or removed from the real + ChunkMeta would never move this test. A TypedDict has no runtime + constructor to validate against, so pin it via ``__annotations__``/ + ``__required_keys__`` instead: a change to config.py's ChunkMeta now + has to be reflected here too, or this test fails. + """ from Auto3D.config import ChunkMeta - meta: ChunkMeta = { - "output": "/path/to/output.sdf", - "optimized_og": "/path/to/optimized.sdf", - "enumerated_sdf": "/path/to/enumerated.sdf", - "sorted_sdf": "/path/to/sorted.sdf", - "housekeeping_folder": "/path/to/housekeeping", + expected_keys = { + "output", "optimized_og", "output_taut", "smiles_enumerated", + "smiles_reduced", "smiles_hashed", "enumerated_sdf", "sorted_sdf", + "housekeeping_folder", "path", "dir", } - - assert meta["output"] == "/path/to/output.sdf" - assert meta["housekeeping_folder"] == "/path/to/housekeeping" + assert set(ChunkMeta.__annotations__) == expected_keys + # ChunkMeta declares no Optional/NotRequired fields, so every key is + # required -- a self-consistency check that would catch a field + # becoming optional without a matching intent. + assert ChunkMeta.__required_keys__ == frozenset(expected_keys) + assert ChunkMeta.__optional_keys__ == frozenset() def test_optimization_config_exposes_no_energy_criterion_knobs(): @@ -186,6 +185,42 @@ def test_capacity_default_matches_across_layers(): assert Auto3DOptions(path="x.smi").capacity == CLIConfig(path="x.smi").capacity +@pytest.mark.xfail( + strict=True, + reason=( + "config.py FIELD_BOUNDS['opt_steps'] declares a floor of ('ge', 1), " + "but Auto3D.utils.validation.check_valid_configuration (validation.py:621) " + "and check_input (validation.py:354) each hand-write a floor of 10. " + "A value FIELD_BOUNDS calls valid is rejected by the other validator. " + "This test does not decide which number is right -- only that the two " + "must agree on ONE minimum, whichever the eventual fix picks." + ), +) +def test_opt_steps_minimum_agrees_between_config_and_validation(): + """FIELD_BOUNDS['opt_steps'] and utils.validation's opt_steps floor must + be the SAME minimum -- see the xfail reason for the defect this pins. + """ + from Auto3D.config import FIELD_BOUNDS, check_field_bounds + from Auto3D.utils.validation import check_valid_configuration + + kind, bound_min = FIELD_BOUNDS["opt_steps"] + assert kind == "ge" + + # config.py's own gate must accept its own declared floor (premise, not + # the point of this test). + check_field_bounds({"opt_steps": bound_min}) # must not raise + + # utils.validation must not disagree with config.py's declared floor: + # a value config.py calls valid must not be flagged as an error there. + errors = check_valid_configuration( + path=None, k=1, opt_steps=int(bound_min), use_gpu=False, + ) + assert not any("opt_steps" in e for e in errors), ( + f"config.py says opt_steps={bound_min} is valid (FIELD_BOUNDS), but " + f"utils.validation.check_valid_configuration disagrees: {errors}" + ) + + def test_negative_k_rejected(): from Auto3D.config import Auto3DOptions from Auto3D.exceptions import ConfigurationError diff --git a/tests/test_filtering.py b/tests/test_filtering.py index 354fe1d..c362646 100644 --- a/tests/test_filtering.py +++ b/tests/test_filtering.py @@ -61,10 +61,24 @@ def test_empty_list_returns_empty(self): assert result == [] def test_single_mol_returns_itself(self): - """Single molecule should be returned as-is.""" + """Single molecule should be returned as-is, explicit Hs and all. + + The RMSD comparison strips Hs from a throwaway copy for speed; the + returned molecule must be the caller's original (H-explicit) object, + not the no-H comparison copy -- the MLIP downstream requires explicit + H, and this is the len(mols) <= 1 short-circuit that never even + reaches the strip/compare loop. + """ mol = _create_mol_with_energy("C", -10.0) + n_atoms_before = mol.GetNumAtoms() + assert any(a.GetAtomicNum() == 1 for a in mol.GetAtoms()), "test premise: has explicit Hs" + result = _filter_within_cluster([mol], rmsd_threshold=0.5) + assert len(result) == 1 + assert result[0] is mol, "the single-mol short-circuit must return the original object" + assert result[0].GetNumAtoms() == n_atoms_before + assert any(a.GetAtomicNum() == 1 for a in result[0].GetAtoms()) def test_identical_mols_returns_one(self): """Identical molecules should be deduplicated to one.""" @@ -217,11 +231,17 @@ def test_empty_list_returns_empty(self): assert result == [] def test_filters_unconverged_structures(self): - """Unconverged structures should be filtered out.""" + """Unconverged structures should be filtered out -- and it must be + specifically the unconverged one that is gone, not just any one of + the two (e.g. a dedup bug that merged them for an unrelated reason + would also leave len(result) == 1). + """ mol1 = _create_mol_with_energy("C", -10.0, converged=True) mol2 = _create_mol_with_energy("CC", -11.0, converged=False) result = filter_unique_optimized([mol1, mol2], rmsd_threshold=0.5) assert len(result) == 1 + assert result[0] is mol1, "the converged structure must be the survivor" + assert result[0].GetProp("Converged").lower() == "true" def test_removes_duplicates(self): """Optimized filter should remove similar structures.""" @@ -311,6 +331,56 @@ def test_small_energy_window_creates_separate_clusters(self): assert len(result) == 2 +class TestMissingEnergyPropertyMustNotCrash: + """filter_unique_optimized must tolerate a record with no 'E_tot', the + way the legacy ``utils.chemistry.filter_unique`` already does. + + KNOWN DEFECT (found during cluster E brainstorming, not fixed by this + lane): ``filtering.py:75`` sorts the valid-mols list by + ``Auto3D.utils.energy.e_tot_ev``, which RAISES (KeyError/ValueError) for + a molecule with no usable 'E_tot' property. ``_filter_within_cluster``'s + own energy guard, two dozen lines later in the same module, instead uses + the tolerant ``try_e_tot_ev`` and treats a missing energy as "fall back + to RMSD only". ``utils.chemistry.filter_unique`` (the OTHER conformer + filter, sharing the same duplicate criterion since 4.0.1) also uses + ``try_e_tot_ev`` throughout and does not crash on this input. So the two + filters diverge on malformed input: the same list of mols that + ``filter_unique`` happily filters crashes ``filter_unique_optimized``. + + This matters here specifically because cluster B5 is about to delete one + of the two filters, and the survivor is the stricter (crashing) one -- + fixing filtering.py's sort key to use ``try_e_tot_ev``, matching its own + energy guard and the legacy filter, is what should make this pass. + """ + + @pytest.mark.xfail( + strict=True, + reason=( + "filtering.py:75 sorts by e_tot_ev (raises KeyError for a mol " + "with no 'E_tot' property) instead of the tolerant try_e_tot_ev " + "that _filter_within_cluster's own energy guard and the legacy " + "utils.chemistry.filter_unique both use -- the two conformer " + "filters disagree on malformed input (cluster E brainstorm defect)." + ), + ) + def test_missing_e_tot_property_does_not_crash(self): + mol_no_energy = Chem.AddHs(Chem.MolFromSmiles("CCO")) + AllChem.EmbedMolecule(mol_no_energy, randomSeed=42) + mol_no_energy.SetProp("Converged", "true") + # Deliberately no set_e_tot_from_ev call: this record has no 'E_tot'. + assert not mol_no_energy.HasProp("E_tot"), "test premise" + + mol_with_energy = _create_mol_with_energy("CC", -10.0) + + result = filter_unique_optimized( + [mol_no_energy, mol_with_energy], rmsd_threshold=0.3 + ) + + # Correct behavior: no crash, and a mol with no usable energy simply + # cannot be deduped by energy -- it must survive alongside the other. + assert len(result) == 2 + + class TestFilterUniqueBehavior: """Tests verifying behavior matches original filter_unique.""" diff --git a/tests/test_isomer_engine.py b/tests/test_isomer_engine.py index da92505..393df1c 100644 --- a/tests/test_isomer_engine.py +++ b/tests/test_isomer_engine.py @@ -92,9 +92,37 @@ def test_rd_isomer_conformer_func(): def test_SDF2chunks(): + """Chunks must partition the source records exactly: same count, same + identity (by name) in the same order, and same atom count each -- not + merely the same total count, which passes even if a chunk boundary + duplicated or dropped a molecule and another chunk absorbed the slack. + """ chunks = SDF2chunks(example_sdf) assert(len(chunks) == count_sdf(example_sdf)) + reference_mols = [ + mol for mol in Chem.SDMolSupplier(example_sdf, removeHs=False) + if mol is not None + ] + chunk_mols = [ + Chem.MolFromMolBlock("".join(chunk), removeHs=False) for chunk in chunks + ] + assert all(mol is not None for mol in chunk_mols), ( + "a chunk failed to parse back into a molecule" + ) + + reference_names = [m.GetProp("_Name") for m in reference_mols] + chunk_names = [m.GetProp("_Name") for m in chunk_mols] + assert chunk_names == reference_names, ( + "chunks do not reproduce the source records' identity/order" + ) + + reference_atom_counts = [m.GetNumAtoms() for m in reference_mols] + chunk_atom_counts = [m.GetNumAtoms() for m in chunk_mols] + assert chunk_atom_counts == reference_atom_counts, ( + "a chunk's atom count does not match its source record" + ) + def test_rd_isomer_with_parallel_embedding(): """Test RDKitIsomer with parallel embedding enabled.""" diff --git a/tests/test_isomers.py b/tests/test_isomers.py index dc8e790..8a23fda 100644 --- a/tests/test_isomers.py +++ b/tests/test_isomers.py @@ -170,15 +170,23 @@ def test_unknown_engine_raises_error(self): ) def test_engine_type_case_insensitive(self): - """Test that engine type is case insensitive.""" - # This should not raise - it will fail on actual instantiation - # but the case normalization should work - with pytest.raises(ValueError, match="Unknown isomer engine type"): - create_isomer_engine( - "UNKNOWN", + """A *valid* engine name in an unexpected case must resolve to the + correct adapter, not merely fail to crash on an already-invalid name. + + The previous version passed "UNKNOWN" -- invalid in any case -- so it + could never have distinguished case normalization working from case + normalization being entirely absent. + """ + for name in ("RDKit", "RDKIT", "rdkit"): + engine = create_isomer_engine( + name, input_path="/input.smi", output_path="/output.sdf", + smiles_enumerated="/enum.smi", + smiles_reduced="/reduced.smi", + smiles_hashed="/hashed.smi", ) + assert isinstance(engine, RDKitIsomerAdapter), name def test_omega_engine_creates_adapter(self): """Test that 'omega' creates OmegaIsomerAdapter.""" @@ -257,6 +265,52 @@ def test_rdkit_engine_parallel_embedding_enabled(self, tmp_path): assert engine.parallel_embedding_threshold == 5 assert engine.parallel_workers == 2 + def test_rdkit_engine_parallel_embedding_enabled_actually_runs_parallel_path( + self, tmp_path, monkeypatch + ): + """Constructor kwargs alone don't prove the parallel path executes. + + Drive ``.run()`` for real (small, hermetic, no NNP) and spy on + ``embed_conformers_parallel`` -- the parallel path's only entry point + -- so a regression that silently falls back to serial embedding + would be caught even though every attribute above still reports + correctly. + """ + import Auto3D.isomers.parallel_embed as parallel_embed_mod + + job_dir = tmp_path / "job" + job_dir.mkdir() + smi = tmp_path / "in.smi" + smi.write_text("CCO ethanol\n") + + calls = {"n": 0} + + def spy(*args, **kwargs): + calls["n"] += 1 + return iter([]) # no conformers written; only the call matters + + monkeypatch.setattr(parallel_embed_mod, "embed_conformers_parallel", spy) + + engine = create_isomer_engine( + "rdkit", + input_path=str(smi), + output_path=str(tmp_path / "output.sdf"), + smiles_enumerated=str(tmp_path / "enum.smi"), + smiles_reduced=str(tmp_path / "reduced.smi"), + smiles_hashed=str(tmp_path / "hashed.smi"), + job_dir=str(job_dir), + use_parallel_embedding=True, + parallel_embedding_threshold=1, # even one molecule triggers it + parallel_workers=2, + ) + + engine.run() + + assert calls["n"] == 1, ( + "embed_conformers_parallel was never called: the parallel path " + "did not run despite use_parallel_embedding=True" + ) + class TestCreateTautomerEngine: """Tests for create_tautomer_engine factory function.""" @@ -271,14 +325,17 @@ def test_unknown_engine_raises_error(self): ) def test_engine_type_case_insensitive(self): - """Test that engine type is case insensitive for valid types.""" - # Both RDKIT and rdkit should work (case normalization happens) - with pytest.raises(ValueError, match="Unknown tautomer engine type"): - create_tautomer_engine( - "UNKNOWN", - input_path="/input.smi", - output_path="/output.smi", - ) + """A *valid* engine name ("RDKIT") must resolve to the same + rdkit-backed engine as lowercase "rdkit" -- not merely fail to crash + on an already-invalid name, which "UNKNOWN" could never distinguish. + """ + from Auto3D.isomer_engine import TautomerEngine as TautEngine + + engine = create_tautomer_engine( + "RDKIT", input_path="/input.smi", output_path="/output.smi" + ) + assert isinstance(engine, TautEngine) + assert engine.mode == "rdkit" class TestIsomerEngineFactory: diff --git a/tests/test_parallel_embed.py b/tests/test_parallel_embed.py index 7643f40..40b5e1a 100644 --- a/tests/test_parallel_embed.py +++ b/tests/test_parallel_embed.py @@ -39,16 +39,34 @@ def test_embed_single_returns_list_of_tuples(self): assert "methane" in conf_id def test_embed_single_with_dynamic_conformers(self): - """_embed_single with n_conformers=None should use dynamic calculation.""" + """_embed_single with n_conformers=None should use dynamic calculation. + + Compare the actual conformer count against ``calculate_conformer_count``'s + own formula for this molecule, instead of a bare ">= 1" that would + pass even if the None branch silently stopped calling that formula + (e.g. fell back to a fixed conformer count). Hexane is flexible + enough that the two counts are not degenerate (unlike a rigid/small + molecule, where embedding + RMSD pruning collapses to 1 regardless of + the requested count and could hide a formula regression). + """ + from Auto3D.utils.chemistry import calculate_conformer_count + + mol = Chem.AddHs(Chem.MolFromSmiles("CCCCCC")) # hexane: flexible + expected_upper_bound = calculate_conformer_count(mol) + results = _embed_single( - smi="CC", - name="ethane", + smi="CCCCCC", + name="hexane", n_conformers=None, threshold=0.3, np_threads=1, ) - assert len(results) >= 1 + assert 1 <= len(results) <= expected_upper_bound, ( + f"expected between 1 and {expected_upper_bound} conformers " + "(calculate_conformer_count's own dynamic formula for hexane), " + f"got {len(results)}" + ) def test_embed_single_filters_invalid_conformers(self): """_embed_single should filter conformers with atom clashes.""" diff --git a/tests/test_utils_chemistry.py b/tests/test_utils_chemistry.py index e5b3c60..f0b8345 100644 --- a/tests/test_utils_chemistry.py +++ b/tests/test_utils_chemistry.py @@ -370,27 +370,44 @@ def test_amend_mol_returns_none_for_invalid(self): # (either returns None or attempts to fix) def test_amend_mol_with_sanitize(self): - """Test that amend_mol can sanitize molecules.""" + """amend_mol(sanitize=True) must actually run RDKit's sanitization, + not just return a non-None object. + + A molecule parsed with ``sanitize=False`` has no ring-perception / + implicit-valence pass yet, so querying ring info raises a + precondition violation -- it genuinely needs sanitizing to become + usable. If ``amend_mol``'s sanitize branch were a no-op, this + molecule would still raise after the call. + """ from Auto3D.utils.chemistry import amend_mol - mol = Chem.MolFromSmiles("CCO") - mol = Chem.AddHs(mol) - AllChem.EmbedMolecule(mol, randomSeed=42) + + mol = Chem.MolFromSmiles("c1ccccc1", sanitize=False) # benzene, unsanitized + with pytest.raises(RuntimeError): + mol.GetRingInfo().NumRings() # ring perception never ran amended_mol = amend_mol(mol, sanitize=True) + assert amended_mol is not None + # Sanitizing actually ran: ring perception now works and finds the ring. + assert amended_mol.GetRingInfo().NumRings() == 1 class TestGetMolConnectivity: """Test the get_mol_connectivity function.""" def test_ethane_connectivity(self): - """Test connectivity for ethane (C-C single bond).""" + """Test connectivity for ethane (C-C single bond). + + Pins the exact canonical ordering (atom1_idx < atom2_idx, per the + function's own docstring/example), not "either order" -- which would + equally accept a broken ``get_mol_connectivity`` that stopped sorting + its tuples. + """ from Auto3D.utils.chemistry import get_mol_connectivity mol = Chem.MolFromSmiles("CC") connectivity = get_mol_connectivity(mol) - # Should have C-C bond - assert (0, 1) in connectivity or (1, 0) in connectivity + assert connectivity == {(0, 1)} def test_ethanol_connectivity(self): """Test connectivity for ethanol.""" @@ -425,18 +442,22 @@ def test_methane_connectivity(self): assert len(connectivity_with_h) == 4 # 4 C-H bonds def test_include_bond_order(self): - """Test that bond order can be included.""" + """Test that bond order can be included. + + The previous version's real assertion sat inside ``if len(bond_info) + == 3:``, which is false exactly when ``include_bond_order`` silently + stops adding the third element -- the one failure mode this test + exists to catch. Assert the 3-tuple shape unconditionally, then the + bond order value. + """ from Auto3D.utils.chemistry import get_mol_connectivity mol = Chem.MolFromSmiles("C=C") # Ethene connectivity = get_mol_connectivity(mol, include_bond_order=True) - # Should contain tuple with bond order info - # Format: (atom1_idx, atom2_idx, bond_order) - assert len(connectivity) >= 1 + assert connectivity == {(0, 1, 2.0)} for bond_info in connectivity: - if len(bond_info) == 3: - # Has bond order - assert bond_info[2] == 2.0 # Double bond + assert len(bond_info) == 3 # (atom1_idx, atom2_idx, bond_order) + assert bond_info[2] == 2.0 # Double bond class TestFilterUnique: @@ -633,6 +654,9 @@ def test_filter_custom_threshold(self): # Large threshold should definitely merge identical mols assert len(unique_mols_large) == 1 + # A tighter threshold can never merge MORE than a looser one -- the + # discarded half of this test's own computation, now actually checked. + assert len(unique_mols_small) >= len(unique_mols_large) def test_filter_unique_removehs_is_linear_and_nondestructive(self, monkeypatch): """Legacy filter_unique strips Hs once per molecule (not per comparison) and diff --git a/tests/test_utils_validation.py b/tests/test_utils_validation.py index 732380f..75997c9 100644 --- a/tests/test_utils_validation.py +++ b/tests/test_utils_validation.py @@ -223,19 +223,37 @@ def test_filter_unique_keeps_records_with_no_converged_property(self): ) def test_filter_unique_custom_threshold(self): - """Test filter_unique with custom RMSD threshold.""" - supp = Chem.SDMolSupplier(path_example_sdf, removeHs=False) - mols = [mol for mol in supp if mol is not None] - - for mol in mols: - mol.SetProp("Converged", "True") - - # With very small threshold, more structures should be kept - result_strict = filter_unique(mols, crit=0.01) - # With larger threshold, fewer structures should be kept - result_lenient = filter_unique(mols, crit=1.0) - - assert len(result_strict) >= len(result_lenient) + """A tighter RMSD threshold must keep MORE structures than a looser + one -- not merely "at least as many", which passes even when the two + thresholds produce identical results (as the fixture in + ``path_example_sdf`` does: two molecules of different sizes, so + ``species_key`` alone already keeps both regardless of ``crit``). + + Constructs two conformers of ONE molecule whose RMSD sits strictly + between the two thresholds, so equality cannot pass silently. + """ + mol1 = Chem.AddHs(Chem.MolFromSmiles("CCCCCCCC")) # octane: flexible + mol2 = Chem.Mol(mol1) + from rdkit.Chem import AllChem, rdMolAlign + AllChem.EmbedMolecule(mol1, randomSeed=1) + AllChem.EmbedMolecule(mol2, randomSeed=99) + AllChem.MMFFOptimizeMolecule(mol1) + AllChem.MMFFOptimizeMolecule(mol2) + mol1.SetProp("Converged", "True") + mol2.SetProp("Converged", "True") + + rmsd = rdMolAlign.GetBestRMS(Chem.RemoveHs(mol1), Chem.RemoveHs(mol2)) + assert rmsd > 0.05, "test premise: conformers must be geometrically distinct" + + crit_strict = rmsd / 2 # below the actual RMSD -> kept separate + crit_lenient = rmsd * 2 # above the actual RMSD -> merged + + result_strict = filter_unique([mol1, mol2], crit=crit_strict) + result_lenient = filter_unique([mol1, mol2], crit=crit_lenient) + + assert len(result_strict) == 2, "strict threshold must not merge distinct conformers" + assert len(result_lenient) == 1, "lenient threshold must merge near-identical conformers" + assert len(result_strict) > len(result_lenient) class TestCheckValidConfiguration: From e31b5a829edea12ac60428508c8609e9ee16e9f0 Mon Sep 17 00:00:00 2001 From: isayev Date: Tue, 4 Aug 2026 00:56:52 -0400 Subject: [PATCH 2/4] perf!: stop importing torch and rdkit to import Auto3D Three eager optional-dependency probes in __init__.py defeated the _LAZY_API mechanism that exists to avoid exactly this. Probing for ANI2xt reached batch_opt, which reached the utils barrel, which reached validation, which imported torch and models.loading. So `import Auto3D` paid for the entire package plus torch and rdkit before the caller had asked for anything. Measured in a subprocess on one box: import Auto3D 1.35 s -> 0.031 s len(sys.modules) 1175 -> 154 (stdlib floor is 128) torch, rdkit loaded yes -> no Auto3D.* submodules 20 -> 0 The probes are deleted rather than made lazy: they are not public API, and every real probe already exists at its use site. A comment records that and says not to reintroduce one. __version__ now comes from a private _detect_version(), and __dir__ reports the lazy API, so `main` is discoverable and warnings, version, ANI2xt and PackageNotFoundError stop leaking into the namespace. __getattr__ deliberately does not cache. Caching would turn it into a capturing binding, which is how a lazily imported reader ends up holding a stub permanently while monkeypatch reports success; test_lazy_torchani_import documents that failure appearing 182 tests downstream. A test asserts the non-caching property. The cost tests assert module count rather than wall-clock seconds, and run in a subprocess since the parent pytest process has already imported everything. Verified: five tripwires confirmed red first for the stated reasons, then each fix mutation-verified by reverting it and confirming the named test goes red. 1298 passed across three random seeds plus fixed order; ruff clean. --- src/Auto3D/__init__.py | 84 ++++++++--- tests/test_import_boundaries.py | 252 ++++++++++++++++++++++++++++++++ 2 files changed, 315 insertions(+), 21 deletions(-) create mode 100644 tests/test_import_boundaries.py diff --git a/src/Auto3D/__init__.py b/src/Auto3D/__init__.py index 54501df..613017a 100644 --- a/src/Auto3D/__init__.py +++ b/src/Auto3D/__init__.py @@ -5,32 +5,45 @@ using neural network potentials (AIMNet2, ANI2x, ANI2xt). """ -import warnings -from importlib.metadata import PackageNotFoundError, version -try: - __version__ = version(__name__) -except PackageNotFoundError: - __version__ = "unknown" +def _detect_version() -> str: + """Read the installed distribution version, or "unknown" if not installed. -# Optional dependency imports with proper exception handling -with warnings.catch_warnings(): - warnings.simplefilter("ignore") + The ``importlib.metadata`` import lives inside this function on purpose: at + module level it would put ``version`` and ``PackageNotFoundError`` into the + package namespace, where they are reachable as ``Auto3D.version`` and + ``Auto3D.PackageNotFoundError`` -- two names this package never meant to + export. See ``tests/test_import_boundaries.py``. + """ + from importlib.metadata import PackageNotFoundError, version try: - from openeye import oechem, oeomega, oequacpac # noqa: F401 (optional dependency probe) - except ImportError: - pass # OpenEye is optional + return version(__name__) + except PackageNotFoundError: + return "unknown" - try: - import torchani # noqa: F401 (optional dependency probe) - except ImportError: - pass # TorchANI is optional - try: - from Auto3D.batch_opt.ANI2xt_no_rep import ANI2xt # noqa: F401 (optional dependency probe) - except ImportError: - pass # ANI2xt model is optional +__version__ = _detect_version() + +# NOTE: this module imports nothing but the standard library, and that is a +# tested property, not an accident (tests/test_import_boundaries.py). Importing +# the package root is the cost every consumer pays unconditionally -- `auto3d +# --help`, a build script reading `__version__`, a tool that merely lists +# installed distributions -- and none of them need torch or rdkit. +# +# This file used to end with three eager optional-dependency probes (openeye, +# torchani, and `from Auto3D.batch_opt.ANI2xt_no_rep import ANI2xt`) wrapped in +# `warnings.catch_warnings()`. Nothing consumed any of them, and the third one +# defeated the `_LAZY_API` mechanism below outright: it reached ANI2xt_no_rep -> +# the Auto3D.utils barrel -> utils.validation -> Auto3D.models.* + torch, which +# turned `import Auto3D` into 1175 modules and 1.35 s and eagerly loaded 20 +# Auto3D submodules. Every probe is already duplicated where it is load-bearing +# and where its result is actually used: +# * openeye -> isomer_engine.py (names used at call time) and +# utils/validation.py (raises DependencyError with a fix hint) +# * torchani -> utils/validation.py (same) and batch_opt/ANI2xt_no_rep.py +# * ANI2xt -> constructed only through model_factory / models.adapter +# Do not reintroduce a probe here. A dependency is checked where it is needed. __all__ = [ "__version__", @@ -73,9 +86,38 @@ # Lazy imports for public API def __getattr__(name: str): - """Lazy import for public API functions (see _LAZY_API).""" + """Lazy import for public API functions (see _LAZY_API). + + Design constraint, not a missed optimization: this **must not** cache the + resolved object into ``globals()``. Caching would turn every access after + the first into a snapshot, i.e. exactly the import-time binding that makes + ``from X import y`` capture a stub -- a test that touches ``Auto3D.main`` + and then patches ``Auto3D.auto3D.main`` would patch nothing, report + success, and fail somewhere else entirely (the mechanism written up at + length in ``tests/test_lazy_torchani_import.py``). After the first access + ``import_module`` is a ``sys.modules`` dict lookup, so a cache buys nothing + measurable. Pinned by + ``test_import_boundaries.py::test_getattr_does_not_cache_resolved_attributes``. + """ if name in _LAZY_API: import importlib module_name, attr = _LAZY_API[name] return getattr(importlib.import_module(module_name), attr) raise AttributeError(f"module {__name__!r} has no attribute {name!r}") + + +def __dir__() -> list[str]: + """Report the public API alongside whatever is genuinely present. + + PEP 562 requires this to accompany ``__getattr__``: a module-level + ``__getattr__`` resolves names that are not in ``globals()``, and the + default ``dir()`` only sees ``globals()`` -- so without this, ``"main" in + dir(Auto3D)`` was False and neither tab-completion nor introspection could + find the public API. + + The union (rather than ``sorted(__all__)``) matters because ``__dir__`` + replaces the default entirely: imported submodules become real attributes + of the package, so ``Auto3D.cli`` after ``import Auto3D.cli`` must stay + visible. + """ + return sorted(set(globals()) | set(__all__)) diff --git a/tests/test_import_boundaries.py b/tests/test_import_boundaries.py new file mode 100644 index 0000000..d70e35c --- /dev/null +++ b/tests/test_import_boundaries.py @@ -0,0 +1,252 @@ +"""Import boundaries for the ``Auto3D`` package root: what ``import Auto3D`` +is allowed to cost, and what the package root is allowed to expose. + +Two properties are locked here. + +**Import cost.** ``import Auto3D`` must reach nothing but the standard library. +The package root is the entry point every consumer pays for -- ``auto3d +--help``, ``from Auto3D import __version__`` in a build script, an unrelated +library that merely lists installed packages -- and none of those need torch, +rdkit, or a neural network potential. The cost is asserted as a **module count** +rather than a wall-clock time because seconds are a property of the machine +(CPU, warm page cache, NFS-mounted site-packages) while ``len(sys.modules)`` is +a property of the code. Eager optional-dependency probes in ``__init__.py`` once +made this 1175 modules / 1.35 s; the cap below leaves generous headroom over the +stdlib-only floor so it tracks a regression in kind, not a fluctuation in +degree. + +**Public surface.** ``__getattr__`` without ``__dir__`` violates PEP 562: +``dir()`` stops reporting the lazily resolved names (``"main" in dir(Auto3D)`` +was False) while the module namespace still exposes whatever the module body +happened to import. Both halves are asserted -- the public names are visible, +and the import-machinery names are not. + +Every cost measurement runs in a **subprocess**. It cannot be done in-process: +``conftest.py`` deliberately imports every ``Auto3D`` submodule before the first +test runs (see ``_import_every_auto3d_module_before_any_test``), so by the time +any assertion here executes, ``sys.modules`` already holds torch, rdkit, and all +of Auto3D. An in-process version of these tests would pass unconditionally. +""" +from __future__ import annotations + +import importlib +import json +import subprocess +import sys + +import pytest + +# Generous cap over the stdlib-only floor (~140 modules: interpreter startup +# plus importlib.metadata's own imports). Set well above the floor and well +# below the pre-fix 1175 so it survives a stdlib growing an internal import or +# a site-packages .pth adding one, while still going red the moment +# ``Auto3D/__init__.py`` reaches for torch, rdkit, or a domain submodule. +MAX_MODULES_AFTER_BARE_IMPORT = 250 + +# Names that leaked out of the package root as attributes without ever being +# part of the public API: the ``import warnings`` and ``from +# importlib.metadata import PackageNotFoundError, version`` used to compute +# ``__version__``, and the optional-dependency probes' bindings. +LEAK_CANDIDATES = ( + "warnings", + "version", + "PackageNotFoundError", + "ANI2xt", + "oechem", + "oeomega", + "oequacpac", + "torchani", +) + +# Runs in a fresh interpreter; reports what a bare ``import Auto3D`` cost and +# what it left reachable. Emits one JSON line on stdout. +_PROBE_SOURCE = """ +import json, sys, time + +t0 = time.perf_counter() +import Auto3D +elapsed = time.perf_counter() - t0 + +leak_candidates = %(leaks)r +print(json.dumps({ + "elapsed": elapsed, + "n_modules": len(sys.modules), + "auto3d_modules": sorted( + m for m in sys.modules if m == "Auto3D" or m.startswith("Auto3D.") + ), + "torch": any(m == "torch" or m.startswith("torch.") for m in sys.modules), + "rdkit": any(m == "rdkit" or m.startswith("rdkit.") for m in sys.modules), + "version": Auto3D.__version__, + "dir": sorted(dir(Auto3D)), + "all": list(Auto3D.__all__), + "reachable_leaks": sorted(n for n in leak_candidates if hasattr(Auto3D, n)), +})) +""" % {"leaks": LEAK_CANDIDATES} + + +@pytest.fixture(scope="module") +def bare_import(): + """Measure a cold ``import Auto3D`` in a fresh interpreter, once.""" + proc = subprocess.run( + [sys.executable, "-c", _PROBE_SOURCE], + capture_output=True, + text=True, + ) + assert proc.returncode == 0, f"probe failed:\n{proc.stdout}\n{proc.stderr}" + return json.loads(proc.stdout.strip().splitlines()[-1]) + + +# --------------------------------------------------------------------------- # +# Import cost +# --------------------------------------------------------------------------- # + +def test_bare_import_does_not_load_torch_or_rdkit(bare_import): + """The two heavyweight third-party dependencies stay unimported. + + They are what makes the difference between 0.02 s and 1.35 s. Anything that + needs them imports them itself, at the point of need. + """ + assert not bare_import["torch"], "import Auto3D pulled in torch" + assert not bare_import["rdkit"], "import Auto3D pulled in rdkit" + + +def test_bare_import_loads_no_auto3d_submodule(bare_import): + """Only the package root itself is imported. + + This is the sharp form of the cost assertion: it names the defect (the + package root executing a domain submodule's module body) rather than a + number. ``_LAZY_API`` exists precisely so that no submodule loads until a + public name is used. + """ + assert bare_import["auto3d_modules"] == ["Auto3D"], ( + "import Auto3D eagerly imported submodules: " + f"{[m for m in bare_import['auto3d_modules'] if m != 'Auto3D']}" + ) + + +def test_bare_import_module_count_stays_under_cap(bare_import): + """``len(sys.modules)`` after a bare import stays near the stdlib floor.""" + assert bare_import["n_modules"] < MAX_MODULES_AFTER_BARE_IMPORT, ( + f"import Auto3D loaded {bare_import['n_modules']} modules " + f"(cap {MAX_MODULES_AFTER_BARE_IMPORT}, took " + f"{bare_import['elapsed']:.3f}s)" + ) + + +def test_bare_import_still_reports_a_version(bare_import): + """Cutting the cost must not cost ``__version__``. + + ``__version__`` is computed by a private helper so that ``version`` and + ``PackageNotFoundError`` do not land in the module namespace; this checks + the helper still runs, in a fresh interpreter, outside pytest. + """ + assert isinstance(bare_import["version"], str) + assert bare_import["version"] + + +# --------------------------------------------------------------------------- # +# Public surface: __dir__ (PEP 562) +# --------------------------------------------------------------------------- # + +def test_dir_reports_the_public_api(): + """Every ``__all__`` name is visible to ``dir()`` and to tab-completion.""" + import Auto3D + + listed = set(dir(Auto3D)) + assert "main" in listed + missing = sorted(set(Auto3D.__all__) - listed) + assert not missing, f"__all__ names absent from dir(Auto3D): {missing}" + + +def test_dir_does_not_leak_import_machinery(bare_import): + """The package root exposes no name it merely needed in order to load. + + Checked in both directions and in both processes: not in ``dir()`` here, + and not reachable via ``hasattr`` in a fresh interpreter. ``dir()`` alone is + not enough -- a name can be absent from ``dir()`` and still resolve -- and + ``hasattr`` alone is not enough in this process, because ``conftest``'s + eager submodule import legitimately adds submodule attributes. + """ + import Auto3D + + listed = set(dir(Auto3D)) + leaked_here = sorted(n for n in LEAK_CANDIDATES if n in listed) + assert not leaked_here, f"dir(Auto3D) leaks import machinery: {leaked_here}" + assert not bare_import["reachable_leaks"], ( + "package root exposes non-public attributes: " + f"{bare_import['reachable_leaks']}" + ) + + +# --------------------------------------------------------------------------- # +# Public surface: _LAZY_API +# --------------------------------------------------------------------------- # + +def test_lazy_api_is_a_bijection_with_all(): + """``_LAZY_API`` and ``__all__`` describe the same surface. + + Without this, a name can be added to ``__all__`` and be unreachable, or + added to ``_LAZY_API`` and be undocumented. + """ + import Auto3D + + assert set(Auto3D._LAZY_API) == set(Auto3D.__all__) - {"__version__"} + + +def test_every_lazy_api_name_resolves_to_its_target(): + """Each lazy name resolves to the exact object at its declared location. + + Stronger than "is not None": it pins the target, so moving a function + without updating ``_LAZY_API`` fails here instead of silently exporting + something else of the same name. + """ + import Auto3D + + for name, (module_name, attr) in Auto3D._LAZY_API.items(): + target = getattr(importlib.import_module(module_name), attr) + assert getattr(Auto3D, name) is target, name + + +def test_getattr_does_not_cache_resolved_attributes(): + """Repeated access must re-read the source module, not return a snapshot. + + Caching into ``globals()`` looks like a free optimization and is not: it + turns ``__getattr__`` into an import-time binding, so a test that touches + ``Auto3D.main`` and *then* patches ``Auto3D.auto3D.main`` would patch + nothing and pass for the wrong reason -- the failure mode + ``tests/test_lazy_torchani_import.py`` documents, which surfaced 182 tests + downstream of its cause. After the first access ``import_module`` is a + ``sys.modules`` dict hit, so there is nothing to buy. + + Verified behaviorally (a post-access change to the source module is seen) + and structurally (the name never lands in the package namespace). + """ + import Auto3D + + first = Auto3D.Auto3DOptions + assert first is not None + assert "Auto3DOptions" not in vars(Auto3D), ( + "__getattr__ cached into the module namespace" + ) + + sentinel = object() + module = importlib.import_module("Auto3D.config") + original = module.Auto3DOptions + try: + module.Auto3DOptions = sentinel + assert Auto3D.Auto3DOptions is sentinel, ( + "second access returned a cached value instead of re-reading " + "Auto3D.config" + ) + finally: + module.Auto3DOptions = original + + assert Auto3D.Auto3DOptions is original + + +def test_unknown_attribute_raises_attribute_error(): + """``__getattr__`` must not turn a typo into an ImportError or a hang.""" + import Auto3D + + with pytest.raises(AttributeError, match="no attribute 'nope'"): + getattr(Auto3D, "nope") # noqa: B009 (the lookup is the assertion) From daa7952bd4f31a1ba1c8f2af7d4aec7b16ca1076 Mon Sep 17 00:00:00 2001 From: isayev Date: Tue, 4 Aug 2026 00:57:16 -0400 Subject: [PATCH 3/4] docs: record the import-cost change and the four removed namespace attributes Auto3D.ANI2xt, .warnings, .version and .PackageNotFoundError were never public API, but removing them is still visible to anyone who reached for them, so they belong in Breaking Changes rather than only in the perf note. --- CHANGELOG.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5eb8c64..c648faa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 mass of the isotope it names; the change applies only where no isotope was specified. +- **`import Auto3D` no longer imports torch or RDKit, and four attributes are + gone from the package namespace.** `Auto3D.ANI2xt`, `Auto3D.warnings`, + `Auto3D.version` and `Auto3D.PackageNotFoundError` were never public API — + they were leaked into the namespace by three eager optional-dependency probes + and by module-level imports. All four are removed. + + The probes existed to detect whether an optional engine was installed, but + probing for ANI2xt reached `batch_opt`, which reached the `utils` barrel, + which reached `validation`, which imported torch and `models.loading`. So + importing the package paid for the whole package, plus torch and RDKit, + before the caller had asked for anything. Every real probe already exists at + its use site, so nothing was lost by deleting them. + + | | before | after | + |---|---|---| + | `import Auto3D` | 1.35 s | 0.031 s | + | `len(sys.modules)` | 1175 | 154 | + | torch / RDKit loaded | yes | no | + | `Auto3D.*` submodules loaded | 20 | 0 | + + **What stops working:** referencing any of those four names through the + `Auto3D` package object. **What to do instead:** `import warnings` yourself; + read the version from `Auto3D.__version__`; import optional engines from the + module that owns them. Documented public names are unaffected and still + resolve lazily on first access. + - **One exit-code scheme, used by every command.** `cli/errors.py` has mapped exception types to differentiated exit codes since 3.x -- 0 success, 1 generic, 2 configuration/input, 3 dependency, 4 GPU, 5 model, plus 6 for a From 5dcc965475fbc959402db671402b0b94ac8f3291 Mon Sep 17 00:00:00 2001 From: isayev Date: Tue, 4 Aug 2026 01:04:39 -0400 Subject: [PATCH 4/4] test: make the model, batch_opt and pipeline tests able to fail The same hardening as the previous commit, for the tests covering the code the model-contract consolidation is about to move. The assertions that were doing no work: - test_create_unknown_model_raises_error caught bare Exception, so it could not tell an unresolvable registry name from any other failure. It now matches the message. - Two pytest.raises(ModelLoadError) had no match=, so either would have passed on an unrelated ModelLoadError. Both now match on the argument order they exist to check. - test_ensemble_opt_returns_convergence_info ignored the convergence info it is named for; it now asserts the mask and the oscillation counts. - test_fire_independent_molecule_tracking asserted that per-molecule state existed, not that it was independent. Rewritten as a two-phase protocol: bootstrap with every molecule progressing, then make exactly one oscillate, and assert the exact a and dt values that follow. - test_optimizer_handles_empty_file accepted any log output; it now asserts the empty-file message and that the missing-file message is absent, since those two guards were indistinguishable. New coverage where a code path had none: - The arity check's len(required) > 3 half was never exercised. Only the too-few-arguments half was. - The alias vocabulary is now parametrized over the full synonym set in both correct and transposed order, 14 cases, calling validate_custom_nnp directly so nothing needs to pickle. - forces = -grad is written separately in ANI2xtAdapter and ANI2xAdapter and was tested in neither. Both now assert -2*coords against a toy quadratic model, hermetically, by bypassing each adapter's real __init__. - The "produced no conformers after clash relief" warning had no test. One drives the real embedding path with relieve_clash stubbed by atom count, and asserts the warning fires once, names the right molecule, and that a sibling species survives without being warned about. - test_one_bad_molecule_does_not_remove_the_others parsed IDs with a stale split("_")[0] that made a Tier 1 test vacuous. test_calc_spe_uses_model_factory and test_model_name2model_calculator_uses_factory were slow-marked despite monkeypatching every model construction. Both move to the fast tier; the module-level pytestmark becomes per-test decorators so the 15 real-NNP tests in those two files stay slow. Verified: exactly those two are collected by -m "not slow". Padding invariance gains an ANI2x case at 1e-3, slow-marked. It is not run here: torchani is absent locally and no NNP may be loaded on this box, so CI is what verifies it. Verified: 1302 passed, 9 skipped, 2 xfailed. --- tests/test_SPE.py | 15 ++- tests/test_batchopt.py | 21 +++- tests/test_custom_nnp_contract.py | 88 ++++++++++++++- tests/test_fire_optimizer.py | 76 +++++++++++-- tests/test_model_adapter.py | 88 +++++++++++++++ tests/test_model_factory.py | 2 +- tests/test_optimization_engine_validation.py | 7 +- tests/test_padding_invariance.py | 11 +- tests/test_pipeline_e2e.py | 112 ++++++++++++++++++- tests/test_thermo.py | 20 +++- tests/test_workflow.py | 8 +- 11 files changed, 412 insertions(+), 36 deletions(-) diff --git a/tests/test_SPE.py b/tests/test_SPE.py index d806258..ddee4b8 100644 --- a/tests/test_SPE.py +++ b/tests/test_SPE.py @@ -7,9 +7,13 @@ # from tests import skip_ani2xt_test skip_ani2xt_test = False -# Mark all tests in this module as slow (single-point energy calculations) -pytestmark = pytest.mark.slow - +# Every real-model test below is marked @pytest.mark.slow individually +# (single-point energy calculations, each loading a real NNP). NOT a +# module-level `pytestmark`: test_calc_spe_uses_model_factory below mocks +# every model-construction call (create_model/EnForce_ANI/pad_from_mols) and +# loads no NNP, so it must run in the fast tier -- a module-level mark would +# have swept it in with everything else regardless of what it actually does. +# # Every calc_spe call below passes use_gpu=False on purpose. calc_spe's # `use_gpu` default is True, and Auto3D 4.0 made "GPU requested but no CUDA # device visible" FATAL rather than a silent CPU fallback @@ -138,6 +142,7 @@ def forward(self, return out['energy'].reshape(-1) +@pytest.mark.slow @pytest.mark.skipif(skip_ani2xt_test, reason="ANI2xt model is not installed.") def test_calc_spe_ani2xt(): #load B97-3c results file @@ -154,6 +159,7 @@ def test_calc_spe_ani2xt(): assert(diff <= 0.01) +@pytest.mark.slow def test_calc_spe_ani2x(): #load wB97X/6-31G* output file path = os.path.join(folder, "tests/files/wb97x_dz.sdf") @@ -170,6 +176,7 @@ def test_calc_spe_ani2x(): print(idx, spe_out, diff) assert(diff <= 0.011) +@pytest.mark.slow def test_calc_spe_aimnet(): path = os.path.join(folder, 'tests/files/cyclooctane.sdf') e_ref = -314.689736079491 @@ -179,6 +186,7 @@ def test_calc_spe_aimnet(): e_out = float(mol.GetProp('E_hartree')) assert(abs(e_out - e_ref) <= 0.01) +@pytest.mark.slow @pytest.mark.skipif(not test_userNNP1, reason="TorchANI is not installed.") def test_calc_spe_userNNP1(): #load wB97X/6-31G* output file @@ -203,6 +211,7 @@ def test_calc_spe_userNNP1(): assert(diff <= 0.011) +@pytest.mark.slow def test_calc_spe_userNNP2(): path = os.path.join(folder, 'tests/files/cyclooctane.sdf') e_ref = -314.689736079491 diff --git a/tests/test_batchopt.py b/tests/test_batchopt.py index 99730bc..bbabd78 100644 --- a/tests/test_batchopt.py +++ b/tests/test_batchopt.py @@ -104,8 +104,13 @@ def mock_forward(coords, species, charges, atom_mask=None): energy, forces = model.forward_batched(coords, species, charges) - # Should have called forward multiple times due to batching - assert mock_adapter.forward.call_count >= 1 + # batch_size = max(1, batchsize_atoms // N) = max(1, 10 // 5) = 2 + # molecules per sub-batch; 4 molecules split into chunks of 2 -> the + # adapter must be called exactly twice, not "at least once" (which a + # single unbatched call would also satisfy, defeating the point of a + # forward_batched-specific test). Mirrors test_model_wrapper.py's + # stronger call_count==2 sibling for the same batchsize_atoms/N ratio. + assert mock_adapter.forward.call_count == 2 assert energy.shape == (4,) assert forces.shape == (4, 5, 3) @@ -134,13 +139,19 @@ def test_ensemble_opt_returns_convergence_info(self): result = ensemble_opt(model, coord, numbers, charges, param, torch.device("cpu")) - # Verify new fields are present + # Verify new fields are present, with their actual VALUES: zero force + # on step 1 means fmax (0.0) is at once below opttol (0.01) for both + # structures, so both must be reported converged and neither must + # have been counted as oscillating -- checking only key + # presence/type/length (as before) would pass even if the values + # were transposed, all-False, or a stray increment leaked into + # oscillating_count. assert 'converged_mask' in result, "converged_mask missing from ensemble_opt return" assert 'oscillating_count' in result, "oscillating_count missing from ensemble_opt return" assert isinstance(result['converged_mask'], list) assert isinstance(result['oscillating_count'], list) - assert len(result['converged_mask']) == 2 - assert len(result['oscillating_count']) == 2 + assert result['converged_mask'] == [True, True] + assert result['oscillating_count'] == [0, 0] @pytest.mark.slow diff --git a/tests/test_custom_nnp_contract.py b/tests/test_custom_nnp_contract.py index c04dfe2..e6c722a 100644 --- a/tests/test_custom_nnp_contract.py +++ b/tests/test_custom_nnp_contract.py @@ -92,6 +92,18 @@ def forward(self, species, coords): return (coords ** 2).sum(dim=(1, 2)) +class TooManyArgsNNP(torch.nn.Module): + """Extra required positional argument -- Auto3D never passes a fourth.""" + + def __init__(self): + super().__init__() + self.coord_pad = 0.0 + self.species_pad = -1 + + def forward(self, species, coords, charges, cutoff): + return (coords ** 2).sum(dim=(1, 2)) * cutoff + + class ExoticNamesNNP(torch.nn.Module): """Right arity, names outside the known vocabulary -- must be accepted.""" @@ -178,7 +190,7 @@ def test_transposed_forward_is_rejected_through_the_adapter(tmp_path): from Auto3D.models.adapter import CustomModelAdapter path = _save(TransposedNNP(), tmp_path, "transposed_adapter.pt") - with pytest.raises(ModelLoadError): + with pytest.raises(ModelLoadError, match="species, coords, charges"): CustomModelAdapter(path, CPU) @@ -209,7 +221,7 @@ def test_transposed_forward_is_rejected_by_input_validation(tmp_path): verbose=False, job_name="", ) - with pytest.raises(ModelLoadError): + with pytest.raises(ModelLoadError, match="species, coords, charges"): check_input(args) @@ -220,6 +232,67 @@ def test_alias_names_in_the_wrong_order_are_rejected(tmp_path): load_custom_nnp(path, CPU) +def _model_with_param_names(species_name, coords_name, charges_name): + """A plain object (no torch.save/pickle needed) whose forward's parameter + NAMES are exactly the given ones, so validate_custom_nnp's order check + (models/contract.py::_classify / _check_forward_signature) sees them. + + Not an nn.Module: this only needs to be introspectable by + inspect.signature, which is all validate_custom_nnp actually uses, and + dynamically-named parameters cannot be produced by a module-level + class (needed elsewhere in this file for torch.save's pickling). + """ + namespace: dict = {} + exec( # noqa: S102 - test-only, fixed trusted template, no user input + f"def forward(self, {species_name}, {coords_name}, {charges_name}):\n" + f" return ({coords_name} ** 2).sum(dim=(1, 2))\n", + namespace, + ) + return type( + "DynamicNamedNNP", + (), + {"coord_pad": 0.0, "species_pad": -1, "forward": namespace["forward"]}, + )() + + +# Full synonym vocabulary from models/contract.py's _SPECIES_NAMES/ +# _COORDS_NAMES/_CHARGES_NAMES, covered at least once each, plus one +# mixed-case ("Numbers"/"Positions"/"Charge") combination to confirm the +# order check case-folds via _classify's ``name.lower()``. +ALIAS_VOCABULARY = [ + ("species", "coords", "charges"), + ("numbers", "positions", "charge"), + ("atomic_numbers", "coordinates", "charge"), + ("atomicnumbers", "coord", "q"), + ("z", "pos", "charges"), + ("elements", "xyz", "charge"), + ("Numbers", "Positions", "Charge"), +] + + +@pytest.mark.parametrize("species_name,coords_name,charges_name", ALIAS_VOCABULARY) +def test_alias_vocabulary_in_correct_order_is_accepted( + species_name, coords_name, charges_name +): + """Every recognized synonym, in the right order, must not be rejected -- + a false rejection here would break a working model that merely spelled + the contract differently.""" + model = _model_with_param_names(species_name, coords_name, charges_name) + validate_custom_nnp(model, "") # must not raise + + +@pytest.mark.parametrize("species_name,coords_name,charges_name", ALIAS_VOCABULARY) +def test_alias_vocabulary_transposed_is_rejected( + species_name, coords_name, charges_name +): + """The same synonyms, transposed (coords first), must still be caught -- + synonyms are not an escape hatch from the order check, across the full + vocabulary, not just the one numbers/positions/charge pair.""" + model = _model_with_param_names(coords_name, species_name, charges_name) + with pytest.raises(ModelLoadError, match="species, coords, charges"): + validate_custom_nnp(model, "") + + def test_missing_both_padding_attributes_are_rejected_at_load(tmp_path): """coord_pad/species_pad are part of the contract; absent, the layers used to disagree on the default, so a silent fallback is worse than a refusal.""" @@ -247,6 +320,17 @@ def test_wrong_arity_is_rejected_at_load(tmp_path): load_custom_nnp(path, CPU) +def test_wrong_arity_too_many_required_args_is_rejected_at_load(tmp_path): + """The ``> 3`` branch: a fourth REQUIRED positional argument is just as + uncallable as the two-argument case above, but exercises the other half + of ``len(positional) < 3 or len(required) > 3`` in + ``models/contract.py::_check_forward_signature``.""" + path = _save(TooManyArgsNNP(), tmp_path, "fourargs.pt") + with pytest.raises(ModelLoadError, match="three positional arguments") as excinfo: + load_custom_nnp(path, CPU) + assert "cutoff" in str(excinfo.value) + + # --- acceptance (a false rejection is a regression) ------------------------- def test_contract_conforming_model_loads_and_runs(tmp_path): diff --git a/tests/test_fire_optimizer.py b/tests/test_fire_optimizer.py index 73a9c4c..91306fe 100644 --- a/tests/test_fire_optimizer.py +++ b/tests/test_fire_optimizer.py @@ -2,6 +2,7 @@ """Unit tests for the FIRE optimizer module.""" from __future__ import annotations +import pytest import torch from Auto3D.batch_opt.fire_optimizer import FIRE @@ -304,22 +305,73 @@ def test_fire_handles_mixed_convergence(self): assert coord.shape == (4, 5, 3) def test_fire_independent_molecule_tracking(self): - """FIRE should track each molecule's state independently.""" - coord = torch.zeros(3, 5, 3) + """FIRE tracks each molecule's dt/a state independently, driven only + by that molecule's OWN progressing flag -- not by whatever the other + molecules in the batch are doing (fire_optimizer.py's per-molecule + torch.where selects on ``progressing``/``speedup``, but ``a3``, + ``dt``, ``self.a`` etc are shared *tensors* the batch is stepped + through together, so a batch-index mix-up would leak one molecule's + state into another's). + + Protocol (all pure tensor arithmetic; deterministic, no randomness): + Phase 1 -- all 3 molecules push in the same fixed direction, so they + progress together and jointly build up ``Nsteps`` past ``Nmin`` + (bootstrapping requires ALL molecules progressing at least once; see + the ``all_progressing`` branch). Phase 2 -- molecule 0 starts + flipping its own force sign every step. Once misaligned with its own + velocity, a molecule can never re-progress under an alternating + force (each non-progressing step resets v to align with THAT step's + force, and the next step's opposite force is then anti-aligned) -- + so molecule 0 is guaranteed to be "not progressing" for the rest of + phase 2, while molecules 1/2 keep progressing and trigger the + speed-up branch (``past_nmin`` already true from phase 1, + ``all_progressing`` now false because molecule 0 dissents). + """ + n_atoms = 2 + astart = 0.1 + dt_max = 0.1 + coord = torch.zeros(3, n_atoms, 3) optimizer = FIRE(coord) + steady_force = torch.ones(n_atoms, 3) * 0.1 - # Give different molecules different force histories - for i in range(3): - forces = torch.zeros(3, 5, 3) - forces[i] = torch.randn(5, 3) * 0.1 + # Phase 1: all three molecules progress together, bootstrapping + # Nsteps past Nmin (=5) for everyone. + for _ in range(8): + forces = torch.stack([steady_force, steady_force, steady_force]) + coord = optimizer(coord, forces) + assert (optimizer.Nsteps > 5).all(), "phase 1 setup failed to reach Nmin" + + # Phase 2: molecule 0 alternates sign every step (can never progress + # again); molecules 1 and 2 keep pushing steadily. + for step in range(10): + forces = torch.stack( + [ + steady_force if step % 2 == 0 else -steady_force, + steady_force, + steady_force, + ] + ) coord = optimizer(coord, forces) - # Each molecule should have different state - # (at minimum, different velocities) - v_norms = [optimizer.v[i].norm().item() for i in range(3)] - - # They shouldn't all be identical - assert not (v_norms[0] == v_norms[1] == v_norms[2]) + # Molecule 0 never progressed in phase 2, so its mixing parameter + # `a` must be exactly reset to astart every single step -- the + # oscillating molecule's own branch, untouched by its batch-mates. + assert optimizer.a[0].item() == pytest.approx(astart) + + # Molecules 1/2 share an identical force history and so must reach + # an identical (and, since they triggered the speed-up branch, + # strictly smaller-than-astart) mixing parameter -- proving the + # speed-up state is tracked per molecule, not smeared across the + # batch by molecule 0's resets. + assert optimizer.a[1].item() == pytest.approx(optimizer.a[2].item()) + assert optimizer.a[1].item() < astart + + # dt tells the same story from the other side: molecule 0's dt was + # repeatedly shrunk (fdec) by its own non-progress, while 1/2's dt + # saturated at dt_max via their own speed-up. + assert optimizer.dt[1].item() == pytest.approx(dt_max) + assert optimizer.dt[2].item() == pytest.approx(dt_max) + assert optimizer.dt[0].item() < optimizer.dt[1].item() class TestFIRETorchScript: diff --git a/tests/test_model_adapter.py b/tests/test_model_adapter.py index e21fda2..0777b85 100644 --- a/tests/test_model_adapter.py +++ b/tests/test_model_adapter.py @@ -132,6 +132,47 @@ def test_ani2xt_adapter_creates_model(self): assert adapter.species_pad == -1 assert adapter.coord_pad == 0.0 + def test_ani2xt_adapter_force_sign_with_toy_model(self): + """``forces = -grad`` is duplicated once per adapter in adapter.py: + ``ANI2xtAdapter.forward`` has its own copy, distinct from (and + untested by) ``CustomModelAdapter``'s copy that + ``test_custom_model_adapter_runs`` already checks (audit M32). A sign + bug introduced independently in THIS copy would not be caught by that + test, and ``test_ani2xt_adapter_creates_model`` above never calls + ``.forward`` at all. + + ``BaseModelAdapter.__init__`` is called directly on a bypassed + instance (skipping ``ANI2xtAdapter.__init__``, which imports the real + bundled ANI2xt weights) and handed a toy quadratic model instead -- + same technique ``TestBaseModelAdapter`` already uses for a mock + model, applied here so the REAL ``ANI2xtAdapter.forward`` runs. + Hermetic: no NNP loaded, no torchani import. + """ + from Auto3D.models.adapter import ANI2xtAdapter, BaseModelAdapter + + class _ToyANI2xtModel(torch.nn.Module): + def forward(self, species, coords): + return (coords ** 2).sum(dim=(1, 2)) + + device = torch.device("cpu") + adapter = ANI2xtAdapter.__new__(ANI2xtAdapter) + BaseModelAdapter.__init__( + adapter, _ToyANI2xtModel(), device, coord_pad=0.0, species_pad=-1 + ) + + coords = torch.randn(2, 4, 3) + species = torch.tensor([[0, 1, 2, 3], [0, 1, 2, -1]]) + charges = torch.zeros(2) + energy, forces = adapter.forward(coords, species, charges) + + # _ToyANI2xtModel does not mask padding: E = sum(coords^2) over every + # slot => dE/dx = 2*coords => F = -dE/dx = -2*coords, exactly like + # test_custom_model_adapter_runs's reference calculation. + torch.testing.assert_close( + energy, (coords ** 2).sum(dim=(1, 2)), rtol=1e-5, atol=1e-6 + ) + torch.testing.assert_close(forces, -2.0 * coords, rtol=1e-5, atol=1e-6) + class TestANI2xAdapter: """Tests for the ANI2x adapter.""" @@ -150,6 +191,53 @@ def test_ani2x_adapter_creates_model(self): assert adapter.species_pad == -1 assert adapter.coord_pad == 0.0 + def test_ani2x_adapter_force_sign_with_toy_model(self): + """``forces = -grad`` is duplicated again in ``ANI2xAdapter.forward`` + -- a third, separate copy from ``ANI2xtAdapter``'s and + ``CustomModelAdapter``'s (audit M32), also untested by + ``test_custom_model_adapter_runs``. + + The toy model mimics torchani's ``SpeciesEnergies`` return shape (an + object with a ``.energies`` attribute) rather than + ``ANI2xtAdapter``'s plain-tensor return, since ``ANI2xAdapter.forward`` + calls ``self.model((species, coords)).energies`` and multiplies by + ``HARTREE_TO_EV`` -- the toy divides by the same constant first so the + expected force in eV is still the clean ``-2*coords``. Coordinates are + float32 from the start (matching what ``ANI2xAdapter.forward`` casts + to internally) so the adapter's own ``coords.float()`` cast is a + no-op here and cannot be blamed for any looseness in the comparison + (the brainstorm's dtype-cast risk flag for this specific test). + Hermetic: no NNP loaded, no torchani import. + """ + from collections import namedtuple + + from Auto3D.constants import HARTREE_TO_EV + from Auto3D.models.adapter import ANI2xAdapter, BaseModelAdapter + + _SpeciesEnergies = namedtuple("SpeciesEnergies", ["species", "energies"]) + + class _ToyANI2xModel(torch.nn.Module): + def forward(self, species_coords): + species, coords = species_coords + energies = (coords ** 2).sum(dim=(1, 2)) / HARTREE_TO_EV + return _SpeciesEnergies(species, energies) + + device = torch.device("cpu") + adapter = ANI2xAdapter.__new__(ANI2xAdapter) + BaseModelAdapter.__init__( + adapter, _ToyANI2xModel(), device, coord_pad=0.0, species_pad=-1 + ) + + coords = torch.randn(2, 4, 3, dtype=torch.float32) + species = torch.tensor([[1, 6, 7, 8], [1, 6, 7, -1]]) + charges = torch.zeros(2) + energy, forces = adapter.forward(coords, species, charges) + + torch.testing.assert_close( + energy, (coords ** 2).sum(dim=(1, 2)), rtol=1e-5, atol=1e-6 + ) + torch.testing.assert_close(forces, -2.0 * coords, rtol=1e-5, atol=1e-6) + class TestCustomModelAdapter: """Tests for the CustomModelAdapter.""" diff --git a/tests/test_model_factory.py b/tests/test_model_factory.py index 80644fc..c99554a 100644 --- a/tests/test_model_factory.py +++ b/tests/test_model_factory.py @@ -42,7 +42,7 @@ def __init__(self, *a, **k): raise RuntimeError("unresolvable registry name") monkeypatch.setattr(model_factory, "AIMNet2Adapter", _Boom) - with pytest.raises(Exception): + with pytest.raises(RuntimeError, match="unresolvable registry name"): ModelFactory.create( "totally-not-a-real-model-xyz", device=torch.device("cpu"), diff --git a/tests/test_optimization_engine_validation.py b/tests/test_optimization_engine_validation.py index 8609ad3..760d6b9 100644 --- a/tests/test_optimization_engine_validation.py +++ b/tests/test_optimization_engine_validation.py @@ -153,6 +153,9 @@ def test_n_steps_error_is_not_assertion_error(self): 'nn': None, } - # Should NOT raise AssertionError - with pytest.raises(ValueError): + # Should NOT raise AssertionError, and must be raised for the coord + # shape defect this fixture actually has -- a bare `ValueError` would + # also pass for e.g. an unrelated numbers/charges ValueError, so pin + # the message to the coord/3D guard this fixture is built to hit. + with pytest.raises(ValueError, match="coord.*3D"): n_steps(invalid_state, n=10, opttol=0.01, patience=100) diff --git a/tests/test_padding_invariance.py b/tests/test_padding_invariance.py index 6d99501..c464078 100644 --- a/tests/test_padding_invariance.py +++ b/tests/test_padding_invariance.py @@ -31,14 +31,19 @@ class TestPaddingInvariance: # already asserts with atol=1e-2 eV, and ANI2xt's float32 output caps # usable precision at ~float32 ULP (~4e-3 eV) at typical total-energy # magnitudes per src/Auto3D/batch_opt/ANI2xt_no_rep.py:148-155. 1e-6 would - # demand sub-ULP reproducibility and flake on a correct model. + # demand sub-ULP reproducibility and flake on a correct model. ANI2x + # (torchani, periodic-table indexing) shares ANI2xt's float32 output and + # the same -1 species_pad convention, so it gets the same 1e-3 budget + # rather than AIMNet2's looser 1e-2 -- there is no reason to expect it + # tighter than its ANI-family sibling, and no measurement here to justify + # tighter than that either. @pytest.mark.parametrize( "engine, atol", - [("AIMNET", 1e-2), ("ANI2xt", 1e-3)], + [("AIMNET", 1e-2), ("ANI2xt", 1e-3), ("ANI2x", 1e-3)], ) def test_energy_unchanged_when_padded(self, engine, atol, device): """Batching a small molecule alongside a large one must not shift its energy.""" - if engine == "ANI2xt": + if engine in ("ANI2xt", "ANI2x"): pytest.importorskip("torchani") from Auto3D.model_factory import create_model diff --git a/tests/test_pipeline_e2e.py b/tests/test_pipeline_e2e.py index 6f74c97..a8b7cdf 100644 --- a/tests/test_pipeline_e2e.py +++ b/tests/test_pipeline_e2e.py @@ -7,7 +7,10 @@ the reconciliation function, exists and is exported and tested with zero production callers (C7). -Slow tier: uses the real aimnet2 registry model on CPU. +Slow tier: uses the real aimnet2 registry model on CPU. NOT a module-level +`pytestmark`, though: `TestClashReliefWarning` below is hermetic (no NNP, no +network) and must run in the fast tier, so every real-pipeline test in this +module is marked `@pytest.mark.slow` individually instead. """ from __future__ import annotations @@ -15,8 +18,7 @@ from rdkit import Chem from Auto3D.config import Auto3DOptions - -pytestmark = pytest.mark.slow +from tests.helpers_pipeline_output import base_molecule_id def _input_ids(smi_path: str) -> set[str]: @@ -32,6 +34,7 @@ def _input_ids(smi_path: str) -> set[str]: class TestInputOutputAccounting: """No input may vanish without being reported.""" + @pytest.mark.slow def test_every_input_is_present_or_reported(self, job_dir): """Each input ID must appear in the output or in a reported failure list. @@ -84,6 +87,7 @@ def test_every_input_is_present_or_reported(self, job_dir): f"{sorted(missing)}" ) + @pytest.mark.slow def test_one_bad_molecule_does_not_remove_the_others(self, job_dir): """A sodium counterion must fail, and must fail alone. @@ -124,8 +128,17 @@ def test_one_bad_molecule_does_not_remove_the_others(self, job_dir): args = Auto3DOptions(path=str(smi), k=1, use_gpu=False, max_confs=2) out = main(args) + # `.split("_")[0]` used to truncate "sodium_acetate" to "sodium" at + # this line, so `"sodium_acetate" not in produced` below was true + # UNCONDITIONALLY -- true whether sodium_acetate correctly failed + # (the intended case) or a regression let it silently succeed and + # reach the output SDF under a name split() would still mangle to + # "sodium". `base_molecule_id` is the pipeline's own id-recovery + # helper (matches what ConformerRanker/decode_ids leave in `_Name` + # by this point), so this now actually depends on whether + # sodium_acetate is or is not in the output. produced = { - m.GetProp("_Name").split("_")[0] + base_molecule_id(m.GetProp("_Name")) for m in Chem.SDMolSupplier(out, removeHs=False) if m is not None } @@ -162,6 +175,7 @@ def test_one_bad_molecule_does_not_remove_the_others(self, job_dir): class TestExitStatus: """Losing molecules must not exit 0.""" + @pytest.mark.slow def test_cli_exits_nonzero_when_molecules_are_missing(self, job_dir): """auto3d run must signal partial failure through its exit code.""" from typer.testing import CliRunner @@ -196,6 +210,7 @@ def test_cli_exits_nonzero_when_molecules_are_missing(self, job_dir): class TestEnergyAndRankingSanity: """Assert on the numbers, not merely that the program ran.""" + @pytest.mark.slow def test_energies_are_negative_and_ordered(self, isolated_input): """E_tot must be negative and ascending within a conformer group.""" from Auto3D.auto3D import main @@ -219,6 +234,7 @@ def test_energies_are_negative_and_ordered(self, isolated_input): f"{base}: conformers are not energy-ordered: {energies}" ) + @pytest.mark.slow def test_top_k_returns_distinct_conformers(self, isolated_input): """k=3 must yield at most 3 per molecule, and the first is the minimum.""" from Auto3D.auto3D import main @@ -238,3 +254,91 @@ def test_top_k_returns_distinct_conformers(self, isolated_input): for base, energies in groups.items(): assert len(energies) <= 3, f"{base}: k=3 but got {len(energies)}" assert energies[0] == min(energies), f"{base}: first is not the minimum" + + +class TestClashReliefWarning: + """RDKitIsomer's serial embedding path (isomer_engine.py's + ``_run_serial_embedding``) must warn, once, when EVERY embedded conformer + of a species is rejected by clash relief -- the species then silently + vanishes from the output with no other trace. Before this test, + ``grep -rn "produced no conformers after clash relief" tests/`` returned + nothing: this is distinct from (and untested by) the parallel-embedding + path's own version of the same warning in ``isomers/parallel_embed.py``, + which ``test_workflow.py`` already covers. + + Hermetic: no NNP, no network. ``relieve_clash`` itself is monkeypatched, + so this exercises only isomer_engine.py's warn-and-continue behavior + around it, not the clash-relief force field logic. + """ + + def test_species_with_no_surviving_conformer_is_warned_and_a_sibling_is_not( + self, tmp_path, caplog, monkeypatch + ): + import logging + + import Auto3D.isomer_engine as isomer_engine_mod + from Auto3D.isomer_engine import RDKitIsomer + + smi = tmp_path / "in.smi" + # "bad_mol" (methane) will have every conformer rejected below; + # "good_mol" (ethanol) is the companion the guard must NOT warn about + # (the over-fire check the spec's verification standard asks for). + smi.write_text("C\tbad_mol\nCCO\tgood_mol\n") + + job_dir = tmp_path / "job" + job_dir.mkdir() + + engine = RDKitIsomer( + smi=str(smi), + smiles_enumerated=str(tmp_path / "enumerated.smi"), + smiles_enumerated_reduced=str(tmp_path / "enumerated_reduced.smi"), + smiles_hashed=str(tmp_path / "hashed.smi"), + enumerated_sdf=str(tmp_path / "enumerated.sdf"), + job_name=str(job_dir), + max_confs=1, + threshold=0.3, + np=1, + flipper=False, + ) + + # relieve_clash(mol, conf_id) never sees the species name -- only the + # embedded RDKit Mol -- so identify "the methane-derived molecule" + # the only way the stub can: by its (distinctive) atom count after + # AddHs. Bypassing the real force-field logic entirely keeps this + # test hermetic and removes any dependence on ETKDG's random seed + # actually producing a clashing geometry. + bad_atom_count = Chem.AddHs(Chem.MolFromSmiles("C")).GetNumAtoms() + + def fake_relieve_clash(mol, conf_id): + return mol.GetNumAtoms() != bad_atom_count + + monkeypatch.setattr( + isomer_engine_mod, "relieve_clash", fake_relieve_clash + ) + + with caplog.at_level(logging.WARNING): + out = engine.run() + + warnings = [ + r.message for r in caplog.records if r.levelno == logging.WARNING + ] + clash_warnings = [ + m for m in warnings if "produced no conformers after clash relief" in m + ] + assert len(clash_warnings) == 1, ( + f"expected exactly one clash-relief warning, got {clash_warnings}" + ) + assert "bad_mol" in clash_warnings[0] + # The over-fire check: a sibling molecule in the same batch that DID + # survive clash relief must not be named in any such warning. + assert "good_mol" not in clash_warnings[0] + assert not any("good_mol" in m for m in clash_warnings) + + # bad_mol is absent from the output entirely; good_mol made it through. + produced = { + m.GetProp("_Name") + for m in Chem.SDMolSupplier(out, removeHs=False) + if m is not None + } + assert not any(name.startswith("bad_mol") for name in produced), produced + assert any(name.startswith("good_mol") for name in produced), produced diff --git a/tests/test_thermo.py b/tests/test_thermo.py index e2b7429..71ea6b0 100644 --- a/tests/test_thermo.py +++ b/tests/test_thermo.py @@ -14,9 +14,13 @@ write_perturbed_sdf, ) -# Mark all tests in this module as slow (thermodynamic calculations) -pytestmark = pytest.mark.slow - +# Every real-model test below is marked @pytest.mark.slow individually +# (thermodynamic calculations, each loading a real NNP). NOT a module-level +# `pytestmark`: test_model_name2model_calculator_uses_factory below patches +# both create_model and EnForce_ANI and loads no NNP, so it must run in the +# fast tier -- a module-level mark would have swept it in regardless (its +# test_SPE.py twin, test_calc_spe_uses_model_factory, had the same defect). +# # Every opt_geometry/calc_thermo call below passes use_gpu=False on purpose. # Both default to use_gpu=True, and Auto3D 4.0 made "GPU requested but no CUDA # device visible" FATAL rather than a silent CPU fallback @@ -252,6 +256,7 @@ def assert_thermo_record(mol, *, reference_G=None, reference_H=None): ) +@pytest.mark.slow def test_calc_thermo_aimnet(): """AIMNET thermochemistry for cyclooctane against a wB97m-D4/Def2-TZVPP run. @@ -273,6 +278,7 @@ def test_calc_thermo_aimnet(): except OSError: pass +@pytest.mark.slow def test_vib_hessian_includes_external_dispersion(): """Regression guard: the AIMNET vibrational Hessian must run the full energy pipeline (external D3 dispersion + Coulomb), not the bare aimnet nn.Module. @@ -371,6 +377,7 @@ def _perturbed_DA(tmp_path) -> tuple[str, list]: return write_perturbed_sdf(source, tmp_path / "DA.sdf", DA_EXPANSION) +@pytest.mark.slow def test_opt_geometry1(tmp_path): """ANI2x relaxes a displaced geometry and annotates it correctly.""" path, inputs = _perturbed_DA(tmp_path) @@ -378,6 +385,7 @@ def test_opt_geometry1(tmp_path): assert_opt_geometry_output(out, input_mols=inputs, moved_at_least=DA_MIN_RELAXATION, label="ANI2x") +@pytest.mark.slow def test_opt_geometry2(tmp_path): """ANI2xt relaxes a displaced geometry and annotates it correctly.""" path, inputs = _perturbed_DA(tmp_path) @@ -385,6 +393,7 @@ def test_opt_geometry2(tmp_path): assert_opt_geometry_output(out, input_mols=inputs, moved_at_least=DA_MIN_RELAXATION, label="ANI2xt") +@pytest.mark.slow def test_opt_geometry3(tmp_path): """AIMNet2 relaxes a displaced geometry and annotates it correctly.""" path, inputs = _perturbed_DA(tmp_path) @@ -393,6 +402,7 @@ def test_opt_geometry3(tmp_path): moved_at_least=DA_MIN_RELAXATION, label="AIMNET") +@pytest.mark.slow def test_opt_geometry_with_patience_and_batchsize(): """Test opt_geometry with explicit patience and batchsize_atoms parameters.""" path = os.path.join(folder, "tests/files/DA.sdf") @@ -411,6 +421,7 @@ def test_opt_geometry_with_patience_and_batchsize(): except OSError: pass +@pytest.mark.slow @pytest.mark.skipif(not test_userNNP1, reason="TorchANI is not installed.") def test_opt_geometry4(tmp_path): """A scripted custom NNP relaxes a displaced geometry through opt_geometry.""" @@ -426,6 +437,7 @@ def test_opt_geometry4(tmp_path): moved_at_least=DA_MIN_RELAXATION, label="scripted userNNP1") +@pytest.mark.slow def test_opt_geometry5(tmp_path): """An eager AIMNet2-backed custom NNP relaxes a displaced geometry.""" path, inputs = _perturbed_DA(tmp_path) @@ -441,6 +453,7 @@ def test_opt_geometry5(tmp_path): label="eager userNNP2") +@pytest.mark.slow @pytest.mark.skipif(not test_userNNP1, reason="TorchANI is not installed.") def test_calc_thermo_userNNP1(): #load wB97m-D4/Def2-TZVPP output file @@ -478,6 +491,7 @@ def test_calc_thermo_userNNP1(): pass +@pytest.mark.slow def test_calc_thermo_userNNP2(): #load wB97m-D4/Def2-TZVPP output file path = os.path.join(folder, "tests/files/cyclooctane.sdf") diff --git a/tests/test_workflow.py b/tests/test_workflow.py index 9f4d533..5fd996c 100644 --- a/tests/test_workflow.py +++ b/tests/test_workflow.py @@ -297,7 +297,13 @@ def test_optimizer_handles_empty_file(self, tmp_path, caplog, monkeypatch): with caplog.at_level(logging.WARNING): optimizer.run() - assert "empty" in caplog.text + # Pin the exact guard that fired: the empty-file message must be + # distinguishable from the missing-file message above ("does not + # exist"), which is also a file literally named "empty.sdf" would + # trivially satisfy a bare "empty" in caplog.text check without ever + # proving the *empty-file* branch (not the missing-file branch) ran. + assert f"Input file {empty_sdf} is empty." in caplog.text + assert "does not exist" not in caplog.text def test_workers_importable_from_workflow_workers():