Repository navigation
fix(sweep): run both arms at each CONC level so a budget cut keeps its pairs - #1892
zoroyihan7 wants to merge 1 commit into
Conversation
5d375ac to
b66b37d
Compare
Code metrics gate: PASSEDJudged: the 13 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): 2
Already over at the base (backlog, not this change's): 1
99 findings in files this change does not touchOutside this change (informational): 99
...and 74 more; the job log lists every row.
Thresholds and sources: |
b66b37d to
e74ca2f
Compare
|
@ZhengGong-amd @xiaofei-zheng rebased onto current main and ready for review: the sweep now runs both arms at each CONC level, so a budget cut keeps complete pairs. |
|
Closed the edge case: once the first arm of a CONC level has run, its partner is admitted even if that rung overran its estimate, so a budget cut can no longer leave an unpaired level (test added). |
5257be4 to
5fec4b5
Compare
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #1892 -- fix(sweep): run both arms at each CONC level so a budget cut keeps its pairs
What it does: The concurrency sweep used to run the optimized arm's whole CONC ladder before the baseline arm, so a --conc-sweep-total-budget-sec that ran out inside the first arm left zero pairs and SWEEP failed. It now walks the ladder one CONC level at a time (optimized, then baseline), starts a level only when 2 x rung_cost x passes is left (pass count lifted into _grid_runner.variant_rounds), gives the second arm of a started level an extended deadline (partner_bounds), and deletes the per-arm boot-once-and-reuse path so every rung boots its own server.
Blocking issues: 2
- [free:partner-overruns-session-deadline] The partner rung's deadline is pushed past the session wall-clock deadline, not only past the sweep budget [verified]
Problem:partner_boundsreturnsmax(session_deadline_sec, now + 2 x variant_expected_sec x variant_rounds()), andrun_runghands it torun_gridas the kill deadline (run_with_session_kill/ serving-leasesession_remaining_sec).session_deadline_sechere ismin(state.grid_session_deadline_sec(), now + total_budget)(conc_sweep.py:955,1062-1067), so when the session clock is the binding limit, which is the usual case because SWEEP clamps the budget to the session clock (phases/sweep.py:165-176), the partner runs past--max-hours' usable deadline and into the CLOSE reserve. src/hyperloom/orchestrator/kernel/conc_sweep.py:645
Impact: If the first rung of the last admitted level overruns, the partner can run up to 2 x rung_sec (with the default warm-up pass, 4 x the measured rung cost) beyond the session deadline. That overruns the window the phase granted (budget + 600 s, phases/sweep.py:246) and eats the reserve that CLOSE/report need. The edited help text at parser.py:1086 still promises the sweep is "bounded above by the main session wall-clock deadline", and nothing logs the extension.
Action: Extend only the sweep's own budget deadline and cap it at the session grid deadline (keep both deadlines on_SweepRuninstead of their min). Alternatively, droppartner_boundsand record a refused partner as an unpaired level. Add abounded=Truecase to the overrun test. - [C3] Removing the single-server path leaves its writers with no production caller [verified]
Problem: The only production callers were deleted with_sweep_one_arm_single_server(base conc_sweep.py:556,835,911,928,948). Left behind with no production caller:run_grid(lifecycle_boot_only=...)and its boot-only branch (_grid_runner.py:813,1739-1750),ConcSweepEventRecorder.record_arm_boot(conc_sweep_event.py:499),commit_variant(:624), thecommitted=parameter of_SweepRun.record_rung(conc_sweep.py:522), and thelifecycle_eligible/lifecycle_reason/port/frameworkparameters ofrecord_arm_strategy.STRATEGY_SINGLE_SERVERhas no reader, despite the comment at conc_sweep_event.py:83-84 saying it is "kept for events written by the earlier ladder". src/hyperloom/orchestrator/actions/executors/_grid_runner.py:1739
Impact: Only test_sbd_v6_conc_sweep_timeline.py keeps these names alive. The tree still advertises a boot-retry/commit protocol that the system no longer performs, which breaks AGENTS.md Finish the replacement ("delete what it replaced"; owners:_grid_runner.run_grid,ConcSweepEventRecorder).
Action: Delete them together with the tests that only exercise them. Keep the read side for persisted old events (conc_sweep_event.py:868STAGE_BOOT/STAGE_BOOT_ATTEMPT).
SKIPPED: V4 -- "# PARTIAL: #1869 / #1675 / #1276 / #1154 / #1032 ... only its first 100 were compared", overlap there unchecked
SKIPPED: tests -- pytest not installed in the local Python; scoped test_conc_sweep_session_deadline.py not run
Checked: conc_sweep.py (run_conc_sweep, _SweepRun.run_rung/_exhaust_on_deadline/admit_level/partner_bounds, _Ladder, _run_level_rung, _sweep_interleaved), _grid_runner.py (variant_rounds, run_grid gate, auto warm-up, _run_magpie deadline forwarding, lifecycle_boot_only), conc_sweep_event.py, phases/sweep.py (budget clamp, grant), shared_state.grid_session_deadline_sec, parser.py help; tree-wide sweep at head and base for lifecycle_boot_only, record_arm_boot, commit_variant, committed=False, STRATEGY_SINGLE_SERVER, STAGE_REUSE/BOOT | Ran: git fetch origin pull/1892/head; git show base/head; git grep (head, base) | Base: c9dbdc7 | Head: 5fec4b5
5fec4b5 to
9a5c396
Compare
|
Thanks, both findings addressed in 9a5c396 (branch also rebased onto current main).
Tests: 3388 passed across the conc-sweep/grid-runner test files; 3 failures also fail on current main in this environment and are unrelated to this change. |
9a5c396 to
6a8855b
Compare
|
Rebased onto current main (new head 6a8855b). Behaviour is unchanged. Two mechanical differences: |
…s pairs The sweep walks the ladder one CONC level at a time, highest first, and runs the optimized arm and then the baseline arm at each level, so a budget cut drops whole pairs rather than one arm. A level starts only when the time left admits both of its rungs, and once its first arm has run its partner is admitted even if that rung overran its estimate. The partner's extended deadline is capped at the session grid deadline, so it cannot run into the closing reserve; a partner the session cannot fit is refused by run_grid as an unpaired level. Delete what the removed single-server path left without a production caller: run_grid(lifecycle_boot_only=...) and its boot-only branch, ConcSweepEventRecorder.record_arm_boot and commit_variant, the lifecycle/port/framework fields of ArmStrategy and STRATEGY_SINGLE_SERVER, with the tests that only exercised them. The read side for persisted events (STAGE_BOOT / STAGE_BOOT_ATTEMPT, the arm boot and lifecycle blocks) stays, with a test. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: zoroyihan7 <Yihan.Wang@amd.com>
6a8855b to
8eca5fd
Compare
|
Closing. This PR was written for in-session concurrency sweeps under AgentX, where each rung is an hour-long replay and a budget cut used to leave rungs with only one arm. Since #1922, native AgentX no longer runs an in-session sweep, so the problem it addressed no longer occurs on main; the non-AgentX sweep rungs are short enough that a budget cut rarely splits a pair. The pair-major requirement (both arms back to back per concurrency, a cut drops whole pairs) is carried into the native AgentX proposal in #1966. |
2 participants
Description: The concurrency sweep ran the optimized arm's whole CONC ladder before the baseline arm started, and a pair needs both arms measured at one CONC. When
--conc-sweep-total-budget-secran out inside the optimized arm, the baseline arm never ran and the sweep endedskipped(budget_exhausted_no_successful_pairs) with zero pairs, which the SWEEP phase reads assweep_failed. With long rungs, such as a 3600 s measurement window plus a server start (about 4,500-5,700 s per rung), a 30,000 s budget ended this way.The sweep now walks the ladder one CONC level at a time, highest first, and runs the optimized arm and then the baseline arm at each level before moving down. Admission: a level starts only when the time left admits a rung of both arms, i.e.
2 x rung_cost_sec x passes, whererung_cost_secis the same per-variant estimate the grid already uses andpassesis whatrun_gridcharges a variant (2 with the default warm-up round, 1 without, plus 1 on a multi-node run with its benchmark warm-up on). That count is now one helper,variant_rounds, used by bothrun_gridand the sweep. A level that would end with one arm measured and its partner refused is not started. Once the first arm of a level has run, its partner is given a deadline of twice one rung's estimate (never earlier than the sweep's own), so a first rung that runs long cannot leave it refused byrun_grid's own gate; the ladder stops after that level.Because the arms take turns on the same GPUs, every rung now boots its own server, and one Ray serving lease spans the sweep instead of one per arm (two interleaved arms would otherwise hold two leases at once). The per-arm boot-once-and-reuse ladder (
_sweep_one_arm_single_server, boot-retry-descend) is removed.Unchanged:
conc_sweep_summary.json/conc_sweep_raw.csvformats,succeededon one or more pairs,skippedwithbudget_exhausted_no_successful_pairswhen the budget prevented every pair,cancelled, thebudget_skip_reasonvalues, and the SWEEP-phase re-dispatch on resume (fix(sweep): re-dispatch the conc_sweep when a resume finds SWEEP without one #1870), which sits above the kernel and is not touched.Linked issue(s): none.
Tests:
test_conc_sweep_session_deadline.pydrives the production grid and launch path on a virtual clock. New there, each failing on main: a budget that fits two levels yields two pairs (main: zero); a benchmark timeout in the optimized rung at the top level does not stop its partner or the levels below; a first rung that overruns its estimate still gets its partner run (fails without the change); levels run high to low with both arms at each, for sorted, shuffled and repeated ladders; a budget for exactly one arm does not start the level, before any pair, after a pair, and with the warm-up round doubling the admission. Mutation check of the admission rule: admitting on one arm's cost, dropping the level gate, dropping the warm-up passes,>=to>, and dropping the expired-deadline guard each fail at least one test. Replaced: the single-server boot/reuse/boot-retry tests intest_conc_sweep.pyandtest_sbd_v6_conc_sweep_wiring.pygo with the path they covered; closing, rung-exception, cancel, mid-sweep summary and event-wiring tests now assert the interleaved order and per-rung restart, and a cancel now records the failure on both arms. Commands:pytestover every test file that referencesconc_sweep,run_grid,_grid_runner, the CLI parser or code metrics;ruff check,ruff format --check,lint-imports(7 kept),python scripts/code_metrics.py --base-ref origin/main(PASSED).Size/complexity triggers crossed: none added.
run_conc_sweepdrops below the ceilings it was baselined over and_sweep_one_arm_single_serveris gone;scripts/code_metrics_baseline.txtis lowered to match. The new units (_Ladder,_run_level_rung,_sweep_interleaved,_SweepRun.admit_level) are each under the thresholds.If this simplifies or refactors: the per-arm single-server reuse mechanism is removed, since alternating arms per level cannot keep one server per arm. Contract preserved: report/CSV/summary schema and statuses, budget skip attribution (
budget_exhausted/session_time_exhausted,total_budget_exhausted/insufficient_remaining_for_variant), refused arms recording the gate, and the roundsrun_gridadmits a variant against (same arithmetic, now invariant_rounds).Observable effect: after a budget cut, no concurrency is left with only one arm measured (a level starts only when the time left admits both of its rungs, and levels run back to back), so the Pareto chart and the comparison show complete pairs instead of an unpaired point or zero pairs failing the phase. A level whose first arm has run always runs its partner: it is given a deadline that clears
run_grid's gate even when the first rung overran its estimate. Each rung now pays a server start, including on frameworks that used to boot once per arm, and theconc_sweepevent recordsstrategy=server_restartwithstage=server_restartrungs for both arms. Sizing: to pairLlevels give the sweep(L - 1) x 2 x Wplus2 x rung_cost_sec x passes, whereWis one rung's wall time including its server start; with a 4,700 s rung cost, about 5,000 s per rung and the warm-up round on, 30,000 s pairs two levels. CLI help,SKILL.md, the SWEEP section ofdocs/conceptual/optimization-loop.mdand the release notes say this.Breaking changes: no CLI or schema change. Behaviour change: a lifecycle-capable framework no longer reuses one server down an arm's ladder, so a sweep under a generous budget takes longer by one server start per rung.
PR addresses single concern: yes.
Root cause is upstream: no.
🤖 Generated with Claude Code