[Stepping] Relocate the enablement subsystem and make ENABLEMENT the sixth phase - #1491
Conversation
| from hyperloom.orchestrator.tests._fixtures import ( # noqa: F401 | ||
| NoLaunchBackendInstalled, | ||
| _isolate_session_layout_env, | ||
| launch_backend, | ||
| virtual_clock, | ||
| ) |
9a39948 to
9eb6f49
Compare
| from hyperloom.orchestrator.tests._fixtures import ( # noqa: F401 | ||
| NoLaunchBackendInstalled, | ||
| _isolate_session_layout_env, | ||
| launch_backend, | ||
| virtual_clock, | ||
| ) |
d0e541b to
26c539c
Compare
|
|
||
| #: The marker substrings alone, for a reader that must guarantee its text still | ||
| #: witnesses every milestone after it drops the bulk of a log. | ||
| PROGRESS_MARKER_SUBSTRINGS: tuple[str, ...] = tuple(marker for _stage, marker in _PROGRESS_MARKERS) |
|
|
||
| #: The marker substrings alone, for a reader that must guarantee its text still | ||
| #: witnesses every milestone after it drops the bulk of a log. | ||
| PROGRESS_MARKER_SUBSTRINGS: tuple[str, ...] = tuple(marker for _stage, marker in _PROGRESS_MARKERS) |
|
Reviewed the full diff. Two blocking issues; everything else I checked held up. 1.
|
|
Both blocking issues are fixed in 1. The budget flag is removed, not wired up. Wiring 2.
One correction to the clean list. On The stamping is gated twice on eval-enablement, not just on a live accuracy gate. Both Worth keeping reachable, because the same predicate carries this PR's |
The two report sub-trees were spelled by hand in five places. reports/bringup/ had no owner at all: bringup/persist.py and bringup/trees.py each appended the literal to reports_dir() independently, and breakdown/collectors/sessions.py bypassed enablement_dir() to spell the enablement setting-script path twice. Adds bringup_dir(), enablement_builds_dir() and enablement_stacks_dir() alongside the existing enablement_dir(), and exports BRINGUP_SEGMENT / ENABLEMENT_SEGMENT so session_package.py's glob patterns -- which cannot take a Path -- derive from the same source rather than repeating the string. enablement_stacks_dir() covers <session>/enablement/stacks/, a second root outside reports/ that nothing owned either. Co-authored-by: Cursor <cursoragent@cursor.com>
The classifier had no production consumer inside the agent runtime -- all of its call sites are in the orchestrator, two of them (bringup/ladder.py and framework/adapters.py) at module level. Living under agents/framework/ meant the orchestrator imported an agent's Python in-process, which makes the subprocess JSON contract that is supposed to be the API surface decorative. common/ is the only non-inverting home: placing it under enablement/ would force bringup and framework to import enablement, which imports both of them. The module has zero first-party imports, so it satisfies the common/ layering guard as-is, and FailureSignature belongs to the same vocabulary as the BootObservation and LadderStage already in common/bringup.py. All 15 failure-kind string values are unchanged -- they are persisted in session breakdowns. Co-authored-by: Cursor <cursoragent@cursor.com>
enablement_ops.py held the repo's only reverse dependency edge: a function-local import of orchestrator.framework.paths wrapped in a bare `except Exception: pass`, both of which existed only to hide the cycle orchestrator -> agents.framework.enablement_ops -> orchestrator.framework.paths. As enablement/mandate.py that becomes an ordinary top-level import and the guard goes with it. The narrower try/except around resolve_kernel_search_roots stays: the probe itself can fail on a host with no source trees, which is a different failure from the module being unimportable. _enablement_artifacts.py was a private module of phases/ whose only production consumer was the enablement lane. It becomes enablement/artifacts.py and loses the underscore. This also removes the enablement -> phases module-level edge. Both modules keep their module-level imports of agents.framework.keywords and repo_map. That is the legal orchestrator -> agents direction, and keywords.py is genuinely shared agent-side, so it must not move. Co-authored-by: Cursor <cursoragent@cursor.com>
Sixteen lines re-exporting one function and one constant. repo_url_for_framework now comes straight from agents.framework.repo_map, which is what enablement/params.py already did. DISCOVER_FAILURE_RETRY_LIMIT moves to phases/framework.py rather than to framework/artifacts.py: artifacts.py classifies candidate outcomes, and a discovery retry bound has nothing to do with that. Its only two production readers are in phases/framework.py, so it lands beside them and the module count drops by one instead of staying flat. Co-authored-by: Cursor <cursoragent@cursor.com>
bringup/argv_preflight.py reached framework/adapters.get_adapter() through a function-local import purely to call argv_parser_source(), which pinned the whole adapter registry -- venv creation, pip installs, ROCm probes -- at the bringup layer. That is what blocked the acquisition half from moving. The coupling turned out to be thinner than the class hierarchy suggests: argv_parser_source has exactly one caller in the repo and all four implementations are a bare `return "<source string>"` with no calls into the acquisition side. Extracting the strings into framework/adapter_parsers.py lets bringup import a 65-line lookup and nothing else, so no class hierarchy is split and no shared base module is needed. With that edge gone, adapters, stack_actions, localization, build_actions, build_utils and targeted_build move to enablement/runtime/ intact. The cluster depends on nothing in orchestrator/ outside itself, so the move adds no reverse edges. framework/ is left holding paths.py, artifacts.py and adapter_parsers.py. Two things that would have failed silently: - build_lifecycle._driver_command spawned `python -m hyperloom.orchestrator.framework.targeted_build`. A stale string there raises at build-spawn time, not import time, so it now derives the path from the module's own __name__. - test_git_foreign_checkout_callers reads a module's source by path to scan for unguarded git calls. Left pointing at the old path it would have scanned a 33-line shim and silently protected nothing. Also carries the attempt_root fix: enqueue_targeted_build pre-generates the task_id so it can fill action.attempt_root before the row is written. The executor's private _attempt_root() and the re-derive fallback in enablement/build.py both go away, and the params no longer keep the enqueue-time default they were documented as keeping. Co-authored-by: Cursor <cursoragent@cursor.com>
Twenty-six test files under inference_optimizer/tests/ imported only hyperloom.orchestrator.* and belonged in the packages they test. The style guide already asks for **/tests/ next to the code under test, and pyproject's src/**/tests glob collects the new directories with no config change. The relocation is not a bare file move, because a test taken out of inference_optimizer/tests/ silently loses that package's 466-line conftest -- including the autouse _isolate_session_layout_env, which clears the session-dir pin and points MULTI_NODE_STATE_FILE at a missing sentinel. So: - orchestrator/conftest.py takes _isolate_session_layout_env, launch_backend and virtual_clock. It sits at the orchestrator root rather than src/, whose scope would be every test package in the repo. - orchestrator/tests/_helpers.py takes init_git_repo, git_commit_all, patch_integrate_patch_roots and variant_result. These were pulled in through `from .conftest import`, which pytest does not make available across packages, so they need a real importable module. Every new tests/ directory gets an __init__.py. The repo is split on this today, but with importmode=prepend two same-named files in two non-package directories collide in sys.modules, and several of these basenames are generic enough to collide later. test_argv_refusal_round.py stays put: it imports fixtures from test_bringup_round_scenario.py, which is one of four files that genuinely depend on inference_optimizer.protocol / session / breakdown and belong where they are. Co-authored-by: Cursor <cursoragent@cursor.com>
enablement/__init__.py was four lines with no __all__. It now exports the three mandate names that callers outside the package address, and deliberately not the four CoordinatorCollaborator subclasses: those are resolved by Coordinator._COLLAB_MODULES through dotted strings, and exporting them would invite instantiation outside the coordinator. This is the shape bringup/__init__.py already uses. The guard follows kernelforge's test_rename_completeness.py: a git ls-files sweep, an allowlist whose every entry carries a written justification, and a self-validating test that fails when an entry stops exempting anything. It enforces two rules: - No pre-relocation dotted path may come back. The relocation left no shims, so any such reference is a live regression rather than a deprecation. - agents/framework/* must not import orchestrator/*. This is the rule that would have caught the enablement_ops reverse edge the day it landed. Scoping it to agents/framework rather than all of agents/ means it lands with an empty allowlist; the repo-wide version would have needed twenty entries, nearly all for agents/kernel/tools, and traded one real fix for a list nobody maintains. It also asserts the relocated modules import and that _driver_command's spawn argv still matches targeted_build's own __name__, since both are string-typed and fail late. adjustment.md records where the implementation departed from enablement-refactor-2.plan.md and why. Co-authored-by: Cursor <cursoragent@cursor.com>
Three fallbacks that could not fire. enqueue_targeted_build fills attempt_root, so the re-derive in _route_succeeded_build and TargetedBuildExecutor._attempt_root were unreachable; both are gone, and the enqueue now fills the field only when the caller left it unset instead of overwriting an explicit one. The broad `except Exception` around resolve_kernel_search_roots guarded nothing -- the function returns an empty tuple for "nothing here to search" rather than raising -- so it and the test that asserted the swallow are gone too. Seven docstrings described the move rather than the code: "Moved from", "Hoisted from", "previously reached these through". A reader of the module does not need its history, and the style guide asks for neither. orchestrator/conftest.py had copied four definitions that still existed in inference_optimizer/tests/conftest.py. They move to orchestrator/tests/_fixtures.py and both conftests import them, which is what registers a fixture; neither package sits under a shared ancestor conftest, and one at src/ would scope them to the whole repo. The relocation guard shrinks: the empty agents/framework allowlist drove a loop that could only ever match nothing, and two exemptions covered docstring lines this commit deletes. adjustment.md keeps the decisions and drops the plan-step narration. Also repoints a comment in common/provenance.py that named targeted_build by its old path. Co-authored-by: Cursor <cursoragent@cursor.com>
The phase chain is now: PRELUDE -> ENABLEMENT -> FRAMEWORK_AGENT -> KERNEL_AGENT -> SWEEP -> CLOSE ENABLEMENT is entered from PRELUDE when enablement is admitted (--enablement != off, not multi-node) and at least one baseline has failed (baseline_failure_streak >= 1). A healthy run never enters the phase. compute_next_phase gains three keyword arguments -- enablement_enabled, enablement_stalled (from RoundStore.consecutive_stalled), and enablement_in_flight -- passed by the already-async _advance_phase_if_needed. machine_state.py acquires no new first-party imports. Normal exit requires all three: baseline_tput > 0, no open revalidation window, and no queued/running enablement targeted_build or integrate_patch. The third conjunct is what makes the close-guard collapse sound: without it a build outliving the round would re-open validation_pending from inside FRAMEWORK_AGENT through _maybe_rearm_authored_lane. Terminal exits: server_argv_invalid, environment_fault (written by the lane's terminal helpers via stop_reason, routed by _global_terminal), and enablement_attempts_exhausted (consecutive_stalled >= ENABLEMENT_MAX_ATTEMPTS). The close guard is deleted: - enablement_close_guard_active() from shared_state.py - MAX_SKIP_TO_CLOSE_SUPPRESSIONS constant - skip_to_close_suppressions field from EnablementRound - the suppression block and observation in intent_router.py The drain exit condition makes validation_pending implies phase == ENABLEMENT a consequence of the machine, not of a separate predicate. Both the three-strike gate (baseline_failure_streak >= 3) and the combined backstop (_BASELINE_MAX_TOTAL_FAILURES = 3) in writeback.py are suppressed while phase == ENABLEMENT. Without this, the cap of 8 rounds is unreachable because 3 failures terminate the run first. The dead stop reason "enablement_stalled" is removed from STOP_REASON_VOCAB (it had no production writer). ENABLEMENT_MAX_ATTEMPTS moves from coordinator.py to machine_state.py, removing the import inversion through lane.py. Budget: ENABLEMENT 5%, FRAMEWORK_AGENT down from 40% to 38%, KERNEL_AGENT down from 50% to 47%. Sum stays 1.0; work >= 0.8. Allowlist: PRELUDE's set plus specialist, integrate_patch and targeted_build. targeted_build is added because cancel_queued_not_allowed fires on every transition and targeted_build was in no allowlist; it stays non-proposable because allowed_actions_for() subtracts COORDINATOR_INTERNAL_ACTIONS. Surfaces updated: _PHASE_ORIENTATION (critic gets an ENABLEMENT entry), _BASELINE_RECOVERY_PHASES (rules F1/F2 render in ENABLEMENT too), orchestration.md (phase-goal section, roofline tag), v6.py phase_map, attribution.py phase_buckets, render.py cycle_reloop set, CLI flag pair, failure_recovery.md reference tag, and all external docs. Three three-strike tests set enablement_mode="off" to keep exercising the documented fast-fail path. Six close-guard and suppression-counter tests are deleted; one is rewritten against the phase check the machine now owns. The SVG diagram is re-laid-out with a sixth box (dashed orange border for ENABLEMENT, indicating conditional entry); the PNG should be regenerated from the SVG. Co-authored-by: Cursor <cursoragent@cursor.com>
The ENABLEMENT branch of compute_next_phase re-derived three exits the lane already writes to stop_reason, which _global_terminal routes ahead of every phase branch — dead code, and a second place to keep the vocabulary in sync. Drop the branch's terminal arm and the enablement_stalled argument that fed it; the branch now decides only the normal exit. Fold the rest of the duplication the phase change introduced: - _enablement_work_in_flight re-implemented the lane's _enablement_in_flight and got the discriminator wrong: it looked for params["enablement"], which TargetedBuildAction.to_state never sets, so a running build read as drained. Call the lane's own query, and only from inside the phase, since it renews the round lease as a side effect. - The admission predicate lived in both the lane and the phase machine. It belongs to the lane; the phase machine reaches it through the collaborator registry. Hoisting it also absorbs the is_multi_node check that sat halfway down the pump, past work a multi-node host cannot use. - writeback derived in_enablement twice in one block; derive it once, next to the eval-suppression flag it is read beside. - _on_enter_enablement only logged, duplicating the ENTER lifecycle event the dispatcher already emits for every phase. enablement_stalled is no longer written by anything now that the cap stops with enablement_attempts_exhausted, so drop its report explanation and point the three tests that sampled it at the reason that is actually reachable. Add the enablement key to the PhaseBreakdown TypedDict so the schema again describes what the collector emits, and trim the comments that restated their own code or narrated the move. Co-authored-by: Cursor <cursoragent@cursor.com>
…rminal The lane helper was renamed in a prior commit; seven call sites in test_environment_fault_round.py still used the old name, causing an AttributeError at module collection time that silently dropped all 13 tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Neither enablement_entered nor enablement_done appeared in any test; enablement_enabled=True was never passed. Five focused cases now cover: - PRELUDE with a baseline_failure_streak routes to ENABLEMENT - the branch is skipped when enablement_enabled=False - ENABLEMENT exits (enablement_done) once tput is set and work is drained - the phase holds while work is in flight - the phase holds while validation_pending is set Co-authored-by: Cursor <cursoragent@cursor.com>
EnablementBreakdown reached the JSON output but was invisible in the Markdown report. The new renderer surfaces admission status, round outcomes, a bounded rounds table, and a build-attempts table. Skipped automatically when the enablement section is empty, so sessions that never triggered enablement are unaffected. Co-authored-by: Cursor <cursoragent@cursor.com>
- redistribute_budget_pct docstring: ENABLEMENT is excluded from absorbers alongside PRELUDE and CLOSE - _post_prelude_target docstring: called on ENABLEMENT exit too, not only PRELUDE - backfill_langfuse.py comment: add ENABLEMENT to the phase-span list - session-breakdown.md: note that a subset renders in the Markdown report Co-authored-by: Cursor <cursoragent@cursor.com>
…p commits The renderer read four field names no producer emits (attempts/opened_at on rounds, status/framework on builds), so half of every table rendered blank. Point the columns at the fields EnablementRoundSummary and TargetedBuildAttemptSummary actually carry. Also drop what was redundant: - unused warnings list threaded into RenderedSection - two near-identical table helpers collapsed into one - per-row isinstance skips, which silently dropped malformed rows that render_section already reports - a 10-row display cap duplicating the collector's _MAX_ROUNDS bound - str()/or-None coercions md_kv_list and _md_cell already handle - duplicated test state builders merged; a disjunctive assertion made exact and its explanatory comment removed Restore the return-value precedence dropped from the _post_prelude_target docstring and trim the session-breakdown note. Co-authored-by: Cursor <cursoragent@cursor.com>
…efactor #1455 retired the read-side breakdown collectors and #1415 gave the lane a first-class timeline event. Rebasing onto them left four defects and two surfaces main introduced against pre-relocation names. Rebase damage: - schema.py carried literal conflict markers into the tree, so the module did not parse. Its only change here targeted PhaseBreakdown, a TypedDict #1455 deleted, so main's file is restored whole. - collectors/attribution.py was resurrected by a later commit in the series after the modify/delete resolution dropped it. Nothing imports it; #1455 deleted it deliberately. - lane.py lost the local binding record_dispatch reads for its origin, because the admission check moved into _enablement_admitted. Read it from state. The enablement renderer read a top-level "enablement" key that #1455 removed along with EnablementBreakdown, so it skipped on every session. It now reads the lane's timeline event, whose ext carries richer counters than the retired projection did: rounds settled/landed/advanced, build failures, revalidations promoted, and human-review parks. Columns are taken from the fields enablement_event actually writes. Mirrored onto the relocated names: - test_integrate_patch_provision patched adapters at the pre-relocation path; the relocation guard caught it. - test_sbd_v6_enablement_wiring imported ENABLEMENT_MAX_ATTEMPTS from coordinator and bound the terminal helpers by their pre-rename names, and needed _enablement_admitted bound alongside them. Co-authored-by: Cursor <cursoragent@cursor.com>
#1455 added _framework_policy_fields with a function-local import of ..framework.client, a module this branch deletes. _open_framework_timeline wraps record_policy in except Exception, so the ImportError was swallowed and the phase silently recorded no policy at all. The constant already lives in phases/framework.py, so the import just goes. The relocation guard missed it: its pattern anchors on the package-qualified orchestrator.framework.X, and a relative import carries no such prefix. Widen it to the relative spelling so a function-local one cannot hide again. test_reporters_smoke pins the renderer registration order, which the new enablement renderer joins. Co-authored-by: Cursor <cursoragent@cursor.com>
adjustment.md is a local working note, not a repo artifact, so it joins the PR-scratchpad block in .gitignore. Its allowlist entry in the relocation guard goes with it, since an entry that stops matching fails test_every_allowlist_entry_still_exempts_something. Co-authored-by: Cursor <cursoragent@cursor.com>
read_bringup_log took 64 KiB from each end of server.log on the assumption that the boot milestones are at the head and the wall at the tail. Only the second half holds. A build whose kernel layer logs per shape -- aiter on ROCm emits a line per GEMM/MoE shape -- pushes `application startup complete` megabytes in: on a GLM-5.3 MXFP4 boot it landed at byte 329,448 of a 4.9 MB log, inside the discarded middle. The ladder then witnessed no HTTP_READY and stamped stage_reached=ENGINE_INIT with no stage_failed, so BootObservation.booted was False for a server that had loaded, captured graphs, served 936 tok/s and scored gsm8k 0.975. That is indistinguishable from a server that hung, and it is load-bearing: runnable_decision reads booted, so the enablement lane could only ever rate such a round `advanced`, never `kept`. succeeded never flips, the phase never exits, and the session spends its whole budget authoring patches against a wall it already cleared. Stream the middle for milestone-bearing lines and carry those into the text between the edges. The edges still go to the failure excerpt untouched: the terminal frame really is at the tail. Markers come from the ladder's own table so there is one source of truth for what a milestone looks like. Co-authored-by: Cursor <cursoragent@cursor.com>
…scan preflight_optimizer skipped only os.getpid() when scanning /proc for a leftover optimizer or serving process. The shell that invokes the tool carries the whole launch command in its own argv, so the shell, its wrapper and the agent harness above it all match `hyperloom.inference_optimizer.cli` with no prior run in existence. IR-1 then exits 2 and the launcher must abort on a machine whose GPUs are idle. Walk PPid up to init and exclude the chain, so the scan reports foreign workload only. Co-authored-by: Cursor <cursoragent@cursor.com>
BaselineExecutor, ExploreExecutor and IntegratePatchExecutor resolved session_dir in __init__. Two of them are instantiated as module-level singletons at import, which happens before the CLI's make_session_dir() pins $INFERENCE_OPTIMIZER_CURRENT_SESSION_DIR, so _resolve_session_dir() took its workspace_root() fallback and the executors stayed pinned there for the whole run. Nothing failed loudly, because the paths that matter most are passed in per task. What leaked was the bring-up observations: every session on a workspace wrote them into one shared $USER_DATA_PATH/reports/bringup/, where concurrent runs for other models accumulate, while session_package globs the session's own reports/bringup and finds nothing. On a WekaFS workspace shared by runs on several hosts that is cross-run contamination of gate evidence. Resolve on read instead, with an explicitly passed directory still winning and assignment still supported. The invariant now lives on _resolve_session_dir, which owns the resolution. Co-authored-by: Cursor <cursoragent@cursor.com>
A resume is documented to keep the launch shape, and state.json is the authority for it, but two pieces were rebuilt from the flags instead. --max-hours has a real default, so a bare resume silently reset a 24h session to 2h; with 2.14h already charged across earlier legs the leg closed as time_exhausted before its first action, and wrote a final report that reads like an ordinary end of run. The target flags default to None, so the same resume dropped `--target-gain 300` and left the session stopping only on the clock. The resume banner printed "budget: 24.00h total, 21.86h left" while this happened, because the ledger was right and only the Coordinator's copy was wrong. This is worst where nobody is watching: robustness_monitor.sh auto-resumes with no flags at all, so any session longer than the default could not survive a watchdog restart. Restore both from the archive when the resume did not name them -- the budget from state.max_minutes, which already carries every --extend-hours grant, and the objective from the manifest that recorded it. An explicit flag still wins, and a target named on the resume replaces the persisted objective rather than joining it, which build_objective refuses. Co-authored-by: Cursor <cursoragent@cursor.com>
`baseline_tput` has one writer, reachable only from a promoted `baseline` task, and the ENABLEMENT phase exits on `baseline_tput > 0`. Only an eval-origin KEEP opened the revalidation window that produces that task; a boot-origin KEEP set `succeeded` outright and dispatched nothing. Since a specialist cannot run a baseline and `integrate_patch` deliberately keeps its own measurement out of the anchor, a boot-origin session had no actor left that could satisfy its own exit condition. It would author patch after patch against a wall it had already cleared and close on the wall clock with a zero baseline, even after the server booted, served and passed its eval. Open the window for both origins and let the promote path be the only place that sets `succeeded`. The pump already gates on `validation_pending` alone and already prefers `accepted_config_path`, which a KEEP sets regardless of origin, so the revalidation replays the accepted stack either way. The promote gate needs `accuracy_meets_floor`, which demands a finite, strictly positive score, so a session run with `--no-eval` would never clear it and the window would reopen forever. Honour the operator's choice: when the session disabled eval there is no accuracy to judge and the revalidation promotes on throughput. Co-authored-by: Cursor <cursoragent@cursor.com>
ENABLEMENT holds the machine for the whole phase, so a second bring-up would fight the round in flight for the same cards and port. The table said as much in a comment -- that `baseline` is Coordinator-internal here and the model only proposes `specialist` and `integrate_patch` -- but nothing enforced it. The claim named `_NOT_LLM_PROPOSABLE`, which is global and cannot carry it: `baseline` is the one action PRELUDE exists to propose. So the phase advertised `baseline` in its allowed set, the decision framework told the model to propose one whenever `baseline_tput == 0`, which in this phase is always, and the round guard refused every attempt. The model was asked for the one thing it was not permitted to do. Give the reservation a place to live. `PHASE_COORDINATOR_RESERVED` says which actions a single phase keeps for the Coordinator; `allowed_actions_for` subtracts it, so the phase bullet and the `allowed:` line stop offering `baseline`, and the decision framework's Measure item reads the same table rather than a second hardcoded list of phases. `baseline` stays in the phase's allowed set on purpose: the revalidation is a real part of ENABLEMENT and must survive the cancel sweep a phase transition runs. What changes is who may ask for one. The gate sits in `_admission_denial_for_action`, already the single entry point for propose, delegate and the inline runner, and refuses only a reserved action -- the Coordinator's own revalidation prices itself through the budget gate directly and never passes here. Co-authored-by: Cursor <cursoragent@cursor.com>
The bring-up ladder exists to explain a failure: how far a boot climbed and where it stopped. `runnable_decision` was asking it the opposite question -- did this server serve -- which it cannot observe. It reads a capped window of the server log and looks for one of three readiness substrings, so a build whose kernel layer logs per shape can push `application startup complete` clean out of view. A GLM-5.3 boot did exactly that: the server loaded, captured graphs, served 936 tok/s and scored gsm8k 0.975, and the gate called it `stage_reached=ENGINE_INIT`, hence not runnable, which is indistinguishable from a server that hung. The comment defending the choice said throughput "cannot separate a slow server from a dead one". That confuses the magnitude of a measurement with its existence: `is_valid_measurement` already requires both a positive throughput and a completed request, and a dead server completes none. The benchmark separates served from dead definitionally; only how well it served is a matter of degree. It was the one authority that observes the thing itself, and the one the gate refused. Take the served witness from the measurement and leave the ladder the job it can do -- `round_advanced` and the failure it explains, both still read from the same observations. `boot_timed_out` goes with it: `_bench_patch` never put `timed_out` in the evidence it returns, so that branch was unreachable from the only caller. `BootObservation.booted` keeps its ladder meaning and now says so instead of promising a request it never saw. Co-authored-by: Cursor <cursoragent@cursor.com>
A phase budget divides the session between the phases that produce a result. ENABLEMENT produces none: it exists because the combo cannot run, and a combo that cannot run has nothing to optimise. Sizing it as a fraction of the optimisation clock is the wrong unit in both directions -- generous enough to waste, and tight enough to abandon a model that was one patch from serving. The share was already inert: `compute_next_phase` calls `phase_cap_exceeded` for KERNEL_AGENT, SWEEP and FRAMEWORK_AGENT and never for ENABLEMENT, so the 0.05 only ever reached the rendered budget line. Drop the row rather than wire the check up. `phase_cap_seconds` already separates an absent key, which is no cap, from a zero share, which is no wall clock at all, and the renderer already prints `cap=unlimited` for it -- so this needs no code beyond the deletion. The phase keeps the bounds that suit it: the consecutive-stall cap on rounds, and the session clock. The remaining shares are untouched, since they are upper bounds rather than a partition. `--phase-budget-enablement-pct` still imposes one for an operator who wants it. Co-authored-by: Cursor <cursoragent@cursor.com>
Three executors had grown the same eight-line property to resolve session_dir on read. One descriptor beside the resolver it calls carries the rule instead, so the reason it exists is stated once rather than three times. `read_bringup_log` justified its middle scan by the runnable gate reading `booted`, which no longer decides anything: the gate takes the measurement now. The scan still earns its place, for a different reason the docstring now gives -- an understated `stage_reached` makes two unequal boots compare as equal -- and the constant above it stops repeating the docstring. `_parent_pid` caught IndexError and ValueError for a malformed /proc that cannot occur; a process that exits mid-walk raises OSError and that is the one worth answering. The resume restore drops the isinstance guards it put around a manifest this repo writes itself, and the comments that narrated a bug rather than stating a constraint are cut to the constraint. Co-authored-by: Cursor <cursoragent@cursor.com>
… flags The argv preflight asks the installed parser whether it will accept a server argv, and uses `parse_known_args` so an unrecognised flag comes back as a token it can name and drop. vLLM's FlexibleArgumentParser folds `--<group>-config.<field> <value>` into that group's JSON inside `parse_args` only; its `parse_known_args` adds a deprecation warning and delegates straight to argparse. Every dotted flag therefore came back as a leftover, and preflight spent its one repair dropping flags the server would have taken. The dropped set is not cosmetic. It carried `--profiler-config.max_iterations`, which vLLM reads as "profile until stop_profile" when absent, and Magpie's wrapper then re-adds the unbounded profiler flags of its own. A GLM-5.3 roofline profile recorded the entire 192-prompt workload instead of a steady-state window: 25.7 GB of trace across four ranks, and an analysis window that no longer described decode. `_workload_envs` already re-asserts these bounds for the three ways materialisation can drop them, and says in its comment why a missing `max_iterations` is an OOM rather than a blemish -- but it runs before preflight, so it cannot reach this one. Judge the argv through the entry point that expands the flags, and recover the leftovers from the intercepted `unrecognized arguments` message. A genuinely unknown flag is still named and still droppable; a rejected value still lands on `value_rejected` with no repair, since that message carries no leftover list. The misjudgement spanned every `--<group>-config.*` flag, not just the profiler. Co-authored-by: Cursor <cursoragent@cursor.com>
The monitor's terminal check returned on the mere presence of `reports/final.md` or `reports/final.json`, before reading any state. But a final report is not only CLOSE's work: the crash path writes one too, as a safety net. A resume clears `stop_reason` and carries on; the report from the interrupted leg stays on disk. So the artifact outlives the condition it described, and the monitor reads a running session as finished. A GLM-5.3 session resumed into FRAMEWORK_AGENT with an empty `stop_reason`, and the monitor exited `final_artifact` on its first poll -- abandoning the run it exists to guard, for the rest of that run's life. `state.json` is the authority on session state everywhere else, so let it answer here too. The artifacts stand in only when there is no state to read, which keeps the case they genuinely settle: a supervisor that wrote a final report and left nothing else behind. Co-authored-by: Cursor <cursoragent@cursor.com>
…oducts `test_qwen3_8b_3h_no_kernel_budget_shape` pinned the redistributed shares as literals, 0.9902 and 0.0198, which are the freed KERNEL share fanned out by the override weights. Moving KERNEL from 0.50 to 0.47 changed both products and the test has been failing since, on a number the demo's SKILL.md does not quote -- it names only the 0.50 and 0.01 inputs. Assert what redistribution actually promises: the absorbers keep the ratio the overrides set, and FRAMEWORK_AGENT ends up with essentially the whole wall clock. Neither depends on KERNEL's share, so the next budget change cannot leave a stale literal here. Co-authored-by: Cursor <cursoragent@cursor.com>
…ng an explicit one
Two defects from the review, both mine.
`--max-minutes-enablement-pct` parsed a cap that nothing enforced.
`compute_next_phase` calls `phase_cap_exceeded` for KERNEL_AGENT, SWEEP and
FRAMEWORK_AGENT only, so an operator bounding bring-up got a computed cap, a
`True` from the predicate, and no transition:
ENABLEMENT cap_exceeded=True -> compute_next_phase = None
SWEEP cap_exceeded=True -> ('FRAMEWORK_AGENT', 'cycle_reloop', ...)
Wiring the check up would reinstate the mechanism this branch just argued out of
existence -- a combo that cannot run has nothing to optimise -- so the flag goes
instead. It is new on this branch, so nothing depended on it.
`--max-hours` carried an argparse default of 2.0, and the resume restore treated
that value as "the operator said nothing". Resuming a session whose persisted
budget is 8h while explicitly asking for 2 restored 8, four times the request,
against `_start_run`'s documented contract that a leg starting with a smaller
`--max-hours` runs against the smaller one. Absence is now `None` and the
default is settled after the restore has had its chance, so the two are
distinguishable rather than merely unequal most of the time. The AgentX budget
note tested the same 2.0 by hand; it reads the constant now.
Co-authored-by: Cursor <cursoragent@cursor.com>
main dropped PHASE_EXIT_REASONS and is_valid_phase_exit_reason as a closed vocabulary no production path read. The two ENABLEMENT predicate tests this branch added called the helper, and the exact-value assertion above each is strictly stronger than the membership check below it. Co-authored-by: Cursor <cursoragent@cursor.com>
ATOM became a supported framework on main, which taught the teardown gates its ``atom.entrypoints.openai_server`` cmdline. The IR-1 preflight scan is the same kind of gate on the way in and still matched only vLLM and SGLang, so a leftover ATOM server still holding every rank's VRAM read as a clean machine and the run started on top of it. Matched on the entrypoint alone: the per-rank workers are ``multiprocessing.spawn`` children carrying no identifying argv, so only descent from the wrapper reaches them, which teardown already covers. Co-authored-by: Cursor <cursoragent@cursor.com>
The phase promotion is the user-visible half: phase_history gains ENABLEMENT rows, PHASE_NAMES is six long, and the phase takes no wall-clock budget, so the budget table sums to 0.95 and an absent key reads as no cap. The resume entry is the one an operator can hit without knowing any of this changed -- robustness_monitor.sh auto-resumes with no flags, and the argparse default on --max-hours made that indistinguishable from asking for 2 h. Co-authored-by: Cursor <cursoragent@cursor.com>
The style guide asked for one "when maintainers expect a release note", which is a standard an author cannot apply to their own PR. This branch promoted ENABLEMENT to a phase across 33 commits and carried no entry at all, so the release cut would have had to reconstruct one from commit subjects. The rule names what counts as observable and what is exempt, putting the burden on the omission rather than on whoever reads the diff later. It lands in the four places an author meets it: the authoring rules, the contributing checklist, the PR template, and the AI review prompt -- the last because a missing entry is precisely what no static gate catches. Co-authored-by: Cursor <cursoragent@cursor.com>
01bc91a to
632912d
Compare
|
Re-reviewed at Blocking: CHANGELOG contradicts the budget tableCHANGELOG.md, in the ENABLEMENT entry:
They do not. This branch changes Non-blocking:
|
| invocation | args.max_hours at line 985 |
note printed | wanted |
|---|---|---|---|
no --max-hours (is at the default) |
None |
no | yes |
--max-hours 2 (explicitly chosen) |
2.0 |
yes, "is at its default of 2.0" | no |
test_agentx_budget_and_guards.py cannot see it: _budget_args() hardcodes max_hours=2.0 rather than parsing an argv, so the fixture never produces the None the parser now does. Moving the check below line 1661 fixes it.
Also checked, clean: the ATOM entry added to STALE_PROCESS_PATTERNS; the retired is_valid_phase_exit_reason assertions; the reworked test_phase_budget_help_quotes_the_real_default; and every remaining reader of args.max_hours (bootstrap.py:298, bootstrap.py:546-558 via _seed_shared_state at line 2145, manifest.py:290, and lines 2184/2335/2443/2460/2472) — all either None-tolerant or reached after the settling on both the fresh-launch and resume paths.
Tests: pytest src/hyperloom/inference_optimizer/tests/test_machine_state.py src/hyperloom/inference_optimizer/tests/test_phase_state_machine.py — 60 passed; the remaining failure and errors are all a Windows-only fcntl import in cli/kb.py, unrelated to this PR.
The flag no longer carries an argparse default, so the profile runs before either path settles the budget and sees ``None``. Testing equality against ``DEFAULT_MAX_HOURS`` inverted the note: silent when nothing was passed, loud when the operator typed 2.0. Also correct the changelog's claim that the other phases kept their percentages. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed
Ran |
xiaofei-zheng
left a comment
There was a problem hiding this comment.
No blocking issues. Both items from the earlier round are resolved — disclosure: by my own commit 704e0040f on this branch, so this approval covers the author's work, not mine; 704e0040f itself is unreviewed by anyone else.
What I checked:
compute_next_phaseENABLEMENT entry and exit:baseline_tput > 0,validation_pending,enablement_in_flight; the branch deliberately never consultsphase_cap_exceeded, consistent with ENABLEMENT having noDEFAULT_PHASE_BUDGET_PCTkey. The 0.05 the table no longer allocates falls through to the run's own wall clock, not to a phase that can overrun._pump_enablement_safelyis now gated on the phase being ENABLEMENT. The run-deadline guard removed from_maybe_enqueue_enablement_specialistis backstopped by the end-of-tickdeadline.expired()check incoordinator.py, so removing it does not let work outlive the deadline.- Round mutex (
rounds.open/held),ENABLEMENT_MAX_ATTEMPTS, and the revalidation window:enablement_attempts_exhaustedis the terminal exit and is reachable. - Accuracy gate:
BASELINE_EVAL_FAILED_KEY/_is_promotable_result/_promote_baseline. The sub-floor rearm branch in_promote_baselineis unreachable because a sub-floor result withbaseline_eval_failedset is diverted to_handle_unpromotable_resultfirst — dead, not wrong. - argv preflight:
_PARSE_PROGRAMrebindsparser.error/parser.exitto raise, so every argparse error class reaches theinvalidpath and the drop regex only selects flags that are safe to drop. --max-hourssentinel: absence staysNonethrough_restore_budget_and_objective, so a bare--resume-fromrestores the persisted budget, and an explicit value equal to the default still wins over it. Both paths settle the default afterwards.- CHANGELOG entry and PR description both match the diff, including the 40→38 / 50→47 budget shift.
Tests run locally (targeted, not the full suite): test_agentx_budget_and_guards.py and test_resume_budget_objective.py, 46 passed. Full CI on this head is green across all six shards.
Description: what and why
Three related changes to the enablement subsystem, landed together because each depends on what the previous one establishes.
Part 1 — module relocation (commits 1–8). Enablement code lived in six places across
agents/andorchestrator/. This collapses it to three owners, deletes the repo's only reverse dependency edge, and moves 26 test files next to the code they exercise.The reverse edge is the substantive part:
agents/framework/enablement_ops.pyreached back intoorchestrator.framework.pathsthrough a function-local import wrapped in a bareexcept Exception: passthat existed only to hide the cycle. Asorchestrator/enablement/mandate.pythat becomes an ordinary relative import and the guard disappears. Likewiseclassify_failurehad no consumer inside the agent runtime — all 22 call sites were in the orchestrator — so living inagents/made the orchestrator import agent Python in-process and rendered the subprocess JSON contract decorative. It moves tocommon/failure_signature.py.Two silent failures fixed in passing:
build_lifecycle._driver_commandspawnedpython -magainst a hardcoded module path (now derived fromtargeted_build.__name__), andtest_git_foreign_checkout_callerswas scanning a 33-line shim instead of the module it meant to guard.Part 2 — ENABLEMENT as the sixth phase (commits 9–17). The chain is now:
ENABLEMENT is entered from PRELUDE when the lane is admitted and
baseline_failure_streak >= 1. A run whose baseline boots first try never enters it —streak == 0, so PRELUDE goes straight to_post_prelude_target().compute_next_phasegains two keyword arguments (enablement_enabled,enablement_in_flight) supplied by the already-async_advance_phase_if_needed.machine_state.pygains no new first-party imports; the facts arrive through the same channelkernel_enabledandoptimize_enabledalready use.The normal exit requires three conjuncts:
baseline_tput > 0,not validation_pending, and no enablement work still in flight. The third is what makes the close-guard deletion sound —_maybe_rearm_authored_laneroutes on the result's lane, not the current phase, so a build outliving its round would otherwise reopenvalidation_pendingfrom insideFRAMEWORK_AGENT.This is a net reduction in mechanism.
enablement_close_guard_active(),MAX_SKIP_TO_CLOSE_SUPPRESSIONS, theskip_to_close_suppressionsfield, the intent-router suppression block, and the dead"enablement_stalled"stop reason are all deleted; the phase boundary now expresses what they approximated.Budget: FRAMEWORK_AGENT 40→38%, KERNEL_AGENT 50→47%, and ENABLEMENT deliberately carries no share (see Part 3).
Follow-ups (commits 13–17) close what a branch-wide sweep turned up: a test left broken by a method rename, zero coverage on the new phase predicate, and
EnablementBreakdownreaching the JSON but never the Markdown report.Part 3 — let the phase finish a bring-up it has already achieved (commits 18–28). Part 2 gave enablement a phase boundary; running it against a real model showed the phase could not cross that boundary. A GLM-5.3 MXFP4 session spent its whole budget in ENABLEMENT after its patch had worked: the patched server loaded 409 GB of weights, captured graphs, served 936 tok/s and scored gsm8k 0.975, while the phase recorded
baseline_tput = 0, rated the round "advanced", and kept authoring patches against a wall it had cleared. Four defects had to line up. None is a missing mechanism; each is an existing one applied at the wrong scope, so every fix removes a special case rather than adding a guard.Runnability was decided by a log scan that a chatty log defeats.
runnable_decisionasked the bring-up ladder whether the server served, which the ladder cannot observe: it matches readiness substrings in a capped window ofserver.log. A build whose kernel layer logs per shape pushedapplication startup completeto byte 329,448 of a 4.9 MB log, so a fully served boot classified asstage_reached=ENGINE_INIT— indistinguishable from a hang. The comment defending the choice said throughput "cannot separate a slow server from a dead one", which confuses a measurement's magnitude with its existence:is_valid_measurementalready requires a positive throughput and a completed request, and a dead server completes none. The gate now takes the served witness from the measurement and leaves the ladder the job it can do —round_advancedand the failure it explains.boot_timed_outgoes with it: its evidence key was never written, so the branch was unreachable from its only caller. The log reader is still fixed, for the reason that survives — an understatedstage_reachedmakes two unequal boots compare as equal.Only one of the two origins could establish a baseline.
baseline_tputhas one writer, reachable only from a promotedbaselinetask. An eval-origin KEEP opened the revalidation window that produces that task; a boot-origin KEEP setsucceededoutright and dispatched nothing. Since a specialist cannot run a baseline andintegrate_patchdeliberately keeps its own measurement out of the anchor, boot-origin had no actor left that could satisfy its own exit condition. Both origins now open the window, and the promote path is the only place that setssucceeded. It honours--no-evalthere, sinceaccuracy_meets_floordemands a positive score such a session never produces.baselinewas advertised to the model and then refused. The phase table's comment saidbaselineis Coordinator-internal here, but named a global set to enforce it — which cannot carry the rule, becausebaselineis the one action PRELUDE exists to propose. So the phase offered it, the decision framework told the model to propose one wheneverbaseline_tput == 0(always, in this phase), and the round guard denied every attempt.PHASE_COORDINATOR_RESERVEDnow names what a single phase keeps for the Coordinator,allowed_actions_forsubtracts it, and the prompt reads the same table.baselinestays in the phase's allowed set so the revalidation survives the transition sweep; what changes is who may ask for one.The phase carried an optimisation budget. A budget divides the session between phases that produce a result; ENABLEMENT exists because the combo produces none. The 0.05 share was already inert —
compute_next_phasenever calledphase_cap_exceededfor this phase — so the row is dropped rather than the check wired up.phase_cap_secondsalready separates an absent key (no cap) from a zero share (no wall clock), and the renderer already printscap=unlimited. The phase keeps the bounds that suit it: the consecutive-stall cap on rounds, and the session clock.--max-minutes-enablement-pctgoes with the row rather than staying as a flag that parses a cap nothing enforces (caught in review).Found alongside, in the same area: IR-1's stale-process scan excluded only
os.getpid(), so the launcher shell — whose argv quotes the whole command — matched its own patterns and failed the gate on an idle machine. Three executors are module-level singletons built before the CLI pins the session, so resolvingsession_dirin__init__froze them on the workspace root and every session on a shared WekaFS workspace wrote its bring-up observations into one directory; it is resolved on read now, owned by one descriptor beside the resolver. And a bare--resume-fromrebuilt--max-hoursand the stop target from the flags, so it shortened a 24 h session to the 2 h default and closed the leg astime_exhaustedbefore its first action, and dropped the objective — which is whatrobustness_monitor.shdoes on every auto-resume, since it passes no flags at all.Two unrelated fixes, done in passing (commits 29–30). The argv preflight probed through
parse_known_args, which vLLM's parser does not use to expand--<group>-config.<field>, so every dotted flag read as unrecognised and preflight spent its one repair dropping flags the server would have taken; one of them bounded the profiler, and a roofline recorded 25.7 GB of trace over the whole workload instead of a steady-state window. Separately, the robustness monitor treated the presence ofreports/final.*as terminal, but the crash path writes one as a safety net and a resume clearsstop_reasonwithout removing it, so the monitor read a running session as finished and abandoned it —state.jsondecides now, with the artifacts standing in only when there is no state to read.Linked issue(s): close/fix refs
None.
Tests: added/updated? commands run?
Both.
Added: five cases covering the ENABLEMENT entry and exit predicate (neither
enablement_enterednorenablement_donewas exercised by anything, andenablement_enabled=Truehad never been passed);test_enablement_relocation_completeness.py, agit ls-filessweep with a self-validating allowlist, an importability check for all ten canonical module paths, and a spawn-path check for_driver_command. For Part 3:test_bringup_log_reader.py(a served boot whose readiness marker lands past the head window still reads as booted, and a tail wall still outranks a middle milestone);test_resume_budget_objective.py(six cases over the restore, including that an explicit flag wins and that one named target replaces the persisted objective rather than joining it); the phase-reservation and admission-gate cases; the ladder-independence case onrunnable_decision; and theeval_disabledpromote path.Fixed:
test_environment_fault_round.pybound a lane helper by its pre-rename name, raisingAttributeErrorat collection and silently dropping all 13 of its tests.Updated: constant assertions (
PHASE_NAMES, budget identity/sum,_PHASE_ORIENTATION, CLI redistribution figures,PHASE_GOAL_BLOCKS); three three-strike tests now setenablement_mode="off"to stay honest about the fast-fail path they assert. Six close-guard and suppression-counter tests deleted along with the mechanism. In Part 3, the lane and breakdown tests that asserted a boot-origin KEEP setssucceededsynchronously now assert the revalidation window it opens, the eval-origin-only case is folded into the general one rather than kept beside it, and the enablement gate's bench stubs carrycompleted_requests— which is what the realVariantResulthas always reported.Passing on the touched surfaces (848 + 270 on the two largest selections); leaving the full suite to CI.
Breaking changes: yes/no (details if yes)
No, for anything outside this repo. Internally, resume across the change is not supported: a session whose
state.jsonrecordsphase: "ENABLEMENT"cannot be resumed on pre-merge code. New sessions are unaffected, andEnablementRound.from_dictfilters unknown keys, so olderstate.jsonfiles load without migration.One in-repo behaviour change worth naming: a boot-origin enablement KEEP no longer sets
succeededat the moment it lands. It opens a revalidation window andsucceededfollows when that baseline promotes, which is what eval-origin already did. The lane event stays open for that interval rather than closing at the KEEP.PR addresses single concern: yes/no (details if no)
No — three, plus two unrelated fixes. The phase insertion needs the relocated modules to avoid re-creating the import cycle it deletes, and Part 3 is what running the phase from Part 2 against a real model turned up, so each part reopens the files the previous one established. They are cleanly separated in history (commits 1–8, 9–17, 18–28, 29–30) and can be reviewed in that order.
Root cause is upstream (Magpie/TraceLens/GEAK/IntelliKit/AgentKernelArena), ticket filed:
Not upstream — entirely in-repo. The argv-preflight fix in commit 29 is a misuse of vLLM's
FlexibleArgumentParseron our side, not a defect in it:parse_known_argsis documented to delegate to argparse, and we were asking the wrong entry point.