Repository navigation
refactor(state): normalize current_best once at load and drop its reader-side dict guards - #1883
Conversation
SharedState.from_dict now normalizes current_best the way it already normalizes the other dict-valued fields. Together with the dataclass default and the two writers in writeback, which only assign dicts, every SharedState now carries a dict in current_best whatever state.json holds. Co-authored-by: Cursor <cursoragent@cursor.com>
Every SharedState carries a dict in current_best, so the 43 readers
that re-checked it drop the check. A reader of a real SharedState uses
the field directly; a reader that takes a duck-typed state keeps
getattr(state, "current_best", None) or {} for an absent state and
loses only the type check. Checks on the fields inside current_best
(extra_envs, measurement) stay.
The code-metrics baseline is tightened for the 29 entries that went
down.
Co-authored-by: Cursor <cursoragent@cursor.com>
Code metrics gate: PASSEDJudged: the 17 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
Module length warning: over 800 lines, within the 1200-line failure limit (not failing): 3
Already over at the base (backlog, not this change's): 16
80 findings in files this change does not touchOutside this change (informational): 80
...and 55 more; the job log lists every row.
Thresholds and sources: |
|
/review |
|
Automated review started for head 37545f4. Progress: https://github.com/AMD-AGI/Hyperloom/actions/runs/38551918107 |
…sses Two _MoreState doubles subclass the _BareSharedState dataclass without the decorator, so their field(default_factory=dict) declarations left dataclasses.Field objects on the class instead of dicts. The explore materialize path reads current_best through stack_base_params, which relies on it being a dict as it is on every SharedState. Decorate both like their sibling doubles. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Automated review: no verdict applied -- the PR head moved from 37545f4 to 08b215d during the review. Comment PR #1883 -- refactor(state): normalize current_best once at load and drop its reader-side dict guardsWhat it does: A Blocking issues: none Checked: head-tree reads of shared_state.py (from_dict :1102, load_or_init :1052, field :472), writeback.py writers :1695/:2023, request_handlers.py, recipe_journal.py, forge_handoff.py, roofline_ceiling.py, _partition_shape.py, dispatch.py, experience_kb.py, close.py, kernel.py, render.py; tree-wide greps for every writer of |
…-env' into lean/shafeng/cc-current-best-normalize Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # scripts/code_metrics_baseline.txt
…-env' into lean/shafeng/cc-current-best-normalize Brings in main's phase-budget (#1873) and gemm-tune MoE routing (#1862) fixes through the base branch. The baseline conflicts are adjacent entries both sides tightened; each takes the lower value, which is the merged tree's measured value except for request_handlers.py's module-lines. That entry keeps the base's 5138: main's file is 5149 lines because the two fixes landed one after the other, each recording its own count, and this branch's 5144 is five below it, so the excess is the base's backlog and the baseline does not grow.
PR #1883 -- refactor(state): normalize current_best once at load and drop its reader-side dict guardsWhat it does: Blocking issues: none Checked: diff of all 16 files; every writer of LGTM |
|
Non-blocking suggestions. None of these blocks merging; each removes a defensive check rather than adding one.
|
…-env' into lean/shafeng/cc-current-best-normalize The baseline conflicted only on adjacent lines each side lowered: every entry keeps the lower value, which is also the measured one.
SharedState.current_best is always a dict, so roofline_recipe reads it without an isinstance guard, like every other SharedState reader.
This branch has not been deployed
2 participants
Stacked on #1877. The base is
lean/shafeng/cc-writeback-config-env, so the diff here is only the commits on top of it. It will be retargeted tomainonce #1877 merges.SharedState.current_bestis declareddict[str, Any], its dataclass default is{}, and both writers inwriteback.pyassign dict literals. The one way a non-dict got in was astate.jsonwhosecurrent_bestwasnullor another non-object, becausefrom_dictpassed it through. So 44 readers across 14 modules re-checked the type, each with its own fallback.from_dictnow normalizes the field the way it already normalizespending_kernel_integrationsand the other dict fields, and the readers trust that. Main's #1890 added one more,roofline_recipe: it takes a realSharedState, so it readsstate.current_bestdirectly.SharedState(self.current_best,state.current_best,self.shared_state.current_best) uses the field directly. That includes six reads that appliedor {}and thenisinstanceto the same value.statekeepsgetattr(state, "current_best", None) or {}for an absent state and loses only the type check.cb_nowalias in_promote_explorestays, because it holds the pre-promotion dict that later lines read.SharedState.current_best:gap_for_state: no state means no gap, so it returnsNone.RemoteRecipefrom_state: raises its validation error by contract.report.pyreads: they read the report payload, notSharedState.geak_config: itscurrent_bestis a number.first_positive_tput: generic over several sources.current_best(extra_envs,measurement) are untouched.Not included: twelve reads of a real
SharedStatethat spell(x.current_best or {})with no type check. The invariant makes them redundant too. They are left out to keep this diff to the audited guards.Linked issue(s): none
Tests: added/updated? commands run?
Added:
test_a_non_object_current_best_loads_as_empty, parametrized overNone,[], a string and a number.Updated: two
_MoreStatedoubles intest_critic_verdict_map.pysubclassed a dataclass without@dataclass, so theircurrent_bestwas adataclasses.Fieldobject. The removedstack_base_paramsguard had hidden that. They are now dataclasses, like their sibling doubles.Every changed function was compared between base and head after mechanically stripping, on both sides:
current_bestisinstancechecks;or {}oncurrent_best;After merging the updated base at
794d477d3, the same comparison covers 36 functions. 33 match once thecurrent_bestguards are stripped, includingroofline_recipe. Three differ:from_dict, which is the normalization, and the two narrowed signatures (stack_base_params,_current_best_throughput). This also covers the dedent in_fill_integrate_defaults_from_stateand_build_recipe_attrs_from_state.Checks run locally on Windows:
ruff checkandruff format --check: clean.lint-imports: 7 kept, 0 broken.python scripts/code_metrics.py --base-ref origin/lean/shafeng/cc-writeback-config-env: PASSED.analysis_failures.pyis the file added since; the edit only drops a guard on a parameter typedAny.pytest over the 76 test files that exercise the changed functions, one roofline test deselected because on Windows its
os.kill(pid, 0)probe sends Ctrl+C to the whole console: 3325 passed, 3 skipped, 92 failed. All 92 fail the same way on the base commit794d477d3, each for a Windows-only reason: bash-based fakes, symlink privileges, path separators, encoding, or git path detection.test_promote_shared_state_lock.pyhangs on Windows and is left to CI, as is the full suite.Every touched unit shrinks:
_resolve_forge_server_logdrops under the cyclomatic ceiling (it was 21).28 entries are lowered, for example:
_fill_integrate_defaults_from_state: cognitive 102 → 92._build_recipe_attrs_from_state: 48 → 41._promote_explore: 237 → 231.lift_to_current_best: 148 → 144.If this simplifies or refactors: which mechanism was removed, which contract was preserved, and the tests that show it:
Removed: 44 reader-side type checks on
current_bestand the fallback each one carried. The invariant is now stated once, infrom_dict.Preserved: each reader's output for every
state.jsonthat loads. The existing suites pin it:profile identity:
test_shared_state_units;KB recipe writes:
test_coordinator_kb_writes,test_recipe_sedimentation;integrate defaults:
test_integrate_payload_defaults,test_kernel_request_handlers_units;GEAK handoff:
test_geak_handoff_*;conc sweep:
test_conc_sweep*;the load shape: the new test.
Observable effect:
A
state.jsonwhosecurrent_bestisnull, a list, a string or a number now loads as{}, and the next save writes{}back. The readers this PR touches already treated such a value as "no current best", so a run makes the same decisions. Readers that guarded only againstNone, with(x.current_best or {}).get(...), used to raiseAttributeErroron a truthy non-dict; they now see{}.stack_base_paramsnarrows its annotation fromAnytodict[str, Any] | None. Every caller passes one of those.PR addresses single concern: yes. The concern is the
current_bestdict invariant: the loader enforces it once and the readers rely on it.Root cause is upstream (Magpie/TraceLens/GEAK/IntelliKit/AgentKernelArena), ticket filed: no