diff --git a/CHANGELOG.md b/CHANGELOG.md index 5eb8c64c..c648faae 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 diff --git a/src/Auto3D/__init__.py b/src/Auto3D/__init__.py index 54501df8..613017a6 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_SPE.py b/tests/test_SPE.py index d806258c..ddee4b83 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 99730bc5..bbabd78c 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_cli_config_schema.py b/tests/test_cli_config_schema.py index 113ccd8e..047c5dfb 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 7350eeb0..610519ef 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 e8d1dc27..27376a07 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_custom_nnp_contract.py b/tests/test_custom_nnp_contract.py index c04dfe26..e6c722a7 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_filtering.py b/tests/test_filtering.py index 354fe1dc..c3626466 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_fire_optimizer.py b/tests/test_fire_optimizer.py index 73a9c4c3..91306fea 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_import_boundaries.py b/tests/test_import_boundaries.py new file mode 100644 index 00000000..d70e35ce --- /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) diff --git a/tests/test_isomer_engine.py b/tests/test_isomer_engine.py index da92505c..393df1c1 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 dc8e7905..8a23fda1 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_model_adapter.py b/tests/test_model_adapter.py index e21fda2a..0777b850 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 80644fc3..c99554a2 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 8609ad30..760d6b94 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 6d99501c..c4640788 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_parallel_embed.py b/tests/test_parallel_embed.py index 7643f407..40b5e1a6 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_pipeline_e2e.py b/tests/test_pipeline_e2e.py index 6f74c97d..a8b7cdf4 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 e2b7429d..71ea6b0e 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_utils_chemistry.py b/tests/test_utils_chemistry.py index e5b3c60c..f0b8345a 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 732380f3..75997c9e 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: diff --git a/tests/test_workflow.py b/tests/test_workflow.py index 9f4d533f..5fd996c2 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():