Repository navigation
refactor(orchestrator): remove duplicated writeback, autosubmit and config-env branches - #1877
Conversation
Every path through handle_unpromotable_result records the action failure before the save check, so the any_changed flag was always true by then. Save directly and drop the flag. Co-authored-by: Cursor <cursoragent@cursor.com>
…he winner Every caller builds the winning variant as a dict (a literal or a dict() copy), matching the bv: dict[str, Any] signature, so the per-field isinstance(bv, dict) guards and their empty fallbacks could never fire. Drop them. The guards on the previous current_best are untouched. Co-authored-by: Cursor <cursoragent@cursor.com>
…mary build_kernel_optimization_summary took a schema_version keyword that defaulted to 2, and its only caller, the session report, never passes it, so the version-1 branch was unreachable. Remove the parameter and always build the version-2 shape: lane-summed totals, the overall lane outcome and lane_totals. The emitted summary is unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
…ntegrate review maybe_autosubmit_specialist_patches and maybe_autosubmit_framework_config each carried a copy of the synthetic integrate_patch submission: the settled/in-flight gate, the patch name, provenance and candidate/batch forwarding, the enablement marker, carriers and merged setup commands, the proposal, the observation and the save. That now lives once, in _submit_integrate_review, with the params built by _integrate_review_params; each bridge keeps only what decides its deliverable. Observable effects: - A patch deliverable whose first proposal_set entry is not a dict is reviewed with an empty patch name instead of raising AttributeError out of the patch bridge. - The framework_config_autosubmitted_for_review observation now carries patch_name, as the patch bridge's observation already did. - The patch bridge parses its companion levers before the settled/in-flight check, so a redelivered deliverable for a settled specialist can log a lever-parse warning it used to skip. - Both bridges log the same line when the post-submit save fails. Co-authored-by: Cursor <cursoragent@cursor.com>
…g reader The recipe journal's tag collector and the T0 trace extras each read tp/ep/conc/isl/osl/max_model_len off SharedState, with the same EP environment fallback and PP environment read written out twice, and the T0 prefer hints kept a third copy of the field list. knowledge/workload_tags.py now owns the field list and the reader. The journal adds precision ahead of it, so both tag dicts keep their keys and their key order. No behaviour change. Co-authored-by: Cursor <cursoragent@cursor.com>
warm_specialist_params set the framework default twice. The earlier block already copies the stripped, non-empty SharedState.framework, so the second setdefault could only fire for a whitespace-only framework. It is removed and the earlier block is the single owner. tp, conc, isl, osl and max_model_len now come from one loop instead of five copies of the same positive-int guard. precision keeps its own string default. Observable effect: a whitespace-only SharedState.framework no longer reaches the specialist params as that raw string. Nothing else changes. Co-authored-by: Cursor <cursoragent@cursor.com>
…ables _build_trace_analyze_cmd grew one `if value: cmd += [flag, value]` guard per optional flag. Both runs of those guards now go through one _optional_flags helper fed with ordered (flag, value) pairs, and the three splitter hints come from one loop over (flag, payload key, workload key). The splitter loop keeps its own `not in (None, "")` predicate and .strip() renderer: a zero conc, osl or random_range_ratio is a real workload value and is still forwarded. A new argv case pins that, since no existing case passed a falsy-but-present hint. Cyclomatic complexity falls from 24 to under the gate limit, so its baseline entry is removed; cognitive complexity 45 -> 33. No behaviour change: the argv, including flag order, and the returned steady-state mode are unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
…ptional_positive_int _build_trace_analyze_cmd hand-rolled the same positive-int parse twice, each with its own try/except around int(). Both now use the shared hyperloom.common.coerce.optional_positive_int and emit through _optional_flags, which removes the function's last try blocks and takes its cognitive complexity under the gate limit. Observable effect: a boolean or infinite num_denoise_steps, num_inference_steps or SharedState.tp is now omitted like any other non-positive value. Before, True became `--num-denoise-steps 1` / `--tensor-parallel-size 1` and an infinite float raised OverflowError out of the handler. The argv for every integer, numeric string, zero, negative or unparsable value is unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
… forwarder baseline.py re-exported default_baseline_config and materialize_config_with_envs under underscore aliases. No production caller used them; one test module did. That module now imports the canonical names from _workload_envs, BaselineExecutor calls default_baseline_config directly, and the two YAML comments that named the alias point at the real function. resolve_reference_base only dropped the third element of resolve_reference_launch. Its two callers, the multi-node per-round restart in BenchmarkRunExecutor and the multi-node branch of run_grid, now unpack resolve_reference_launch themselves. No behaviour change. Co-authored-by: Cursor <cursoragent@cursor.com>
…ion state materialize_config_with_envs seeds the reference recipe from resolve_reference_launch, which loads SharedState from session_dir(). The autouse fixture drops the session-dir pin, so session_dir() falls back to workspace_root(): USER_DATA_PATH, or the default workspace. On a host whose workspace holds a session with a reference recipe, its server args and envs leaked into three golden snapshots. _isolate now stubs resolve_reference_launch to an empty recipe, the same seam test_workload_envs_branches_unit already uses. Clearing USER_DATA_PATH instead would only move the read to the default workspace, which is just as real and shared, and would trip the process-wide one-shot USER_DATA_PATH warning for every later test in the worker. Test-only change. Co-authored-by: Cursor <cursoragent@cursor.com>
…back-config-env Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # scripts/code_metrics_baseline.txt
Code metrics gate: PASSEDJudged: the 16 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
Already over at the base (backlog, not this change's): 9
84 findings in files this change does not touchOutside this change (informational): 84
...and 59 more; the job log lists every row.
Thresholds and sources: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| _mn_ref_envs: dict[str, str] = {} | ||
| if _mn_is_multi_node(): | ||
| from ._workload_envs import resolve_reference_base | ||
| from ._workload_envs import resolve_reference_launch |
|
/review |
|
Automated review started for head e611a05. Progress: https://github.com/AMD-AGI/Hyperloom/actions/runs/38551842106 |
…back-config-env main's warm_specialist_params tables cover the knob loop this branch added, so dispatch.py takes main's version. _collect_workload_tags keeps main's loop over the identity strings and reads the workload shape through workload_shape_tags, which also serves the T0 trace extras. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Automated review: no verdict applied -- the PR head moved from e611a05 to 9d5abf5 during the review. Comment PR #1877 -- refactor(orchestrator): remove duplicated writeback, autosubmit and config-env branchesWhat it does: Deletes duplicated or unreachable mechanisms on the writeback/promotion path and the config/env plumbing: an always-true Blocking issues: none SKIPPED: V4 -- open PRs #1276 (1311 files), #1675 (161 files), #1032 (1075 files) and #1154 (1232 files) were only partially compared in openprs.txt, so overlap with those PRs' files past their first 100 is unchecked. Checked: writeback.py handle_unpromotable_result / lift_to_current_best and all 10 production callers; phases/framework.py maybe_autosubmit_specialist_patches / maybe_autosubmit_framework_config / _submit_integrate_review / _integrate_review_params against both base copies; knowledge/workload_tags.py + _collect_workload_tags / _build_t0_trace_extras / _build_warm_prefer outputs run on both trees (byte-identical, including the EP env fallback and PP read); trace_analyze.py _build_trace_analyze_cmd run on both trees for 7 tp and 9 num_denoise value shapes; attempt_summary build_kernel_optimization_summary and its only caller; baseline.py / _grid_runner.py / _workload_envs.py removals swept tree-wide; ruff C901 at 20 on head and base copies of all changed src files; independent adversarial re-read of the card against the diff | Ran: ruff C901 head-vs-base complexity comparison; direct base/head execution of _build_trace_analyze_cmd, _collect_workload_tags, _build_t0_trace_extras; pytest (head tree) TestBuildTraceAnalyzeCmd (7 passed), splitter/tracelens selection (15 passed), test_workload_envs_golden_lock (10 passed), autosubmit selections in test_coordinator_async_batch2_unit (5 passed) and test_framework_agent_selection_audit (10 passed), test_coordinator_kb_writes (37 passed), test_kernel_attempt_summary (27 passed), test_profile_and_kernel_handlers (173 passed, 1 env-only failure reproduced identically on base) | Base: d755931 | Head: e611a05 |
PR #1877 -- refactor(orchestrator): remove duplicated writeback, autosubmit and config-env branchesWhat it does: Deletes mechanisms that existed twice or could never fire in the writeback/promotion path and the config/env plumbing. The always-true Blocking issues: none Checked: diff of all 18 files; LGTM |
|
Non-blocking suggestions. None of these blocks merging; each removes a defensive check or a mechanism rather than adding one.
|
…back-config-env _build_trace_analyze_cmd conflicted with the trace splitter cleanup (#1612). Its behaviour is kept as main has it: no --skip-split, the splitter hints behind --split-llm-inference for a non-scriptable TraceLens run only, and the generic steady-state mode for a scriptable TraceLens run. Its inline four-flag loop is the same rule as _optional_flags and is folded into it; the two branches it gated with "scriptable" and "not scriptable and not is_bypass" are one if/elif.
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #1877 -- refactor(orchestrator): remove duplicated writeback, autosubmit and config-env branches
What it does: deletes mechanisms that existed twice or could never fire on the writeback/promotion path and the config/env plumbing: the always-true any_changed save flag and the winner-side dict guards in lift_to_current_best, the unreachable schema_version branch of the kernel attempt summary, the second copy of the specialist autosubmit integrate review, three copies of the workload-shape tag reader, per-flag trace-analyze argv guards, and the baseline.py aliases and resolve_reference_base forwarder. The PR's own content is unchanged since the previous LGTM at 5836d15: every commit after it is a merge of main.
Blocking issues: 1
- [mergeable] The branch conflicts with main [verified]
Problem: GitHub reportsmergeable: CONFLICTING(mergeStateStatus: DIRTY);mainhas moved 24 commits past the last merge, including #1922.git merge-tree origin/main 794d477d3conflicts inscripts/code_metrics_baseline.txt,assets/configs/baseline_atom.yaml,assets/configs/baseline_sglang.yaml,tests/test_profile_and_kernel_handlers.pyandorchestrator/actions/executors/baseline.py. src/hyperloom/orchestrator/actions/executors/baseline.py:765
Impact: the PR cannot merge, and #1883, stacked on this branch, inherits the same five conflicts.
Action: merge or rebase ontomain. Inbaseline.py, keep this PR's deletion of the_default_baseline_configalias (#1922 kept it onmain, but nothing in the merged tree reads it;:3977callsdefault_baseline_config()). Intest_profile_and_kernel_handlers.py, #1922 already migrated 68 of the 70 alias uses; carry the remaining_default_baseline_configimport onmain's side to the canonicaldefault_baseline_config. In the two baseline YAMLs, takemain's comment (materialize_benchmark_config, the entry point after #1922) rather thanmaterialize_config_with_envs. Regeneratecode_metrics_baseline.txtwith--update-baseline. Then grep the merged tree for_materialize_config_with_envs,_default_baseline_configandresolve_reference_base: there must be no reader left.
Checked: git log --no-merges 5836d152..794d477d3 (only main commits); the five conflict hunks in git merge-tree --write-tree origin/main 794d477d3; every reader of the three deleted names on origin/main and in the merged tree; the YAML comment on both sides | Ran: git merge-tree against origin/main; tree-wide git grep at origin/main and on the merged tree | Base: 0198f43 | Head: 794d477
…back-config-env Main's AgentX contract change (#1922) renamed the entry point the profile tests and the baseline YAML comments name, and re-exported _default_baseline_config from baseline.py. The tests and comments follow main; the alias stays removed, so the one test that used it imports default_baseline_config from _workload_envs. The baseline keeps main's entries, tightened with --update-baseline.
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
Re-reviewed at 0cf8890: the merge of main resolves the five conflicts as advised (the _default_baseline_config alias stays deleted, the last test import moves to default_baseline_config, both baseline YAMLs take main's comment, and the code-metrics baseline only lowers or drops entries). No reader of _materialize_config_with_envs, _default_baseline_config or resolve_reference_base remains, the other 13 files carry the same change as the previous LGTM, and CI is green.
LGTM
Implements the cyclomatic/cognitive-complexity findings for the writeback/promotion path and the config/env plumbing. Each finding is a mechanism that either existed twice or could never fire. The change deletes the copy or the dead branch, and the complexity falls as a consequence. There is one commit per finding, each self-contained and each tightening the baseline for what it shrank.
handle_unpromotable_resultsaved behind anany_changedflag that every path had already set (the action failure is always recorded first), so it now saves directly.lift_to_current_bestguarded each field of the winning variant withisinstance(bv, dict), but every caller builds that variant as a dict, as the signature says. The guards on the previouscurrent_bestare untouched.build_kernel_optimization_summarytook aschema_versionkeyword defaulting to 2. Its only caller never passes it, so the version-1 branch was unreachable. It is gone.maybe_autosubmit_specialist_patchesandmaybe_autosubmit_framework_configeach carried a full copy of the syntheticintegrate_patchreview submission: the settled/in-flight gate, patch name, provenance, candidate/batch forwarding, enablement marker, carriers and merged setup commands, proposal, observation and save. It now lives once, in_submit_integrate_review/_integrate_review_params, and each bridge keeps only what decides its deliverable. Re-checked against feat(predictor): ask PrimaTune for optimization proposals in FRAMEWORK_AGENT #1798 after merging main: both bridges seedprovenance="specialist"before_forward_integrate_source, before and after this change, so a domain-less predictor-mandate dispatch keeps its label through either bridge.SharedStatewith their own EP-env fallback and PP-env read, and the T0 prefer hints kept a third copy of the field list.knowledge/workload_tags.pynow owns the list and the reader._build_trace_analyze_cmdguarded each optional flag with its ownif value: cmd += [flag, value]and hand-rolled the same positive-int parse twice. Main's trace-splitter cleanup (Cleanup trace splitter code #1612) folded four of those guards into an inline loop of its own. Every optional flag now comes from ordered(flag, value)tables through_optional_flags, which absorbs that loop, and the splitter hints come from one loop, which still forwards a zero hint. Both parses useoptional_positive_int. Cleanup trace splitter code #1612's argv is kept as it is: no--skip-split,--split-llm-inferenceand the hints only for a non-scriptable TraceLens run, and--steady-state-mode genericfor a scriptable one.baseline.pyre-exporteddefault_baseline_configunder an underscore alias that only its own_resolve_default_configand one test module used, andresolve_reference_baseonly dropped the third element ofresolve_reference_launch. Callers now use the canonical names. Main's Use Magpie's native AgentX contract for optimization #1922 already removed the second alias,_materialize_config_with_envs.resolve_reference_launch. On a host whose workspace holds a session with a reference recipe, its server args and envs leaked into three goldens._isolatenow stubs that seam, astest_workload_envs_branches_unitalready does.Superseded on main: the specialist-dispatch commit (
93642f651). main'sbdf43087amovedwarm_specialist_paramsonto two tables and dropped the same duplicated framework export. The merge takes main'sdispatch.py, so this PR no longer changes it.Not included: normalizing a non-dict
current_bestonce atSharedState.from_dictand deleting the guards downstream of it. That moves a persisted-state boundary and is scoped separately, in #1883.Linked issue(s): none
Tests: added/updated? commands run?
Added:
test_autosubmit_patch_survives_a_non_dict_first_proposal: a malformed first proposal costs the review its patch name, not the review.framework_agent_authoringassertion on the config bridge intest_autosubmit_config_enablement_propagates_marker_and_setup.test_splitter_forwards_zero_workload_hints: a zeroconc/osl/random_range_ratiois still forwarded. No existing case passed a falsy-but-present hint.Updated:
test_profile_and_kernel_handlers.pyimportsdefault_baseline_configfrom_workload_envsinstead of the deleted alias.test_workload_envs_golden_lock.py::_isolatestubsresolve_reference_launch.Before syncing with main, ran locally on Windows:
ruff check .andruff format --check ..PYTHONPATH=src lint-imports --no-cache: 7 kept, 0 broken.python scripts/code_metrics.py --base-ref origin/main: PASSED.origin/mainfor the same files, none new.origin/mainand of the pre-branch base), each for a Windows-only reason: symlink privileges,\in expected POSIX paths, the cp1252 default encoding, drive-letter colons, TraceLens not installed, or process teardown.After merging
origin/mainata17939e68:ruff checkandruff format --checkon the changed files: clean.origin/main: PASSED, with 16 files judged.origin/mainata17939e68on this host: Windows paths and venv layout, git patch line endings, the cp1252 default encoding, and process teardown.The full suite is left to Linux CI.
The four new units (
_submit_integrate_review,_integrate_review_params,workload_shape_tags,_optional_flags) are under every threshold, and every touched unit shrinks.Leaving the baseline:
_build_trace_analyze_cmd: cyclomatic 23, cognitive 45.maybe_autosubmit_specialist_patches: cyclomatic 24, cognitive 58.maybe_autosubmit_framework_config: cognitive 43, 138 lines._build_t0_trace_extras: cognitive 31.phases/framework.py.Lowered:
lift_to_current_best: cognitive 213 → 148, cyclomatic 51 → 44.build_kernel_optimization_summary: cognitive 63 → 59, cyclomatic 23 → 21._collect_workload_tags: cognitive 42 → 37.maybe_autosubmit_specialist_patches: 170 → 102 lines._build_trace_analyze_cmd: 104 → 87 lines.The units still above a ceiling were already above it.
Removed:
Each of the four new names replaces at least two copies.
Preserved, and pinned by:
the synthetic
integrate_patchparams:test_coordinator_async_batch2_unit.py,test_framework_agent_selection_audit.py;the KB tag key sets and key order:
test_coordinator_kb_writes.py, and main's whole-dict pin intest_coordinator_sync_helpers_coverage_unit.py;the trace-analyze argv, including flag order:
TestBuildTraceAnalyzeCmdintest_kernel_request_handlers_units.py, and Cleanup trace splitter code #1612's framework-kind gating intest_trace_splitting.py;the workload-env goldens, byte for byte:
test_workload_envs_golden_lock.py.Observable effect:
Framework autosubmit:
proposal_setentry is not a dict is reviewed with an empty patch name, instead of raisingAttributeErrorout of the patch bridge.framework_config_autosubmitted_for_reviewobservation now carriespatch_name, as the patch bridge's observation already did.trace-analyze: a boolean or infinite
num_denoise_steps,num_inference_stepsorSharedState.tpis now omitted like any other non-positive value. Before,Truebecame--num-denoise-steps 1/--tensor-parallel-size 1, and an infinite float raisedOverflowErrorout of the handler. Every integer, numeric string, zero, negative or unparsable value produces the same argv as before.Unobservable: the writeback, kernel-summary, KB-tag, argv-table and executor-alias commits, and the test-only isolation fix. The emitted summary, tag dicts, argv and goldens are unchanged.
Breaking changes: no for operators. One Python signature narrows:
build_kernel_optimization_summary(listed in its module's__all__) no longer acceptsschema_version. Its only caller never passed it, and the emitted summary is unchanged. The removedbaseline.pyalias was underscore-private.PR addresses single concern: no. It bundles the complexity findings for two areas, one commit per finding, each reviewable and revertible on its own. They are the same kind of change (delete a duplicated or unreachable mechanism, then tighten the baseline), and most are a few dozen lines. As separate PRs they would each spend a CI run and a review on very little.
Root cause is upstream (Magpie/TraceLens/GEAK/IntelliKit/AgentKernelArena), ticket filed: no