Skip to content

Commit 8ce15eb

Browse files
Felipedinoclaude
andcommitted
Fix what the review found, and teach the audit about inheritance
Five real findings, one of them hiding the others. **A search needs a tuner, not only a target.** Both evaluation strategies guarded on `self.optimizer and self.run_optimizable_parameters`; the units kept only the second half. `Run.optimizer_name` is a plain string and the wizard leaves it empty when no search is asked for, while the model may still declare a parameter optimizable -- a combination that has always meant "fit it once with the values given". It had become a lookup of the empty string in the registry, surfacing as "Metric is not compatible with the Task. ''", a message with nothing to do with what happened. Reproduced, fixed with a shared `_will_search`, and pinned by a test. **The nested unit was not being audited at all.** `_unit_class` matched only classes whose direct base is literally `BaseUnit`, so a unit that extends another unit fell out of every contract check -- and would have failed them, because its PROVIDES are written by the parent's body rather than its own. It is the same blindness a shared helper causes, arriving by inheritance instead: the audit reads one class's source. It now follows the lineage for declarations, context calls and config reads. 32 audited units became 33. **The inner splitter was resolved unconditionally**, so a run still carrying a nested configuration it no longer uses failed on a splitter it would never have touched. **The fold branch hardcoded `splits=["TEST"]`** where the holdout branch derives it from SCORED_SPLITS. Latent today -- no strategy excludes TEST -- but it is exactly the coupling this work exists to remove. And a docstring describing `{split: [scores]}` for something shaped `{split: {metric: [scores]}}`. Two findings were left alone, deliberately. `best_parameters` is published without being in PROVIDES, which is the already-declared limitation that there is no way to express an optional output; the new units repeat it rather than inventing an exception to it. And per-fold progress reporting is gone, which is a real regression: restoring it needs a callback in a unit's contract, and a runtime parameter the engine cannot supply makes the unit unusable as a node -- the static validator rejects it. Both are recorded rather than patched over. Whole suite: 3692 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f30f3bc commit 8ce15eb

7 files changed

Lines changed: 130 additions & 24 deletions

File tree

DashAI/back/job/model_job.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,7 @@ def run(
294294
# uses. Whether there is anything to score is the
295295
# caller's to know: a session that reserved nothing
296296
# leaves that partition empty rather than absent.
297-
if len(x[-1]["test"]) > 0:
297+
if "TEST" in scored_splits and len(x[-1]["test"]) > 0:
298298
EvaluateModelUnit(run_id=run_id, splits=["TEST"])(ctx)
299299
except Exception as e:
300300
log.exception(e)

DashAI/back/units/fit_model_over_folds_unit.py

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ def execute(self, ctx: ExecutionContext) -> None:
9696

9797
plot_paths = []
9898
try:
99-
if optimizable_parameters:
99+
if self._will_search(optimizable_parameters):
100100
# Every read of the context happens in this file rather than in
101101
# the shared helper: the contract audit parses it, so a require
102102
# moved out makes a declared key look unread.
@@ -221,9 +221,10 @@ def _score_every_fold(self, model, x_folds, y_folds) -> dict:
221221
Returns
222222
-------
223223
dict
224-
``{split name: [one score per fold]}``, in fold order, for whoever
225-
aggregates them. A split with no metrics configured is absent
226-
rather than present and empty: the two are different statements.
224+
``{split name: {metric name: [one score per fold]}}``, in fold
225+
order, for whoever aggregates them. A split with no metrics
226+
configured is absent rather than present and empty: the two are
227+
different statements.
227228
"""
228229
scored_splits = self.config.get("scored_splits", TRIAL_SPLITS)
229230
fold_metrics: dict = {}

DashAI/back/units/fit_model_over_nested_folds_unit.py

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -102,11 +102,13 @@ def execute(self, ctx: ExecutionContext) -> None:
102102
y_folds = ctx.require("y_folds")
103103
optimizable_parameters = ctx.require("optimizable_parameters")
104104

105-
# Resolved outside the wrapper below so a splitter that cannot be built
106-
# is reported as that, rather than as a training failure.
107-
inner_splitter = self._resolve_inner_splitter()
108-
109-
if optimizable_parameters:
105+
if self._will_search(optimizable_parameters):
106+
# Resolved here and not at the top: a run with nothing to search
107+
# never carves an outer fold, so a session left carrying a nested
108+
# configuration it no longer uses must not fail on it. And resolved
109+
# outside the wrapper below, so a splitter that cannot be built is
110+
# reported as that rather than as a training failure.
111+
inner_splitter = self._resolve_inner_splitter()
110112
try:
111113
outer_fold_metrics = self._measure_every_outer_fold(
112114
model,

DashAI/back/units/fit_model_unit.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ def execute(self, ctx: ExecutionContext) -> None:
8484

8585
plot_paths = []
8686
try:
87-
if not optimizable_parameters:
87+
if not self._will_search(optimizable_parameters):
8888
self._fit_kept_model(model, x, y)
8989
else:
9090
# Every read of the context happens here rather than in the

DashAI/back/units/fit_scope.py

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -216,17 +216,31 @@ def _resolve_search(self):
216216
self._optimizer = optimizer
217217
return optimizer, goal_metric
218218

219+
def _will_search(self, optimizable_parameters) -> bool:
220+
"""Whether there is a search to run: something to tune, and a tuner.
221+
222+
Both halves are needed. A run can name no optimizer at all -- the
223+
column is a plain string and the wizard leaves it empty when the user
224+
does not ask for a search -- while the model still declares a parameter
225+
as optimizable, and that combination has always meant "fit it once with
226+
the values given". Checking only the parameters turns it into a lookup
227+
of the empty string in the registry, which fails with a message about
228+
the metric being incompatible with the task.
229+
"""
230+
return bool(optimizable_parameters) and bool(
231+
self.config["optimizer"]["component"]
232+
)
233+
219234
def _validate_search(self, optimizable_parameters) -> None:
220235
"""Refuse an impossible search before anything observable happens.
221236
222237
Handed the value rather than the context: the caller reads it with
223238
``ctx.require`` and not ``ctx.get``, because an absent key means the
224239
model has not been built yet -- a call-order mistake, not "there is
225-
nothing to optimize". Only an empty value, the key present and the
226-
model declaring none, skips the checks below, so no registry lookup is
227-
needed either.
240+
nothing to optimize". A run with nothing to search skips the checks
241+
below, so no registry lookup is needed either.
228242
"""
229-
if not optimizable_parameters:
243+
if not self._will_search(optimizable_parameters):
230244
return
231245

232246
self._resolve_search()

tests/back/units/test_fit_model_unit.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,3 +379,26 @@ def test_the_search_is_handed_the_units_own_objective(tmp_path):
379379
finally:
380380
del di["component_registry"]
381381
del di["config"]
382+
383+
384+
def test_a_run_with_no_optimizer_fits_once_even_if_a_parameter_is_optimizable():
385+
"""Both halves are needed to call something a search: a tuner and a target.
386+
387+
``Run.optimizer_name`` is a plain string and the wizard leaves it empty when
388+
the user does not ask for a search, while the model may still declare a
389+
parameter as optimizable. That combination has always meant "fit it once
390+
with the values given". Checking only the parameters turns it into a lookup
391+
of the empty string in the registry, which surfaces as a complaint about
392+
the metric being incompatible with the task -- a message with nothing to do
393+
with what happened.
394+
"""
395+
model = _RecordingModel()
396+
ctx = _fit_context(model, _HOLDOUT, _HOLDOUT)
397+
ctx.put("optimizable_parameters", [("obj", "C", (0, 1), "number")])
398+
399+
unit = _unit(optimizer_name="", goal_metric="")
400+
unit.validate(ctx)
401+
unit(ctx)
402+
403+
assert model.fits == [{"train": "x-train", "validation": "x-val"}]
404+
assert not ctx.has("best_parameters")

tests/back/units/test_unit_contracts.py

Lines changed: 75 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,26 +34,85 @@ def _string_literals(node):
3434
}
3535

3636

37+
def _all_classes():
38+
"""Every class defined under ``units/``, by name.
39+
40+
Built first so a unit that inherits from another unit can be resolved: the
41+
audit reads source rather than importing, so a base class is just a name
42+
until something maps it back to a definition.
43+
"""
44+
classes = {}
45+
for path in _unit_modules():
46+
tree = ast.parse(path.read_text(encoding="utf-8"))
47+
for node in ast.walk(tree):
48+
if isinstance(node, ast.ClassDef):
49+
classes[node.name] = node
50+
return classes
51+
52+
53+
_CLASSES = _all_classes()
54+
55+
56+
def _is_unit(node):
57+
"""A class deriving from ``BaseUnit``, directly or through another unit."""
58+
for base in node.bases:
59+
if not isinstance(base, ast.Name):
60+
continue
61+
if base.id == "BaseUnit":
62+
return True
63+
parent = _CLASSES.get(base.id)
64+
if parent is not None and parent is not node and _is_unit(parent):
65+
return True
66+
return False
67+
68+
3769
def _unit_class(tree):
3870
for node in ast.walk(tree):
39-
if isinstance(node, ast.ClassDef) and any(
40-
isinstance(base, ast.Name) and base.id == "BaseUnit" for base in node.bases
41-
):
71+
if isinstance(node, ast.ClassDef) and _is_unit(node):
4272
return node
4373
return None
4474

4575

76+
def _lineage(cls):
77+
"""The class and the units it inherits from, nearest first.
78+
79+
Everything below reads the whole lineage rather than one class body. A unit
80+
that extends another one inherits both its declarations and the code that
81+
honours them, so auditing only its own body would report that it promises
82+
keys it never writes -- which is the same blindness a shared helper causes,
83+
arriving by a different road.
84+
"""
85+
chain = [cls]
86+
for base in cls.bases:
87+
if not isinstance(base, ast.Name):
88+
continue
89+
parent = _CLASSES.get(base.id)
90+
if parent is not None and parent is not cls and _is_unit(parent):
91+
chain.extend(_lineage(parent))
92+
return chain
93+
94+
4695
def _declared(cls, name):
47-
for node in cls.body:
48-
if isinstance(node, ast.Assign) and any(
49-
isinstance(t, ast.Name) and t.id == name for t in node.targets
50-
):
51-
return _string_literals(node.value)
96+
# Nearest declaration wins: a subclass that redeclares PROVIDES replaces
97+
# what it inherited rather than adding to it, the way Python resolves it.
98+
for ancestor in _lineage(cls):
99+
for node in ancestor.body:
100+
if isinstance(node, ast.Assign) and any(
101+
isinstance(t, ast.Name) and t.id == name for t in node.targets
102+
):
103+
return _string_literals(node.value)
52104
return set()
53105

54106

55107
def _context_calls(cls, methods):
56-
"""Every ``ctx.<method>("key")`` literal inside the class."""
108+
"""Every ``ctx.<method>("key")`` literal in the class and what it extends."""
109+
keys = set()
110+
for ancestor in _lineage(cls):
111+
keys |= _context_calls_in(ancestor, methods)
112+
return keys
113+
114+
115+
def _context_calls_in(cls, methods):
57116
keys = set()
58117
for node in ast.walk(cls):
59118
if (
@@ -302,6 +361,13 @@ def _schema_fields(tree):
302361

303362

304363
def _config_reads(cls):
364+
reads = set()
365+
for ancestor in _lineage(cls):
366+
reads |= _config_reads_in(ancestor)
367+
return reads
368+
369+
370+
def _config_reads_in(cls):
305371
"""Every ``self.config["key"]`` and ``self.config.get("key")`` in the class."""
306372
keys = set()
307373
for node in ast.walk(cls):

0 commit comments

Comments
 (0)