Skip to content

fix(cli): let a resumed session's recorded workload outrank the shell - #1818

Open
xiaofei-zheng wants to merge 2 commits into
mainfrom
feature/xiaofei/resume-workload-authority
Open

xiaofei-zheng wants to merge 2 commits into
mainfrom
feature/xiaofei/resume-workload-authority

Conversation

@xiaofei-zheng

@xiaofei-zheng xiaofei-zheng commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator
  • Description: fix(cli): export every --extra-env pin so all readers resolve it alike #1797 gave ISL/OSL/CONC/TP/EP/PRECISION an environment rung — flag, $NAME, resumed state, default — which also gave a stale shell export one. Hyperloom writes export TP/CONC/ISL/OSL into the repro scripts it generates (reference_script.py:420-425, orchestrator/kernel/request_handlers.py:999-1003) and examples/hyperloom-custom-advanced/SKILL.md:301-304 has operators export the same names, so sourcing one and then running --resume-from in that shell re-measured the session at the shell's shape, with canonical_fingerprint.py:117-121 and session/manifest.py:218-247 recording those numbers as the session's. Reproduced on the merge base: a shell at TP=4 CONC=256 ISL=4096 against a session recorded at tp=8 conc=99 isl=3333 resolved to the shell's values.

    _seed_env_from_resumed_state puts the recorded workload into the environment before the ladder reads it, skipping any name this resume re-pins. The recorded workload therefore outranks the shell for everything resolved from the ladder, and changing it stays an explicit act — a flag, or --extra-env NAME=VALUE passed again on the resume. The fresh branch is unchanged. One reader is deliberately outside this: _preflight runs before the seeding, so check_gpu_visibility still compares $TP from the shell. That is pre-existing — the resume branch has never projected TP before preflight — and it only shapes a warning, so it is left for its own change.

    The reference docs also still stated the pre-fix(cli): export every --extra-env pin so all readers resolve it alike #1797 behaviour in two places that PR did not touch: environment-variables.md called these names "ignored and overwritten" and multi-node.md said "env vars are not" authoritative. Both now state the ladder. --gpu-type stays on the flag-only row, since GPU_TYPE remains a fallback rather than a rung.

  • Linked issue(s): follows fix(cli): export every --extra-env pin so all readers resolve it alike #1797

  • Tests: test_cli_resume_launch_shape.py adds a case driving _seed_env_from_resumed_state and _resolve_workload_knobs in the resume branch's order — a shell carrying TP=4 CONC=256 ISL=4096, a session recorded at tp=8 isl=3333, and --extra-env CONC=16 re-pinned — asserting the recorded values survive while the re-pinned one changes. Without the seeding the shell's TP=4 wins and the case fails. test_cli_resume_launch_shape.py and test_cli_workload_envs.py pass (47).

  • Size/complexity triggers crossed: none

  • If this simplifies or refactors: n/a

  • Observable effect: resuming a session from a shell that has TP/CONC/ISL/OSL left in it — which is what sourcing a generated repro script does — now continues at the workload the session was measured at, instead of silently switching to the shell's and recording that in the fingerprint and the manifest. Re-passing --extra-env still changes it.

  • Breaking changes: no. It restores the pre-fix(cli): export every --extra-env pin so all readers resolve it alike #1797 outcome for a resume whose shell carries these names, while keeping the rung fix(cli): export every --extra-env pin so all readers resolve it alike #1797 added for a fresh launch and for an explicitly re-passed pin.

  • PR addresses single concern: yes

  • Root cause is upstream: no

Giving the knobs an environment rung also gave a stale export one. Hyperloom
writes export TP/CONC/ISL/OSL into the repro scripts it generates and the
custom-workload SKILL has operators export the same names, so sourcing one
and then resuming re-measured the session at the shell's shape and recorded
those numbers in the fingerprint and the manifest. Reproduced: shell TP=4
against a session recorded at TP=8 resolved to 4.

_seed_env_from_resumed_state puts the recorded values in the environment
before the ladder reads it, skipping any name this resume re-pins, so the
workload only changes through a flag or a re-passed --extra-env.

The reference docs said the opposite of the new behaviour in two places the
PR had not touched -- environment-variables.md called these names "ignored
and overwritten", multi-node.md "env vars are not" authoritative. Both now
state the ladder, with --gpu-type kept on the flag-only row.
@xiaofei-zheng
xiaofei-zheng requested a review from a team as a code owner October 11, 2026 02:57
…ationale

Self-review with the rules in #1817 found two things this PR had missed.

T5: the case covering the seeding calls it and the ladder in the order it
wants, so it passes with the two swapped in _run_optimize -- verified by
swapping them, where the behavioural case still passed and the defect was
live. The ordering anchor table gains the pair, resolved against the resume
branch's own ladder call rather than the fresh one, and fails on the swap.

X3: a tree-wide sweep for the claim this PR's ladder contradicts found one
more copy -- the Qwen skill told operators that "CLI defaults can otherwise
override the intended workload", which stopped being true when the default
became the bottom rung. The advice stands; the reason is now the real one.
Two further copies state the advice without a rationale and are left alone.
@jiaqiang-dot-liu

Copy link
Copy Markdown
Collaborator

PR #1818 -- fix(cli): let a resumed session's recorded workload outrank the shell

What it does: #1797 gave ISL/OSL/CONC/TP/EP/PRECISION an environment rung, which also
gave a stale shell export one -- so sourcing a Hyperloom-generated repro script (which does
export TP/CONC/ISL/OSL) and then running --resume-from in that shell silently re-measured
the session at the shell's shape and recorded those numbers as its own. The PR adds
_seed_env_from_resumed_state, called on the resume branch between _emit_launch_info and the
ladder, which writes the session's recorded workload into the environment for every name this
resume does not pin. The recorded workload therefore wins the ladder, while an explicit flag or a
re-passed --extra-env NAME=VALUE still changes it; the fresh branch is untouched.

Blocking issues: 2

  1. [X3] The two reference docs this PR rewrites state the precedence the code inverts [verified]
    Problem: both new paragraphs rank the environment ABOVE the resumed session's recorded value --
    docs/reference/environment-variables.md:80 ("an explicit flag, then the environment, then the
    resumed session's recorded value, then the default ... so a pre-set env var ... is used when the
    flag is absent") and docs/reference/multi-node.md:154 ("the corresponding env var is used,
    then the resumed session's value"). On the resume branch -- the only branch this diff changes --
    a pre-set env var now loses: src/hyperloom/inference_optimizer/cli/__init__.py:1172-1175
    overwrites os.environ[NAME] with the recorded value before _positive_env_int reads it at
    :1189. The added docstring at :1166 says the opposite of the paragraph shipped with it.
    docs/how-to/optimize-custom-workload.md:143 carries the third copy, "A pin and an export of
    the same name are equivalent: no name is withheld", which this diff makes false for the seven
    seeded names: a pin is skipped at :1174, a bare export is overwritten.
    Impact: the operator-facing reference for exactly this knob set now tells an operator that a
    shell export ISL=4096 is honoured on a resume. It is not -- the run continues at the recorded
    ISL -- so the doc leads to the same wrong belief about the workload the PR exists to prevent.
    Action: state the resume ordering in all three copies -- flag, then --extra-env pin, then the
    resumed session's recorded value, then the shell environment, then the default -- and drop or
    re-scope "a value left in the shell by a generated repro script is indistinguishable from one
    you meant", which is the pre-fix warning.

  2. [free:skip-set-is-the-persisted-pins] The new docstring and the description overstate the
    invariant: the skip set is the previous launch's pins, not this resume's [verified]
    Problem: _seed_env_from_resumed_state skips any name in pins
    (src/hyperloom/inference_optimizer/cli/__init__.py:1173), and its docstring at :1169-1170
    calls that "a name this resume re-pins". pins is _resume_extra_env, which at :1865 is
    parse_operator_extra_env(args) or dict(state.operator_extra_env) -- the persisted dict
    whenever no --extra-env is passed on the resume. A launch of --isl 2048 --extra-env ISL=4096
    records state.isl=2048 and operator_extra_env={"ISL": "4096"} (cli/bootstrap.py:391), so a
    bare --resume-from re-exports ISL=4096 at :1293, skips ISL in the seeding, and resolves
    4096. Reproduced with the real function bodies. The behaviour is pre-existing -- base 20d4309
    resolves 4096 for the same case -- but the body's "changing it stays an explicit act -- a flag,
    or --extra-env NAME=VALUE passed again on the resume" and the docstring are both added here
    and are false for it: nothing was re-passed.
    Impact: the one case where a pin and the record disagree is the case the PR's own description
    says cannot happen, so a reader checking whether a session resumed at its measured shape trusts
    a guarantee the code does not give, and the session silently re-measures at the pin.
    Action: either narrow the skip set to parse_operator_extra_env(args) so only a pin re-passed
    on this resume is honoured, or correct the docstring and the description to say the skip set is
    this resume's pins or, failing that, the launch's persisted ones.

Checked: src/hyperloom/inference_optimizer/cli/init.py (_seed_env_from_resumed_state,
_resolve_workload_knobs, _resolve_precision, _export_operator_launch_shape, _run_optimize's resume
branch and its _is_resume guards), cli/bootstrap.py (_seed_shared_state, agentx_state_is_stale,
parse_operator_extra_env), cli/preflight.py (_pin_resumed_session_args), orchestrator/state/
shared_state.py:349-360, canonical_fingerprint.py:114-124, session/manifest.py:245,
reference_script.py:415-428, orchestrator/kernel/request_handlers.py:995-1006,
examples/hyperloom-custom-advanced/SKILL.md:298-306, docs/reference/environment-variables.md,
docs/reference/multi-node.md, docs/how-to/optimize-custom-workload.md:140-152,
tests/test_cli_resume_launch_shape.py | Ran: the real _positive_env_int /
_seed_env_from_resumed_state / _resolve_workload_knobs bodies over six resume cases -- shell export
without the seeding (base), with it, with --extra-env CONC=16 re-pinned, with an explicit --tp
flag, with an unrecorded state knob, and with a pin persisted from the original launch; pytest not
run (hyperloom will not import here: cli/kb.py imports fcntl), CI's 12 shards and both coverage
jobs are green at this head | Base: 20d4309 | Head:
39ab9c3

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants