Put the fp64 upcast on the adapter contract instead of past it - #166
Open
isayev wants to merge 1 commit into
Open
Put the fp64 upcast on the adapter contract instead of past it#166isayev wants to merge 1 commit into
isayev wants to merge 1 commit into
Conversation
`Auto3D.ASE.thermo._load_hessian_model` wrote `adapter.model.double()` -- reaching through `ModelAdapter` to an attribute only `BaseModelAdapter` happens to define. Every in-tree adapter derives from it, so the call worked; a conforming *structural* adapter (test doubles, and anything a downstream user writes -- production has always accepted these) has no `.model` and died with `AttributeError` inside the thermochemistry path. This is audit finding D4, and it is one of the errors `|| true` was discarding: mypy reported `"ModelAdapter" has no attribute "model"` on every run. `ModelAdapter` gains `to_double()`, the counterpart to `analytic_hessian` returning `None`. An adapter with no native second derivative is differentiated by `torch.autograd.functional.hessian` on the fp64 geometry `vib_hessian` builds; `energy` is deliberately dtype-*preserving* so the caller can ask for fp64, but that request is meaningless while the weights are fp32. `to_double` is how the caller says so. `BaseModelAdapter.to_double` is `self.model.double()`, not `self.double()`, although this class is itself an `nn.Module` and the two coincide for every adapter that registers no other child. They are not guaranteed to -- `Module.double` recurses into every registered submodule -- and keeping the operation byte-for-byte what it was is what makes "no reported frequency moves" a claim rather than a hope. `AIMNet2Adapter.to_double` raises rather than inheriting. Whole-graph fp64 through AIMNet2 is false precision, and the adapter never needs the upcast anyway because its Hessian is analytic. The mechanism would also be wrong: `self.model` is `self._calc.model`, so upcasting it mutates the module underneath a calculator that prepares its own fp32 inputs. Inheriting would make all of that a silent, working-looking call -- the same judgment `analytic_hessian` already documents for `None`. Widening the contract widens the `EnForce_ANI` gate, because `missing_adapter_members` derives from the Protocol rather than restating it. Exactly one test double went red: a hand-rolled `_RecordingAdapter` in test_batchopt that duplicated every `FakeAdapter` member to record one dtype. It now subclasses the helper, which is the shape `tests/helpers_adapter` was built to absorb. Every other double already inherited from `FakeAdapter` or `AdapterModuleMixin`, so both grew the member once. No CHANGELOG breaking entry, and this is the first item since 0.6 without one: `ModelAdapter` is internal, and keeping it internal -- explicitly, so it could still gain members -- is what the registry scope decision preserved. What remains of D4, deliberately: the branch in `_load_hessian_model` is still keyed on the engine name. It decides two things, and only one of them moved. `use_cache` must be chosen *before* construction, so it cannot be a capability query on the adapter, and caching the AIMNet2 branch is what stops `calc_thermo` paying for two full model loads. A pre-construction predicate belongs in the registry's `info` slot; that is a later step, not this one. Verification: - 1761 passed, 1 skipped, 70 deselected (+4 new), under both a fixed and a randomized order. - No behavior-lock test needed editing. `test_load_hessian_model_aimnet_is_fp32` and the ANI-branch fp64 assertions pass unmodified, which is the evidence that the operation underneath is unchanged. - mypy 69 -> 68 errors in 21 files, still "checked 72 source files"; the removed one is the D4 `attr-defined` at the old thermo call site. No new error in any touched file. - Mutation-tested both guards that could have passed vacuously: making `BaseModelAdapter.to_double` a no-op fails the upcast test and the ANI routing test; hoisting `to_double()` out of the ANI branch so AIMNet2 also receives it fails the new AIMNET guard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Auto3D.ASE.thermo._load_hessian_modelwroteadapter.model.double()— reaching throughModelAdapterto an attribute onlyBaseModelAdapterhappens to define. Every in-tree adapter derives from it, so the call worked; a conforming structural adapter (test doubles, and anything a downstream user writes — production has always accepted these) has no.modeland died withAttributeErrorinside the thermochemistry path.This is audit finding D4, and it is one of the errors
|| truewas discarding: mypy reported"ModelAdapter" has no attribute "model"on every run.The change
ModelAdaptergainsto_double(), the counterpart toanalytic_hessianreturningNone. An adapter with no native second derivative is differentiated bytorch.autograd.functional.hessianon the fp64 geometryvib_hessianbuilds.energyis deliberately dtype-preserving so the caller can ask for fp64 — but that request is meaningless while the weights are fp32.to_doubleis how the caller says so.Two implementation decisions worth reviewing:
BaseModelAdapter.to_doubleisself.model.double(), notself.double(), although this class is itself annn.Moduleand the two coincide for every adapter that registers no other child. They are not guaranteed to —Module.doublerecurses into every registered submodule. Keeping the operation byte-for-byte what it was is what makes "no reported frequency moves" a claim rather than a hope.AIMNet2Adapter.to_doubleraises rather than inheriting. Whole-graph fp64 through AIMNet2 is false precision, and the adapter never needs the upcast anyway because its Hessian is analytic. The mechanism would also be wrong:self.modelisself._calc.model, so upcasting it mutates the module underneath a calculator that prepares its own fp32 inputs. Inheriting would make all of that a silent, working-looking call — the same judgmentanalytic_hessianalready documents forNone.Blast radius
Widening the contract widens the
EnForce_ANIgate, becausemissing_adapter_membersderives from the Protocol rather than restating it. Exactly one test double went red: a hand-rolled_RecordingAdapterintest_batchoptthat duplicated everyFakeAdaptermember in order to record one dtype. It now subclasses the helper — the shapetests/helpers_adapterwas built to absorb. Every other double already inherited fromFakeAdapterorAdapterModuleMixin, so both grew the member once.Compatibility
No CHANGELOG breaking entry, and this is the first item since 0.6 without one.
ModelAdapteris internal, and keeping it internal — explicitly, so that it could still gain members — is what the registry scope decision preserved.What remains of D4, deliberately
The branch in
_load_hessian_modelis still keyed on the engine name. It decides two things and only one of them moved:use_cachemust be chosen before construction, so it cannot be a capability query on the adapter, and caching the AIMNet2 branch is what stopscalc_thermopaying for two full model loads. A pre-construction predicate belongs in the registry'sinfoslot; that is a later step, not this one.Verification
test_load_hessian_model_aimnet_is_fp32and the ANI-branch fp64 assertions pass unmodified — that is the evidence the operation underneath is unchanged.attr-definedat the old call site; no new error in any touched file.BaseModelAdapter.to_doublea no-op fails the upcast test and the ANI routing test; hoistingto_double()out of the ANI branch so AIMNet2 also receives it fails the new AIMNET guard.