Skip to content

fix(bench): stop baseline benchmarking from a private InferenceX copy - #1456

Merged
ZhengGong-amd merged 1 commit into
mainfrom
fix/shafeng/inferencex-mirror
Sep 17, 2026
Merged

ZhengGong-amd merged 1 commit into
mainfrom
fix/shafeng/inferencex-mirror

Conversation

@fengshaoyi-amd

@fengshaoyi-amd fengshaoyi-amd commented Sep 9, 2026

Copy link
Copy Markdown
Contributor
  • Description: what and why

    materialize_config_with_envs owns benchmark.inferencex_path for all three of its
    callers, but only baseline passed the argument. It handed in a copy of the checkout
    it had made on local disk; explore and integrate_patch fell through to
    $INFERENCEX_PATH. In the reported session that is 87 of 109 materialized configs
    naming the network checkout and 22 naming a mirror nobody else could see, interleaved
    inside a single coordinator process — 11:10 baseline local, 11:39 integrate_patch NFS,
    11:47 baseline local. Which tree a round benchmarks from had stopped being a property
    of the session and become a property of the executor.

    This deletes the copy, rather than teaching the other two callers to make one.

    An exported $INFERENCEX_PATH is the operator's tree. They may edit it between
    rounds, and a copy taken once would go on serving the contents it had then without
    ever saying so. That is the contract session.paths.resolve_dep_dir already gives
    Magpie and TraceLens: an exported path wins verbatim, anything else resolves under the
    cache root. baseline was the one caller that did not honour it.

    It also would not have helped. The deployment that produced the ESTALE failure exports
    INFERENCEX_PATH from Hyperloom-dev/.env, so under that contract nothing downstream
    may relocate that tree — a session-level relocation would never have fired for the
    session the report came from.

  • What this costs, and why it is still the right shape

    Every round now reads whatever $INFERENCEX_PATH names. Where that is a revocable
    mount, the baseline rounds lose the protection they had, so the exposure goes from 87
    of 109 configs to all of them until the two fixes below land.

    So preflight records it instead of working around it. A checkout on a network
    filesystem warns once and lands in the install ledger as network_fs. That is what
    tells the next magpie_nonzero_after_valid_measurement apart from a variant that
    genuinely cannot serve — last time it was diagnosable only because the flap happened
    to kill the shell:

    Requesting API: 100%| 1319/1319 [21:22<00:00,  1.03it/s]
    HYPERLOOM_EVAL_BOUNDS_SUMMARY {... "generations": 1319 ...}
    + local eval_exit=0
    benchmarks/vllm_mi355x.sh: error reading input file: Stale file handle
    

    Removing the mirror removes the only reader of
    INFERENCE_OPTIMIZER_DISABLE_LOCAL_INFERENCEX and
    INFERENCE_OPTIMIZER_LOCAL_INFERENCEX_ROOT. Both were undocumented; they are now gone
    rather than newly documented.

  • The two changes that do close the exposure — neither belongs here

    Both are path resolution, not executor behaviour, and both are follow-ups:

    1. Hyperloom-dev/.env pins INFERENCEX_PATH at
      /shared_nfs/zgong/Hyperloom/.cache/InferenceX@3d55815…, a checkout under a
      different Hyperloom clone. Hyperloom-dev/.cache/InferenceX@3d55815… exists at
      the same SHA, so the pin buys nothing while making every session in the workspace
      depend on a path another clone's installer may re-clone underneath it. Deleting the
      line is a deployment change.
    2. deps_cache_root() defaults to $REPO_ROOT/.cache, so Hyperloom's own dependency
      checkouts follow the repo onto whatever filesystem it lives on. Putting that root
      on local disk is what makes "the resolved checkout is on a network mount"
      impossible rather than merely reported — and it applies to GEAK and TraceLens too,
      so it wants its own PR and a decision about multi-node sharing.
  • Linked issue(s)

    Follows 4 June-9 June run analysis #523 (the cuda-graph dump that motivated the original baseline-only mirror)
    and Profiling Fix: InferenceX in HyperLoom #210 (the MAGPIE_INFERENCEX_PATH pin). Filed from an internal run analysis of
    GLM-5.3-Quark-MXFP4-AttnFP8_20260908T111000Z_3e95db1e.

  • Tests: added/updated? commands run?

    Updated. The 7 tests covering _ensure_local_inferencex go with it. One added, in the
    same source-inspection style as its neighbours, for the ledger field that replaces it.

    pytest src/hyperloom/inference_optimizer/tests/test_inferencex_preflight_clone.py
    pytest src/hyperloom/inference_optimizer/tests/test_baseline_warmup_double_run.py
    pytest src/hyperloom/inference_optimizer/tests/test_baseline_param_overrides.py
    
  • Breaking changes: no

    Behavioural change for a deployment whose $INFERENCEX_PATH is on a network mount:
    baseline rounds now run from it directly, with a warning, as the other executors
    already did.

  • PR addresses single concern: yes

    Which InferenceX checkout a round benchmarks from is the session's to decide, not the
    executor's.

  • Root cause is upstream: no

    Hyperloom-side path resolution. See the two follow-ups above.

@fengshaoyi-amd
fengshaoyi-amd requested a review from a team as a code owner September 9, 2026 08:45
@fengshaoyi-amd
fengshaoyi-amd force-pushed the fix/shafeng/inferencex-mirror branch 2 times, most recently from 45ce86f to 25ece24 Compare September 9, 2026 09:11
@fengshaoyi-amd
fengshaoyi-amd force-pushed the fix/shafeng/inferencex-mirror branch 3 times, most recently from 9fe9cca to 722fdc3 Compare September 10, 2026 10:11
@ZhengGong-amd

Copy link
Copy Markdown
Collaborator

The diagnosis is right and the baseline-only mirror clearly has to go. What is blocking is the shape: relocation is modelled as a per-task operation with its own cross-process garbage collector, when it is a session-level path resolution that this repo already has a single owner for.

Blocking

B1 — Make relocation happen once, at cli/preflight.py:2872, before os.environ["INFERENCEX_PATH"] = inferencex_path.

Preflight already resolves, validates writability, and pins the checkout. Relocate there and the process has one truth value, so env and config can never disagree. That deletes, not defers, most of this PR:

  • the config-first rework (_run_magpie, baseline's read-back, inferencex_root_from_config) — env and config name the same tree again;
  • the leak-root extension in benchmark_result.py$INFERENCEX_PATH is already a leak root;
  • mirror_key, .owner, _sweep_orphaned_buckets, _bucket_owner_alive, the shared bucket — one mirror per session, named by the session hash, removed by one rmtree in Coordinator.stop();
  • phases/kernel.py:1124 (GEAK), profile.py:869, bypass_engine.resolve_inferencex_root, manifest, and the specialist prompt become correct with no edit.

What is left is ensure_local_inferencex itself: copy once, verify, keep the source path on failure.

Please also record inferencex_source_path in the session manifest alongside the relocated one, so an archived config still points at something traceable after the mirror is gone.

B2 — Keying on output_dir is not a cache; it is a full copy per materialize call. Your own session data is 109 materialized configs, i.e. ~109 InferenceX copies reclaimed only at Coordinator.stop(). The stated reason for the per-task key — "the per-task patch step rewrites the mirror in place" — does not hold: everything written into the checkout comes from _inferencex_patcher, whose first docstring line is "Idempotent, backward-compatible patchers". If concurrent rounds really do corrupt a shared tree (Magpie staging scripts), that is today's behaviour on the shared $INFERENCEX_PATH and belongs in its own PR, not smuggled in as per-task copies. B1 resolves this.

B3 — Cross-host sweep deletes a live benchmark's cwd. _bucket_owner_alive uses os.kill(pid, 0) and /proc/<pid>/stat, which are host-local, but the buckets live under $HOME, shared across nodes on these clusters. A coordinator alive on another node has no pid here, so running() is False and the bucket is rmtreed while it is being benched — the exact failure the module docstring says it exists to prevent. Under B1 the sweep goes away; if you keep any sweep, the owner marker must carry os.uname().nodename and only same-host buckets may be swept.

B4 — The destination is never checked to be local disk. mirror_root() hardcodes ~/.cache/... and never asks whether that is itself a network mount. On a host with $HOME on wekafs this copies the whole tree every round and still dies on ESTALE. is_network_fs is already imported: if the destination is on a network filesystem, warn and return src unchanged.

B5 — Delete the except (OSError, yaml.YAMLError) fallback in inferencex_root_from_config. The caller just wrote that YAML; failing to read it is not a legal state. After this PR env and config can name different trees, so that fallback silently recreates the very asymmetry the PR removes — Magpie on the source while the patches land on the mirror. Let it raise. (Under B1 the function is gone entirely; if any config-side resolver survives, reuse bypass_engine.resolve_inferencex_root rather than adding a fourth copy of the same rule.)

B6 — Degradations are invisible where it counts. Every fallback is a log.warning only. The bug you are fixing was diagnosable because it killed the shell; after a failed mirror the next ESTALE still lands as magpie_nonzero_after_valid_measurement. Record the checkout a round actually executed, and whether it was relocated, in the round result / ledger.

Non-blocking

  • Validate benchmarks/benchmark_lib.sh on the source before copying, or drop the check — cli/preflight.py:2851 already validates the checkout. Copying a whole tree to then reject it is the most expensive possible ordering.
  • _claim_bucket rewrites .owner on every call with constant content; write it at creation only.
  • docs/reference/environment-variables.md has neither INFERENCE_OPTIMIZER_DISABLE_LOCAL_INFERENCEX nor INFERENCE_OPTIMIZER_LOCAL_INFERENCEX_ROOT. This PR widens both from baseline to every executor and adds a new on-disk root; document them.
  • The module docstring cites #1332, which is not among the issues in the description. Check the number.
  • Tests worth adding once B1 lands: destination itself on a network mount, and the Coordinator.stop() ordering invariant (cleanup only after in-flight cancellations are awaited) — currently asserted by a comment alone.

@fengshaoyi-amd
fengshaoyi-amd force-pushed the fix/shafeng/inferencex-mirror branch from 722fdc3 to 244a439 Compare September 11, 2026 03:27
@fengshaoyi-amd fengshaoyi-amd changed the title fix(bench): every executor benches from a local InferenceX mirror fix(bench): resolve the InferenceX checkout onto local disk once, in preflight Sep 11, 2026
@fengshaoyi-amd
fengshaoyi-amd force-pushed the fix/shafeng/inferencex-mirror branch from 2af5eea to ac08519 Compare September 11, 2026 08:47
`materialize_config_with_envs` owns `benchmark.inferencex_path` for all three
callers, but only `baseline` passed the argument: it handed in a copy of the
checkout made on local disk, while `explore` and `integrate_patch` fell through
to `$INFERENCEX_PATH`. In the reported session that is 87 of 109 materialized
configs naming the network checkout and 22 naming a mirror nobody else could
see, interleaved inside one coordinator process. Which tree a round benchmarks
from stopped being a property of the session and became a property of the
executor.

Deleting the copy is what makes the three agree, rather than teaching the other
two to copy as well. An exported `$INFERENCEX_PATH` is the operator's tree: they
may edit it between rounds, and a copy taken once would go on serving the
contents it had then without ever saying so -- the contract
`session.paths.resolve_dep_dir` already gives Magpie and TraceLens. The
deployment that produced the ESTALE failure exports that variable from
`Hyperloom-dev/.env`, so no amount of relocation downstream was ever going to
fire for it.

That widens the exposure rather than narrowing it: every round now reads
whatever `$INFERENCEX_PATH` names, and where that is a revocable mount the
baseline rounds lose the protection they had. So preflight records it. A
checkout on a network filesystem warns once and lands in the install ledger as
`network_fs`, which is what tells the next
`magpie_nonzero_after_valid_measurement` apart from a variant that genuinely
cannot serve -- the failure was diagnosable last time only because it killed the
shell.

The two fixes that do close it are both path resolution, not executor
behaviour, and neither belongs here: the `.env` pin names another clone's cache
at the same SHA and buys nothing, and `deps_cache_root()` defaults to
`$REPO_ROOT/.cache`, which follows the repo onto the mount.

Removing the mirror removes the only reader of
`INFERENCE_OPTIMIZER_DISABLE_LOCAL_INFERENCEX` and
`INFERENCE_OPTIMIZER_LOCAL_INFERENCEX_ROOT`; both were undocumented and are now
gone.

Co-authored-by: Cursor <cursoragent@cursor.com>
@fengshaoyi-amd
fengshaoyi-amd force-pushed the fix/shafeng/inferencex-mirror branch from ac08519 to 519dd06 Compare September 11, 2026 09:12
@fengshaoyi-amd fengshaoyi-amd changed the title fix(bench): resolve the InferenceX checkout onto local disk once, in preflight fix(bench): stop baseline benchmarking from a private InferenceX copy Sep 11, 2026
@fengshaoyi-amd

Copy link
Copy Markdown
Contributor Author

Pushed a revision that goes the other way on B1, so it needs an explanation rather than just a re-read.

B1 landed as asked: relocation moved to cli/preflight.py, once, immediately before os.environ["INFERENCEX_PATH"] = inferencex_path, and the per-task bucket, the owner marker and the sweeper went with it.

Then a second rule landed on top: an exported dependency path is used verbatim and is never copied, because the operator may edit that tree between rounds — the contract session.paths.resolve_dep_dir already gives Magpie and TraceLens. That rule removed B1's ground. The deployment that produced the ESTALE failure exports INFERENCEX_PATH from Hyperloom-dev/.env, so under it nothing may relocate that tree: a session-level relocation would never have fired for the session the report came from. The remaining case — Hyperloom resolves a checkout for itself and the cache root is on a revocable mount — has not been observed. The mechanism had no live caller.

So this revision deletes it and keeps only what the original report asked for: baseline stops passing its own copy into materialize_config_with_envs, so all three callers name the same tree. +22 / −282.

Against your points:

  • B1 superseded — there is no relocation to place.
  • B2 resolved by deletion; nothing copies per materialize call any more.
  • B3 the sweep is gone with the module.
  • B4 moot; there is no destination.
  • B5 the config-first rework was reverted, so BaselineExecutor._inferencex_root_from_config is unchanged from main, except (OSError, yaml.YAMLError) included. Env and config can no longer name different trees, which was the asymmetry that made the fallback dangerous, but the fallback itself is untouched here — say the word if you want it in this PR rather than a follow-up.
  • B6 addressed without the mechanism. A checkout on a network filesystem warns once and lands in the install-event ledger as network_fs, which is what tells the next magpie_nonzero_after_valid_measurement apart from a variant that genuinely cannot serve.

The non-blocking points are all moot with the module gone, except the two undocumented env vars: INFERENCE_OPTIMIZER_DISABLE_LOCAL_INFERENCEX and INFERENCE_OPTIMIZER_LOCAL_INFERENCEX_ROOT lose their only reader, so they are deleted rather than newly documented.

One thing worth pushing back on before approving. This widens the exposure rather than narrowing it: the baseline rounds lose the protection they had, so it goes from 87 of 109 configs on the mount to all of them. The description says so, and names the two changes that actually close it — dropping the .env pin, which points at another clone's cache at the same SHA and buys nothing, and moving deps_cache_root() off $REPO_ROOT/.cache so Hyperloom's own checkouts stop following the repo onto the mount. The second one reaches GEAK and TraceLens and needs a decision about multi-node cache sharing, so it wants its own PR. If you would rather this PR wait for that one instead of landing ahead of it, that is a reasonable call.

@ZhengGong-amd
ZhengGong-amd merged commit bbc2e04 into main Sep 17, 2026
32 checks passed
@ZhengGong-amd
ZhengGong-amd deleted the fix/shafeng/inferencex-mirror branch September 17, 2026 03:51
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