Repository navigation
refactor: one table owns every value warmed into a specialist - #1887
Conversation
…alist The per-field export rule was spelled three ways: a table for values exported when truthy, a second for numbers exported when above zero, and a branch of its own for the framework name because that one is stripped first. Converting before the test makes them one rule and one table -- an empty string, an empty container and a zero all read as nothing to export -- so the second table and the framework branch go. The `getattr` defaults go with them: every field in the table is declared on SharedState, so a missing attribute is a programming error worth raising rather than a silent skip. Cyclomatic complexity of warm_specialist_params drops from 45 to 41, cognitive from 109 to 102, and its length from 188 lines to 180. Observable effect: a negative `tp`, `conc`, `isl`, `osl`, `max_model_len` or `baseline_tput` is now exported rather than dropped. None of those fields has a negative sentinel -- a negative value there is already a broken session, and it is now visible to the specialist instead of looking like an unset field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code metrics gate: PASSEDJudged: the 3 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): 1
88 findings in files this change does not touchOutside this change (informational): 88
...and 63 more; the job log lists every row.
Thresholds and sources: |
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #1887 -- refactor: one table owns every value warmed into a specialist
What it does: warm_specialist_params spelled one export rule ("hand a SharedState value to a specialist only when the session has one") three ways: a truthy table, a separate above-zero table for numbers, and an inline strip branch for framework. The PR converts each value before the truthiness test, so framework, tp, conc, isl, osl, max_model_len and baseline_tput become ordinary rows of _STATE_PARAMS. The second table, the framework branch and the unreachable getattr/or 0 defaults are deleted. The one observable change is stated in the description: a negative workload number is now exported instead of dropped.
Blocking issues: none
Checked: src/hyperloom/orchestrator/specialists/dispatch.py (_STATE_PARAMS, warm_specialist_params at head), src/hyperloom/orchestrator/state/shared_state.py (field declarations 345-804, from_dict), every in-tree writer of the 16 exported fields (cli/bootstrap.py, loop/writeback.py, knowledge/recipe_kb_t0.py), the 12 callers of warm_specialist_params, test_coordinator_async_methods_coverage_unit.py (_STATE_EXPORTS and the warm_specialist_params tests), scripts/code_metrics_baseline.txt, overlapping #1883 | Ran: import of head _STATE_PARAMS + SharedState from a git archive of the head (default state exports nothing; tp=-1 is exported), git grep / git log -S for None writers and Optional annotations; scoped pytest SKIPPED (pytest not installed locally; CI test shards green at head); ruff C901 SKIPPED locally (ruff not installed; code-metrics gate passed at head with 45 -> 41) | SKIPPED: V4 -- files past the first 100 of #1869/#1675/#1276/#1154/#1032 not compared | Base: 190604f | Head: 8cccef8
LGTM
Follow-up to review feedback on #1881 (finding 2,
export-rule). The per-field export rule inwarm_specialist_paramswas still spelled three ways:_STATE_PARAMSfor values exported when truthy,_POSITIVE_NUMBER_PARAMSfor numbers exported when above zero, and a branch of its own for the framework name because that one is stripped first. The reviewer is right that this is one rule written three times.Converting before the test collapses them: an empty string, an empty container and a zero all read as nothing to export. The second table and the framework branch go, and
frameworkbecomes an ordinary row withstr.stripas its conversion.The
getattr(state, attr, None)/or 0defaults go with them. Every field in the table is declared on SharedState, so a missing attribute is a programming error worth raising rather than a silent skip (AGENTS.md Trust the caller).Review feedback on #1881.
test_warm_specialist_params_fills_defaultsis deleted as superseded, per review finding 3. Its three assertions are carried across rather than dropped:tpis covered by the table test, andgpu_typeandkb_subgraphmove intotest_warm_specialist_params_always_writes_the_unconditional_keystogether withkeep_threshold_pct, which the reviewer asked be taken out of the "omits an empty field" test. Those two keys andkb_subgraphare written whatever the session holds, so a specialist reads them without a presence test; that is a contract, not an implementation detail, and it now has a test that says so instead of two asserts lodged in a test about something else.The one suggestion not taken: driving the table tests from the production
_STATE_PARAMS. A test that reads the table it is checking cannot catch a wrong row, so the test keeps its own literal table.Run locally:
pytestovertest_coordinator_async_methods_coverage_unit.py,test_coordinator_async_batch2_unit.py,test_specialist_integration.py,test_specialist_lessons_section.py,test_custom_specialist.py,test_experience_kb_integration.py,test_specialist_roofline_evidence.py,test_specialist_runtime_findings.py,test_gaps_field.py,test_bringup_round_scenario.py- 376 passed.warm_specialist_paramsAlready over, lowered again, no new unit. Measured with the pinned
ruff 0.16.2,complexipy 8.0.1and the gate own collectors.Removed: a second export rule and a third spelling of it, plus the getattr defaults that could never fire. Preserved: every value a specialist is warmed with, bar the one effect below, pinned by the table test that already existed.
A negative
tp,conc,isl,osl,max_model_lenorbaseline_tputis now exported to the specialist rather than dropped. None of those fields has a negative sentinel - a negative value there is already a broken session, and an operator debugging one now sees it in the specialist's params instead of seeing a field that looks unset.Breaking changes: no
PR addresses single concern: yes
Root cause is upstream: no