Skip to content

Commit 33ef84b

Browse files
Jammy2211claude
authored andcommitted
fix(jax): structural defense against cached_property pytree/dict leaks
PR #1300 fixed a specific leak where AbstractPriorModel.parameterization (a `@functools.cached_property` added in commit 4564ae9) leaked its cached string into every ModelInstance via Collection._instance_for_arguments. That broke 38 JAX jit(fit_from) calls and the autofit_workspace overview_1 smoke (clusters C1+C4). The minimal fix renamed the cache key to `_parameterization_cache` so the existing `_`-prefix filter at each `__dict__` iterator skipped it. The structural problem remained: every walker uses an opt-out filter (blacklist + underscore prefix), so any future cached_property declared on a model class silently reproduces the same class of bug. This PR closes the class: - New classmethod `AbstractModel._cached_property_names(cls)` delegates to `autoconf.tools.decorators.cached_property_names` (PyAutoConf #111), returning a frozenset of every functools.cached_property and autoconf CachedProperty descriptor name in the MRO. - Extend the filter at every `__dict__` iteration site to union the pre-existing exclusion with this frozenset: autofit/mapper/model.py (__getstate__, ModelInstance.dict) autofit/mapper/model_object.py (ModelObject._dict — feeds Collection.items) autofit/mapper/prior_model/abstract.py (AbstractModel.items) autofit/mapper/prior_model/collection.py (Collection._instance_for_arguments) autofit/mapper/prior_model/prior_model.py (Model._instance_for_arguments) Identifier-hash stability verified: the unique_identifier walker at `autofit/mapper/identifier.py` does NOT call any of these 6 sites — it walks `__dict__` independently with its own `_`-prefix filter. Three representative model shapes (simple Collection, nested Collection, Model with tuple arg) all produce byte-identical identifier hashes pre- and post-defense: simple: f7f19073a8fb19b3d11231fb6eef7e3b ✓ nested: 04e1328c84a1e4c3a81a9d3544dd19f5 ✓ with_tuple: 36084d2c3fec27e0b7aa504add0bd898 ✓ Tests: - `test_cached_property_names_classmethod_walks_mro`: confirms the classmethod surfaces MRO-declared descriptors and memoises per-class. - `test_cached_property_excluded_from_all_dict_walks`: ships a synthetic GuardedCollection with a cached_property returning a string; asserts the value never appears in instance.__dict__, instance.dict, model.items(), tree_flatten() leaves, __getstate__, or pickle round-trip. - 1415/1415 PyAutoFit tests pass (1413 prior + 2 new). Depends on: PyAutoLabs/PyAutoNerves#111. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 2a8f5c7 commit 33ef84b

6 files changed

Lines changed: 131 additions & 4 deletions

File tree

autofit/mapper/model.py

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,9 +83,28 @@ def __init__(self, label=None, id_=None):
8383
self._frozen_cache = dict()
8484
super().__init__(label=label, id_=id_)
8585

86+
@classmethod
87+
def _cached_property_names(cls) -> frozenset:
88+
"""
89+
Return the names of every ``cached_property``-style descriptor
90+
declared anywhere in ``cls``'s MRO.
91+
92+
Used by the ``__dict__``-iteration sites in this module and in
93+
``autofit/mapper/prior_model/`` to exclude cached descriptor values
94+
from instance construction, ``ModelInstance.dict``, pickling, and
95+
downstream JAX pytree flattening. See PyAutoFit#1300 for the
96+
diagnosed leak this defends against.
97+
"""
98+
from autoconf.tools.decorators import cached_property_names
99+
100+
return cached_property_names(cls)
101+
86102
def __getstate__(self):
103+
excluded = type(self)._cached_property_names()
87104
return {
88-
key: value for key, value in self.__dict__.items() if key != "_frozen_cache"
105+
key: value
106+
for key, value in self.__dict__.items()
107+
if key != "_frozen_cache" and key not in excluded
89108
}
90109

91110
def __setstate__(self, state):
@@ -446,11 +465,13 @@ def __hash__(self):
446465

447466
@property
448467
def dict(self):
468+
excluded = type(self)._cached_property_names()
449469
return {
450470
key: value
451471
for key, value in self.__dict__.items()
452472
if key not in ("id", "component_number", "item_number")
453473
and not (isinstance(key, str) and key.startswith("_"))
474+
and key not in excluded
454475
}
455476

456477
def tree_flatten(self) -> Tuple[List, Tuple]:

autofit/mapper/model_object.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,9 +330,20 @@ def dict(self) -> dict:
330330

331331
@property
332332
def _dict(self):
333+
# Pick up any cached_property descriptors declared on the class so
334+
# their cached values don't propagate via `Collection.items()` (which
335+
# delegates here) or any other downstream consumer. The lookup is
336+
# gated on hasattr because ModelObject is the base for the whole
337+
# mapper module: a few non-AbstractModel descendants do not carry the
338+
# ``_cached_property_names`` classmethod.
339+
try:
340+
excluded = type(self)._cached_property_names()
341+
except AttributeError:
342+
excluded = frozenset()
333343
return {
334344
key: value
335345
for key, value in self.__dict__.items()
336346
if key not in ("component_number", "item_number", "id", "cls", "label")
337347
and not key.startswith("_")
348+
and key not in excluded
338349
}

autofit/mapper/prior_model/abstract.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1275,12 +1275,17 @@ def from_instance(
12751275
def items(self):
12761276
"""Return (name, value) pairs for all public, non-internal attributes.
12771277
1278-
Excludes private attributes (prefixed with ``_``), ``cls``, and ``id``.
1278+
Excludes private attributes (prefixed with ``_``), ``cls``, ``id``,
1279+
and any ``cached_property``-style descriptors declared on the class
1280+
(see ``AbstractModel._cached_property_names``).
12791281
"""
1282+
excluded = type(self)._cached_property_names()
12801283
return [
12811284
(key, value)
12821285
for key, value in self.__dict__.items()
1283-
if not key.startswith("_") and key not in ("cls", "id")
1286+
if not key.startswith("_")
1287+
and key not in ("cls", "id")
1288+
and key not in excluded
12841289
]
12851290

12861291
@property

autofit/mapper/prior_model/collection.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,8 +286,9 @@ def _instance_for_arguments(
286286
A list of instances constructed from the list of prior models.
287287
"""
288288
result = ModelInstance()
289+
excluded = type(self)._cached_property_names()
289290
for key, value in self.__dict__.items():
290-
if key.startswith("_"):
291+
if key.startswith("_") or key in excluded:
291292
continue
292293
if isinstance(value, AbstractPriorModel):
293294
value = value.instance_for_arguments(

autofit/mapper/prior_model/prior_model.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -494,12 +494,14 @@ def _instance_for_arguments(
494494
else:
495495
result = self.cls(**constructor_arguments)
496496

497+
excluded = type(self)._cached_property_names()
497498
for key, value in self.__dict__.items():
498499
if (
499500
not hasattr(result, key)
500501
and not isinstance(value, Prior)
501502
and not key == "cls"
502503
and not key.startswith("_")
504+
and key not in excluded
503505
):
504506
if isinstance(value, Model):
505507
value = value.instance_for_arguments(

test_autofit/mapper/test_parameterization.py

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import functools
12
import itertools
23

34
import pytest
@@ -176,6 +177,92 @@ def test_parameterization_cache_does_not_leak_into_instance():
176177
assert not isinstance(child, str)
177178

178179

180+
def test_cached_property_names_classmethod_walks_mro():
181+
"""The ``_cached_property_names`` classmethod on AbstractModel exposes the
182+
autoconf ``cached_property_names`` MRO walker. It must pick up
183+
descriptors declared on any ancestor and memoise the result on the class."""
184+
185+
import functools
186+
187+
import autofit as af
188+
189+
# Build a synthetic subclass with a cached_property to verify the walker
190+
# finds it. We use af.Collection because both AbstractPriorModel and
191+
# ModelInstance inherit from AbstractModel.
192+
class SyntheticCollection(af.Collection):
193+
@functools.cached_property
194+
def synthetic_value(self):
195+
return "a synthetic cached string"
196+
197+
names = SyntheticCollection._cached_property_names()
198+
assert "synthetic_value" in names
199+
200+
# Result is memoised on the synthetic class.
201+
assert "__cached_property_names_cache__" in SyntheticCollection.__dict__
202+
203+
# Plain af.Collection (no synthetic_value) has its own cache.
204+
base_names = af.Collection._cached_property_names()
205+
assert "synthetic_value" not in base_names
206+
207+
208+
class _GuardedCollection(af.Collection):
209+
"""Module-level subclass used by
210+
``test_cached_property_excluded_from_all_dict_walks`` — must live at
211+
module scope so ``pickle.dumps`` can locate the class on round-trip."""
212+
213+
@functools.cached_property
214+
def derived(self):
215+
return "leaky-string"
216+
217+
218+
def test_cached_property_excluded_from_all_dict_walks():
219+
"""Regression: a future ``@functools.cached_property`` declared anywhere
220+
in the model class hierarchy must not surface through any of:
221+
``Collection._instance_for_arguments`` (via ``instance.__dict__``),
222+
``ModelInstance.dict``, ``ModelInstance.tree_flatten()``,
223+
``AbstractModel.items()``, ``ModelObject._dict``, or pickling via
224+
``__getstate__``.
225+
226+
Covers the class of bug PyAutoFit#1300 fixed for ``parameterization``;
227+
this test will fail if a maintainer reintroduces an un-prefixed
228+
cached_property on the model hierarchy without the
229+
``_cached_property_names`` defense applied at every site."""
230+
231+
import pickle
232+
233+
model = _GuardedCollection(gaussian=af.Model(af.ex.Gaussian))
234+
235+
# Trigger the cache. After this, model.__dict__["derived"] = "leaky-string".
236+
_ = model.derived
237+
assert model.__dict__.get("derived") == "leaky-string"
238+
239+
instance = model.instance_from_prior_medians()
240+
241+
# Site 1+4: Collection._instance_for_arguments + ModelInstance.dict
242+
assert "derived" not in instance.__dict__
243+
assert "derived" not in instance.dict
244+
245+
# Site 4 also feeds tree_flatten — no string leaves.
246+
leaves = instance.dict.values()
247+
for leaf in leaves:
248+
assert not isinstance(leaf, str)
249+
250+
# Site 3: AbstractModel.items() on the model itself.
251+
assert all(key != "derived" for key, _ in model.items())
252+
253+
# Site 5: __getstate__ drops the cached value from pickles.
254+
state = model.__getstate__()
255+
assert "derived" not in state
256+
257+
# Round-trip via pickle: the unpickled model re-computes the cached value,
258+
# rather than carrying the pickled string on the wire.
259+
blob = pickle.dumps(model)
260+
revived = pickle.loads(blob)
261+
assert "derived" not in revived.__dict__
262+
# Touching it recomputes.
263+
assert revived.derived == "leaky-string"
264+
265+
179266
def test_integer_attributes():
180267
model = af.Model(af.ex.Gaussian)
181268

0 commit comments

Comments
 (0)