Repository navigation
fix(cli): export every --extra-env pin so all readers resolve it alike - #1797
Conversation
PR #1797 -- fix(agentx): derive client knobs and workload_spec from --extra-env pinsWhat it does: the CLI serializes Blocking issues: none Checked: Cases run, through the real
Ran: LGTM |
PR #1797 -- fix(agentx): derive client knobs and workload_spec from --extra-env pinsWhat it does: the CLI serializes Blocking issues: 1
Note on severity, stated because it is the one thing that weakens this: Checked: Ran: |
The CLI serialized the operator's pins into INFERENCE_OPTIMIZER_EXTRA_ENV alone, so a bare os.environ.get could not see them and the AgentX switch had to re-merge the blob locally. That split let one reader resolve a knob differently from the rest: a pinned HYPERLOOM_AGENTIC_BACKEND selected the MLPerf client while the seeded grading axis, the persisted backend identity and the benchmark timeout all still read aiperf. Pins now become real environment variables, applied after the flag-derived workload knobs so a pinned TP/CONC/EP wins, and dropped on a resume that no longer passes them. The blob stays for the state roundtrip and to name what the previous launch exported. Nothing is withheld from the export: an operator who can pass --extra-env can equally export the same name.
xiaofei-zheng
changed the title
Response to the blocking finding on
|
PR #1797 (re-review at 63c66e7) -- fix(cli): export each --extra-env pin so every reader resolves it alikeWhat it does: the CLI serialized The previous head's finding is resolved at the root, verified: with Blocking issues: 1
Checked: the move of Ran: Base: 1c14e79 | Head: 63c66e7 | CI: all 37 checks success at this head |
PR #1797 -- fix(cli): export each --extra-env pin so every reader resolves it alikeWhat it does: the CLI serialized the operator's Blocking issues: 1
Checked: |
The guard resolves the AgentX backend and benchmark mode with a bare os.environ.get, but the persisted pins were re-exported a hundred lines later. A session seeded with --extra-env HYPERLOOM_AGENTIC_BACKEND=mlperf therefore recorded agentx_backend="mlperf" and was then refused on resume as "measured with agentic backend 'mlperf' but this run is 'aiperf'"; --extra-env HYPERLOOM_AGENTX=1 failed one check earlier on benchmark_mode. Both became reachable only once the pins started reaching those readers. The restore moves ahead of the guard; the re-export block below keeps the state writeback and the operator-facing print.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Which value a pinned ISL/OSL/CONC/TP/EP/PRECISION ended up with was decided by write order, and the two branches write in different orders: the fresh branch projects args back into the environment after the pin export, so --extra-env ISL=4096 ran at the default 1024 while benchmark.envs said 4096; the resume branch exported later and the pin survived. One session, two workload identities across a resume. _resolve_workload_knobs now takes these pins as a rung of its ladder -- flag, pin, resumed state, default -- so args is the single answer and every later projection writes it. An explicit flag outranks a pin, matching the ladder _resolve_run_max_model_len_inner already uses. Those names are no longer exported directly, because feeding both the export and the ladder is what let a pin beat a flag on one branch and lose to it on the other; the blob still carries them, which is what the ladder and state.json read. MAX_MODEL_LEN and FRAMEWORK stay exported: their ladders read the environment, so the pin has to reach it. SKIP_VARIANTS is not pinnable -- it is the policy for one run, not part of the measurement contract.
|
Both findings confirmed and fixed. Reproduced each through the production entry point before changing anything.
The fix is not a reorder. Moving the export below the projections only relocates the collision, because the two branches run it at different points and a pin that both exports itself and feeds a ladder has to beat an explicit flag on one branch and lose to it on the other. So This reverses something the earlier description claimed: a pinned
Added cases, each failing on the commit before its fix: a full |
…ze through _run_optimize writes SKIP_VARIANTS, PD_MODE and INFERENCE_OPTIMIZER_NODES directly, so monkeypatch had nothing recorded to undo and the next test in the worker graded on what this one left behind: shard 1/6 failed on geak_metric_axis and the graded-comparison axes.
PR #1797 -- fix(cli): export each --extra-env pin so every reader resolves it alikeWhat it does: the CLI serialized the operator's Blocking issues: 2
Checked: |
…n change MAX_MODEL_LEN Two regressions from withholding the ladder-resolved names from the export. _export_workload_envs_for_optimize is the only non-test writer of os.environ["TP"|"CONC"|"EP"], and it ran about 500 lines before _resolve_workload_knobs filled the pin, from flag-derived values. The fresh re-projection after the ladder covered MAX_MODEL_LEN/ISL/OSL/PRECISION and omitted these three, so --extra-env TP=4 EP=2 CONC=63 gave args and state.json 4/2/63 against an environment still at 1/1/64 -- and the environment is what the server launches from, what the Magpie YAML and the comparability fingerprint are built from, and what the preflight GPU-count gate reads. The projection moves below the ladder. The topology gates are now a function called twice: once on the flags, so a bad --tp still fails before the slow preflight work, and again on the resolved values, so a pinned TP cannot pass a gate the shape it launches with would fail. On the resume branch _resume_max_model_len read flag-or-state and wrote it back over the pin the restore had just placed, so re-passing --extra-env MAX_MODEL_LEN to change it was silently discarded -- 63c66e7 honoured it because its export sat after that loop. It now goes through _resolve_resume_max_model_len: flag, pin, recorded value.
|
Both confirmed and fixed in
On the gates: I did not move them, because they also run on the resume branch, where
On the tests, you were right that the four ladder cases prove nothing about ordering. The case that pins One thing I did not change, in case you read it differently: |
CodeQL flagged the file for importing hyperloom.inference_optimizer.cli with both 'import' and 'import from'. monkeypatch.setattr takes a dotted string target, so the module object is not needed to patch _preflight and friends.
…e enumeration Two blocking defects in this PR survived a green CI and my own verification, each for a reason the rules did not name. T5: the fix moved a projection relative to a resolver, and the tests added with it called the two in the order they wanted -- passing both before and after, while the production path still ran them the wrong way round. An ordering fix has to assert the order itself, or drive the real entry point. V7: the AST walk I used to record every os.environ write in _run_optimize never listed TP/CONC/EP, because they are written one frame down inside _export_workload_envs_for_optimize. A tool scoped to one function and a tree with no writers produce the same empty output, so the scope of the search has to be stated with its result.
PR #1797 -- fix(cli): export each --extra-env pin so every reader resolves it alikeWhat it does: the CLI serialized the operator's Blocking issues: 1
Checked: |
Moving the TP/CONC/EP projection below the ladder carried it past _preflight, and check_gpu_visibility reads $TP to warn when a run asks for more shards than rocm-smi can see. TP is withheld from the pin export because the ladder owns it, and --tp defaults to None, so on a fresh launch the name was simply unset there: wanted fell back to 1, the warning became unreachable for any non-zero GPU count, and the step recorded tp_requested=1 into the session regardless of --tp. _resolve_workload_knobs now fills the five numeric knobs only, and runs before the gates, the projection and _preflight, so all four see the same TP. Precision splits off into _resolve_precision, which stays after the model is resolved because it reads the checkpoint -- running it early would detect nothing and settle on the default with no later call able to correct it. The second _enforce_topology_gates call goes away: with the ladder ahead of the gates, the first call already sees the resolved shape. The ordering test becomes a table of anchors rather than one pair. Each time this order was corrected for one anchor it broke for another, which a single assertion could not catch.
|
Confirmed and fixed in I did not add a second projection. The projection moved late because the ladder was late, so the ladder moves instead: Precision could not come along: it reads the checkpoint's The ordering test is now a table of anchors rather than one pair, which is the actual lesson -- each time I corrected this order for one anchor it broke for another, and a single assertion could not see that: On I also ran the sweep in the direction that would have caught this: for every name Worth stating plainly: that sweep is the one I should have run before moving the projection, and not running it is why this round existed. It is now |
_enforce_topology_gates still said it was called twice, and the launch-shape export still justified withholding the ladder names by where it runs on each branch. Both describe the previous arrangement; the gate runs once, and the reason those names stay out is that the projection is their single writer.
…ird source The pin machinery had grown five mechanisms -- an exclusion set, a filter in the export, a matching exclusion in the unset loop, a pin-reading int helper and a resume-only MAX_MODEL_LEN resolver -- all of them there to keep a pin from colliding with the projection of the ladder's answer. They existed because I treated a pin as a third source alongside the flag and the environment. It is not: _export_operator_launch_shape puts it in the environment, which MAX_MODEL_LEN has always read as its second rung. Export every pin, have the ladders read $NAME, and the collision cannot arise -- the export runs before the ladder, the ladder writes args, the projection writes args. An explicit flag still outranks both, and a pin and an operator's own export now mean exactly the same thing, which is what this PR claims. Net 92 lines removed from cli/__init__.py against the previous commit, and one concept fewer to explain.
xiaofei-zheng
changed the title
Three sets of tests outside the files I had been running, which is how CI caught them and I did not. The early ladder call reached args.resume_from on a Namespace that the topology-gate tests build with four attributes, since nothing before the gates used to need it; it goes through getattr like every other attribute read in that stretch. _resolve_workload_knobs no longer fills precision, so the cases asserting it call _resolve_precision -- renamed to say so -- or both halves through a local helper, the way _run_optimize calls them. The ladder now has an environment rung, so a test asserting what the flags, the state and the defaults produce has to start from an empty environment rather than whatever the worker ran before it. The autouse fixture in test_cli_workload_envs.py clears the set on the way in as well as restoring it on the way out.
The docstrings and comments added by this PR ran to twice the lines of the code they sat on, most of it recounting how the arrangement got here rather than what holds it in place. Keep the constraints -- the model has to be resolved before precision is detected, the pins have to be exported before the guard reads the backend, SKIP_VARIANTS is not part of the measurement contract -- and drop the rest. One orphaned comment block left over from moving the projection goes with it.
They are review-process rules with no runtime code, and carrying them here made this a two-concern PR. They go out on their own branch instead.
Backtesting the rule against #1797's own history: the whole-diff totals read 6591 code against 1216 comment and cleared at every head, because test code dilutes the ratio and the merge of main swamped it. cli/__init__.py measured on its own read 28 against 56 and fired from the second commit onward. A per-commit reading inverts for the opposite reason -- a docs-only commit adds comment by construction.
PR #1797 -- fix(cli): export every --extra-env pin so all readers resolve it alikeWhat it does: the CLI serialized the operator's Blocking issues: 1
Checked: |
On #1797 the grep on the split symbol reaches two of the three files; the third drives the entry point the moved call sits inside and names the symbol nowhere. The evidence line now asks for that grep too.
3 participants
Description: The CLI serialized the operator's
--extra-envpins intoINFERENCE_OPTIMIZER_EXTRA_ENValone and never exported the individual names. Every Hyperloom control variable is read with a bareos.environ.get, so none of them could see a pin, and AgentX compensated by re-merging the blob insideagentx_env_for_conc— one reader resolving a knob differently from all the others. A pinnedHYPERLOOM_AGENTIC_BACKEND=mlperftherefore selectedmlperf_agentic_client.shwhile the seeded grading axis,state.agentx_backendandresolve_benchmark_timeouts()still saidaiperf: the MLPerf client ran while the session graded it on an interactivity series MLPerf does not publish, so every round REVERTed.The fix is at the producer.
_export_operator_launch_shapeexports every pin under its own name, withholding none, so a pin and an operator's ownexportof the same name become the same thing. The local merge layer is gone and the three copies of the blob parser collapse into oneoperator_extra_env()inenv_safety; the blob itself stays, because it is what reachesstate.jsonand what names the previous launch's pins so a resume that drops one can unset it.Knobs Hyperloom resolves for itself then take the environment as one rung of their own ladder — explicit flag, then
$NAME, then the resumed session's value, then the default — which is the shape--max-model-len/$MAX_MODEL_LENalready had in_resolve_run_max_model_len_inner.argsbecomes the single answer and the projection writes it back, so there is no second writer to collide with._resolve_precisionsplits off from_resolve_workload_knobsbecause it reads the checkpoint and cannot run until the model is resolved, while the five numeric knobs must be settled earlier:_preflight'scheck_gpu_visibilitycompares$TPagainst the visible GPU count. The resulting fresh-branch order is launch-shape export, ladder, projection, topology gates,_preflight; the gates become_enforce_topology_gatesso the same two checks see the resolved shape. On the resume branch the pins are restored ahead ofagentx_state_is_stale, which resolves the AgentX backend from the environment and would otherwise refuse a session whose identity came from a pin.Linked issue(s): none
Tests:
test_cli_resume_launch_shape.pyadds twelve cases, each failing on the commit before the one that fixes it. A pin reaching the environment under its own name;agentx_client_scriptandseed_gradingagreeing once it does; a pin and anexportof the same name resolving identically; the ladder's four rungs including a non-positive value falling through; a full_run_optimizeresume of a session seeded with a pinned backend, which exits 2 before the guard fix; a resume dropping a pin unsetting it; theMAX_MODEL_LENrungs; and a table of ordering anchors asserted against_run_optimize's source — launch-shape export before the ladder, ladder before the projection, projection before_preflight, ladder before the gates. The anchor table is the one that pins the ordering defects: the behavioural case beside it passes on the broken revisions because it calls the functions in the order it wants.test_agentx_switch.py's_pin_extra_envnow pins the way the CLI does. Every test file touchingINFERENCE_OPTIMIZER_EXTRA_ENV/agentx_env_for_conc/ the pin parsers runs; remaining failures reproduce on the merge base and are pre-existing Windows-platform ones.Size/complexity triggers crossed: none
If this simplifies or refactors: removes the
{**os.environ, **_operator_extra_env()}merge inagentx_env_for_conc, two duplicate blob parsers, and — after a round where the pin was treated as a third source alongside the flag and the environment — the exclusion set, the export filter, the matching exclusion in the unset loop, the pin-reading int helper and a resume-onlyMAX_MODEL_LENresolver that existed only to keep those two apart. The contract preserved is thatbenchmark.envsstill carries every pin to the benchmark subprocess, covered bytest_agentx_switch.pyandtest_kernel_integrate_and_report.py's recipe case.Observable effect: a pin means the same thing in every reader and on both branches. Before:
--extra-env HYPERLOOM_AGENTIC_BACKEND=mlperfran the MLPerf client under aiperf grading and could not be resumed;--extra-env ISL=4096measured 4096 while recording 1024 in the comparability fingerprint and the manifest, then reported 4096 after a resume;--extra-env TP=4launched at TP=1 whilestate.jsonsaid 4;check_gpu_visibilitycould not warn about--tp 8on a 2-GPU box; a resume re-passingMAX_MODEL_LENwas silently ignored.Breaking changes: no. A pin that names a Hyperloom control variable now takes effect where it previously did not, and an explicit flag for the same knob outranks it; the docs state the ladder.
PR addresses single concern: yes
Root cause is upstream: no