Skip to content

fix: resolve framework version from an isolated framework venv - #1225

Merged
xiaofei-zheng merged 2 commits into
mainfrom
fix/stack-version-from-isolated-venv
Aug 19, 2026
Merged

fix: resolve framework version from an isolated framework venv#1225
xiaofei-zheng merged 2 commits into
mainfrom
fix/stack-version-from-isolated-venv

Conversation

@fengshaoyi-amd

@fengshaoyi-amd fengshaoyi-amd commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Problem

detect_stack_fingerprint resolves versions with importlib.metadata, which only sees the
interpreter running the orchestrator. --framework-env isolated is the default for vLLM —
its ROCm wheel pins its own torch, so it must not share that environment — which means the
framework is installed where this lookup cannot reach it. Every bare-metal vLLM run therefore
recorded the framework it actually served with as unknown:

- stack          : aiter unknown, rocm 7.2.4, sglang unknown, vllm unknown

The session manifest degraded the same way for a second, independent reason: build_manifest
already called build_provenance(), but discarded its stack_fingerprint and used a local
_detect_stack_fingerprint() that resolves versions by importing the package — an import that
cannot succeed under an isolated venv. Since cli/kb.py copies only non-unknown values into
stack_fingerprint_meta, and phases/explore.py fills the specialist framework_version from
that map, the fingerprint was missing from the KB row, the specialist prompt and resume — not
just from the report.

Approach

When the running interpreter has no such distribution, fall back to the venv root that setup
already records in .env (VLLM_VENV_ROOT) and read the distribution metadata from its
site-packages. Operator pins (VLLM_VERSION) keep priority, and the scan stays behind
probe=True so the hermetic contract that probe=False documents is unchanged.

Then source the manifest's stack_fingerprint from the shared builder and delete the local
detector, together with the _read_first_line helper and _STACK_FINGERPRINT_ENVS table that
only it used. common/provenance.py's module docstring already named the shared implementation
as the intended single source; that note is updated now that the duplication is gone.

Behaviour change

Unifying the two detectors changes one field beyond the bug: aiter was read from the imported
module's __commit__ and now comes from distribution metadata, so it records a version rather
than a commit sha when aiter is installed in the orchestrator's own interpreter. AITER_COMMIT
still takes priority, which is how that sha is normally supplied.

Test coverage

Four cases in test_common_provenance.py: a framework in its own venv is still versioned, an
operator pin still wins over the venv, a venv root that is not there is not a failure, and
probe=False does not touch the filesystem.

One case in test_manifest_unit.py covering the manifest itself: with only VLLM_VENV_ROOT set
and no vllm in the running interpreter, the written manifest carries the version rather than
unknown. Five tests covering the deleted helpers were removed, as were six monkeypatch seams
that patched a name that no longer exists.

Verified on a bare-metal vLLM host: vllm resolves to 0.27.1+rocm723 where it previously
read unknown.

`detect_stack_fingerprint` resolves versions with `importlib.metadata`,
which only sees the interpreter running the orchestrator.
`--framework-env isolated` is the default for vLLM -- its ROCm wheel pins
its own torch, so it must not share that environment -- which means the
framework is installed where this lookup cannot reach it. Every bare-metal
vLLM run therefore recorded the framework it actually served with as
`vllm unknown`.

Fall back to the venv root setup already records in `.env`
(`VLLM_VENV_ROOT`) and read the distribution metadata from its
site-packages. Env pins keep priority and the scan stays behind
`probe=True`, so the hermetic contract of `probe=False` is unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>
@fengshaoyi-amd
fengshaoyi-amd requested a review from a team as a code owner August 18, 2026 12:19
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown

CI E2E report — ❌ Timeout

item value
result ❌ Timeout
model Qwen/Qwen3-0.6B (dense)
resources 1× GPU, TP=1
PR branch fix/stack-version-from-isolated-venv
commit 9bc5a4747485960b241eefb9d8a6932f364070c4
session_id c260da37-adbd-496c-98f6-81d0d6978072
queue → dispatch 1s
run time 222m 9s
total 222m 10s
reason Timed out — the run never reached a terminal state in time (task stuck, or the GPU stayed queued too long).
detail not terminal after 13200s

details

…uilder

The previous commit fixed `detect_stack_fingerprint`, which the run report
reads, but not the session manifest -- and the manifest is the copy that
reaches the KB row, the specialist prompt and resume.

`build_manifest` already called `build_provenance()`, whose result carries a
`stack_fingerprint`, and then discarded that key in favour of a second,
local `_detect_stack_fingerprint()` that resolves versions by importing the
package. Under `--framework-env isolated` -- the default for vLLM -- that
import cannot succeed, so the manifest recorded `vllm unknown`. Downstream
`cli/kb.py` copies only non-`unknown` values into `stack_fingerprint_meta`
and `phases/explore.py` fills the specialist `framework_version` from that,
so the fingerprint stayed empty in all three consumers.

Take the field from the shared builder and delete the local detector, along
with the `_read_first_line` helper and `_STACK_FINGERPRINT_ENVS` table that
only it used. The shared implementation is the one the module docstring in
`common/provenance.py` already named as the intended single source, and it
resolves versions through `importlib.metadata` rather than importing vLLM
into the orchestrator on the session-start path.

One behaviour change beyond the bug: `aiter` was read from the imported
module's `__commit__` and now comes from distribution metadata, so it
records a version rather than a commit sha when aiter happens to be
installed in the orchestrator's own interpreter. `AITER_COMMIT` still takes
priority, which is how that sha is normally supplied.

Co-authored-by: Cursor <cursoragent@cursor.com>
@fengshaoyi-amd

Copy link
Copy Markdown
Contributor Author

You're right, and thanks — I traced the whole chain and it holds exactly as you described.

build_manifest computes _prov = build_provenance(args, env=os.environ) at line 419, and
build_provenance does return stack_fingerprint (provenance.py line 355). The manifest then
threw that key away and called the local _detect_stack_fingerprint(), which resolves versions
by importing the package — an import that cannot succeed under --framework-env isolated. So
the fixed value was being computed and discarded on exactly the path that matters.

The downstream consequence is the one you named: cli/kb.py line 363 copies only non-unknown
values into stack_fingerprint_meta, and phases/explore.py lines 962-969 fills the specialist
framework_version from that same map, also skipping unknown. The report line was fixed
because it comes from platform_probe.py line 243, which calls the shared detector directly —
so the first commit fixed the one consumer that did not go through the manifest.

Worth adding: common/provenance.py's own module docstring already flagged this duplication
("manifest.py still keeps its own _detect_stack_fingerprint ... so those detectors are
currently duplicated here"), so the shared implementation was always the intended single source.

What the follow-up commit does

Takes the first of your two options. build_manifest now writes
_prov.get("stack_fingerprint") or {}, and the local detector is gone along with the
_read_first_line helper and the _STACK_FINGERPRINT_ENVS table that only it used — 144 lines
deleted against 31 added. The docstring above is updated so it no longer describes a duplication
that has been removed.

.get() rather than a subscript is deliberate:
test_build_manifest_shared_provenance_fields monkeypatches build_provenance with a stub that
omits the key, so a subscript would KeyError there.

One behaviour change to flag

The two detectors were not equivalent beyond the venv fallback. The manifest's version read
aiter.__commit__ (a git sha) first; the shared one goes through importlib.metadata and
returns a distribution version. So when aiter happens to be installed in the orchestrator's own
interpreter, that field records a version instead of a sha. AITER_COMMIT still takes priority
and is how the sha is normally supplied, and on the isolated bare-metal layout aiter is not in
that interpreter at all (the field is unknown today either way) — but it is a real change and
unavoidable once the two are unified. Say the word if you'd rather preserve the sha and I'll
carry the commit lookup into the shared detector.

Tests

Added test_manifest_versions_a_framework_installed_in_its_own_venv, which is the case you
asked for: only VLLM_VENV_ROOT set, no VLLM_VERSION, no vllm in the running interpreter, and
the asserted value is the written manifest's, not the detector's return. I checked it is not
vacuous — the interpreter it runs under raises PackageNotFoundError for vllm, so the assertion
can only pass through the venv fallback.

Removed the five tests that covered the deleted helpers, and the six
monkeypatch.setattr(mf, "_detect_stack_fingerprint", ...) seams, which would now raise
AttributeError on a name that no longer exists.

Green locally: 65 in the three directly-touched files, plus 360 across the manifest/session/KB
consumers (test_manifest_unit, test_manifest_units, test_framework_switch_manifest,
test_session_layout, test_cli_kb_unit, test_coordinator_kb_writes,
test_local_kb_requirements, test_warm_replay, test_preflight_serving_framework,
test_build_lifecycle). ruff check clean on all three files. I also confirmed no reference to
any of the removed symbols survives anywhere in the repo.

@fengshaoyi-amd

Copy link
Copy Markdown
Contributor Author

Following up on the aiter question I raised above — I dug into it and I'm keeping the current
behaviour. Reasoning, and one correction to what I said earlier.

Correction first: I wrote that AITER_COMMIT "is how that sha is normally supplied". That
overstated it. Nothing in the repo sets AITER_COMMIT — it appears only in the env-var priority
table and in tests, and no install/setup path writes it. So it is an available override, not a
mechanism anyone is currently using.

Why keep the shared implementation anyway:

  1. This unifies an inconsistency rather than introducing one. platform_probe.py line 243 has
    been calling the shared detector all along, so the run report has always shown aiter as a
    distribution version. The sha only ever existed on the manifest side — the two consumers
    already disagreed, and this is what makes them agree.

  2. Preserving the sha means import aiter, which is precisely what the shared detector avoids
    and says why: the import is heavy (seconds, may touch the GPU/driver, can trigger JIT module
    loads) and this code runs on the session-start path. Paying that on every run to sharpen one
    field in one deployment shape is the wrong trade, and it would contradict the design the
    module documents.

  3. The affected surface is narrower than it sounds. On the isolated bare-metal layout aiter is
    not in the orchestrator's interpreter at all, so the field reads unknown before and after.
    Only a shared-environment install where aiter is importable changes, and it changes from a
    sha to a coarser version rather than losing the field.

The follow-up I'd suggest, separately: have setup record AITER_COMMIT in .env, exactly
the way it already records VLLM_VENV_ROOT — same pattern, same reason ("what the orchestrator
cannot see, setup writes down"). That restores sha-level precision for every layout without
putting an import on the session-start path, and it belongs in the setup path rather than in
this fix. Happy to open an issue for it, or to fold it in here if you'd rather it not wait.

@xiaofei-zheng
xiaofei-zheng merged commit 8a2fea5 into main Aug 19, 2026
26 of 28 checks passed
@xiaofei-zheng
xiaofei-zheng deleted the fix/stack-version-from-isolated-venv branch August 19, 2026 09:40
xiaofei-zheng added a commit that referenced this pull request Aug 25, 2026
…-venv

fix: resolve framework version from an isolated framework venv
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