Hnimrama/interactivity graph

[hnimra-amd]

InferenceX interactivity viewer and run provenance

Base branch: dev/dtni
Head branch: hnimrama/interactivity_graph

Summary

Adds a SemiAnalysis-style interactivity chart to the inference interactive HTML viewer and improves run deck reproducibility metadata (git ref, container image, launch commands). Render-only reporting changes.

Interactivity chart (interactive viewer)

• Token Throughput per GPU vs. Interactivity (one line per ISL/OSL shape; points by concurrency).
• Interactivity = 1000 / mean TPOT (tok/s/user), aligned with InferenceX.
• Y-axis: total token throughput per GPU.
• Hover detail card (click point to pin): date, image, throughput split, TP/GPUs, concurrency, precision, CI/upstream link.
• Zoom/pan: scroll zoom, shift+drag pan, drag box-zoom, reset / double-click.
• Layout: Filters → Overview → Interactivity → other sweep panels.

Run deck & provenance

• Git ref, container image display, image digest at launch, IX launch command provenance.
• Run deck surfaces reproducibility fields; bundle copy skip when artifact already present.

Test plan

• pytest cvs/lib/report/unittests/test_viewer_scaffold.py cvs/lib/report/unittests/test_provenance.py cvs/lib/report/unittests/test_inference_report.py
• pytest cvs/lib/inference/unittests/test_inferencex_atom_launch.py cvs/core/orchestrators/unittests/test_container.py
• Open *_viewer.html from a sweep report: interactivity chart, pin card, links, filters.

Sample reports

• vllm_run_deck.html
• vllm_run_deck.json
• vllm_run_deck_summary.html
• vllm_run_deck_viewer.html

[solaiys]

Review (head 3af5952f, base dev/dtni)

Regular review (blocking issues only) plus the requested analysis of the CVS core-file changes.

Blocking issues

• 🔴 str | None annotation breaks import on Python 3.9 (a declared-supported version) — cvs/core/orchestrators/container.py:549

  container.py has no from __future__ import annotations, so the new def capture_image_provenance(self, container_name: str | None = None) annotation is evaluated eagerly at import time. PEP 604 X | None unions only work at runtime on Python 3.10+, but setup.py declares python_requires=">=3.9"; on 3.9 this raises TypeError while importing container.py. Since ContainerOrchestrator is core and imported by every container use case (training and inference), this breaks all of them on 3.9 — not just the new inference feature.

  Fix: add from __future__ import annotations at the top of container.py (consistent with provenance.py, which already does this), or use Optional[str] from typing. (If 3.9 is no longer actually supported, bump python_requires instead — separate decision.)

CVS core-file changes: inference-specific or all use cases?

• cvs/core/orchestrators/container.py — functionally inference-specific, but with repo-wide side effects.

  • The new capture_image_provenance() (+ _ssh_command_output helper) is called only from cvs/lib/inference/utils/inference_suite_lifecycle.py (test_launch_container), wrapped in try/except so a failure only warns. As a feature it serves the inference run-deck provenance use case exclusively; it is additive and doesn't alter the existing container lifecycle.
  • However, two things now apply to all container use cases regardless of inference:
    1. The module-level from cvs.lib.report.provenance import format_image_display loads whenever ContainerOrchestrator is imported. This is not a circular import (provenance.py imports only subprocess/typing), so it's safe — but it introduces a cvs.core → cvs.lib.report layering dependency (a core orchestrator depending on the reporting layer). Non-blocking; consider a lazy import inside the method.
    2. The str | None annotation above — the blocking issue, which affects all container use cases on 3.9.

• cvs/lib/report_plugins.py — applies to ALL use cases (shared HtmlReportManager), and is a net improvement.

  • add_html_to_report now .resolve()s paths and skips the copy when source == dest (the "bundle copy skip when artifact already present") — safe and beneficial for every report-producing suite.
  • generate_reports_section renames the local html string var to reports_html, fixing a latent bug: the old code shadowed the imported html module, so html.escape(...) would raise AttributeError whenever any suite registered a custom test report. General bug fix, not inference-specific.

So the report_plugins.py change is genuinely cross-cutting (and fixes a real bug for all suites); the container.py method is inference-only in purpose, but its import and the str | None annotation land on every container use case.

Reviewed 1 PR: 1 with a blocking issue.

[amd-droy]

looks good to me. Thanks Humna.

[atnair-amd]

is this really needed? couldnt you do orch.exec commands for this?

[atnair-amd]

should not be bundled in orch

[atnair-amd]

duplication. differs by one fn

[atnair-amd]

confused why _first_sweep_coords is called here just for example_cell. i think build_launch_provenance, server_command, bench_command could be refactored to be cleaner.

Unless there is a need for _first_sweep_coords to be this way (i could've missed something)

[atnair-amd]

why is this needed for the interactivity graph?

[atnair-amd]

Second-pass review at c7cfc65c. Each finding was independently re-verified by attempting to refute it; two initial findings were dropped when the refutation succeeded, and I've noted those below so the negative results are on record too.

The earlier review's two points (py3.9 str | None, core→report layering) are both resolved at this commit — extracting cvs/core/image_display.py is a cleaner fix than the lazy import that was suggested.

Two correctness bugs (inline on interactive.html and inferencex_atom_launch.py), and make ut is currently red with the tests that would have guarded both of them not running at all.

Checked and found not to be problems:

• Locally-built images with no RepoDigests do not render docker's error text as the digest — docker exits 1 there, and the existing exit_code guard catches it. Verified against a live daemon.
• _example_job(orch=SimpleNamespace()) is safe, and is the right call — no argv builder touches orch (bytecode-checked), and SimpleNamespace raises loudly rather than silently stringifying a stub.

Also worth noting as a genuine win: the report_plugins.py rename of the local html var to reports_html fixes a real latent bug — the old code shadowed the html module, so html.escape(...) would AttributeError for any suite registering a custom test report.

[atnair-amd]

🔴 Per-GPU throughput ignores pipeline parallelism.

outputPerGpuThroughput() reads client.output_tput_per_gpu, which only ATOM emits (inferencex_atom_parsing.py:73). vLLM never sets it — I confirmed by executing to_client_metrics across a tp/pp grid rather than grepping — so for vLLM the /tp fallback always fires, overstating by exactly a factor of PP.

On the shipped mi300x_vllm_llama31-70b_fp8_distributed.json (TP=8 at :59, PP=2 at :60), total 4000 / output 2000 tok/s:

Row	Shown	Correct
Total GPUs	16	16 ✓
Token Throughput per GPU	250	250 ✓
Output Token Throughput per GPU	250	125
Input Token Throughput per GPU	0	125

The same tooltip prints Total GPUs: 16 and Pipeline Parallelism: 2 (:741), and :725 already computes tp * pp — so PP is available right there; these rows just don't use it.

Note the chart's Y value is not affected: perGpuThroughput() (:895-897) hits the PP-aware direct metric first, so the plot is correct and only the tooltip is wrong. That makes this easy to ship unnoticed.

Suggested fix, in both this function and inputPerGpuThroughput:

const gpus = tp * (ppFromCellId(cell.cell_id) || 1);
return tput / gpus;

[atnair-amd]

Follow-on from the outputPerGpuThroughput comment above: this mixes a PP-aware value (totalPg, from the direct client.per_gpu_throughput metric, computed as total/(tp*pp)) with a PP-blind one (outputPg, from the /tp fallback).

For the shipped TP=8/PP=2 config that yields 250 - 250 = 0. With an OSL-heavy shape (total 2200 / output 2000) it renders −112.5 — a negative throughput on screen.

Fixing outputPerGpuThroughput to divide by tp * pp fixes this row too, since the subtraction then operates on two consistent quantities.

[atnair-amd]

🔴 This fallback fabricates a sweep point and it reaches the published report.

The ("", "1024", "1024", "128") tuple is undocumented — no constant name, no comment — and to a run-deck reader is indistinguishable from a real swept point.

The miss branch is reachable from a config that loads and runs correctly. inferencex_atom_config_loader.py:86 builds the cell key by interpolating isl / osl / tensor_parallelism, all declared as bare str with no digit constraint, while _CELL_RE here demands \d+ in all three positions — the regex is stricter than the schema.

Concretely: params.tensor_parallelism: "8x2" is a plausible label for a 2-node deployment, and on driver: atom that field is labeling only (real TP comes from roles.server.atom_args). Such a config loads under enforce_thresholds: true, runs ISL=7168 @ conc 128 — and the deck publishes ISL=1024 @ conc 128, with a --result-dir .../isl1024_osl1024_conc128 naming a directory that never existed on the node. Zero warnings, no log line.

Two related notes:

• sweep.runs has no min_length (inferencing_config_loader.py:141), so "runs": [] loads fine and still emits all three launch keys.
• The NNODES= alternative in this regex is dead — no generator anywhere emits it. The only two are ATOM :86 (never PP) and vllm_config_loader.py:228-231 (PP only when pp>1).

Segment order is also fragile — PP=2,TP=8 (swapped), DP=, and EP= all miss.

The durable fix is to stop re-deriving coordinates from a stringified key and read sweep.runs directly, the same source cell_key uses.

[atnair-amd]

This is why omitting launch_example_cell doesn't contain the fabricated coordinates: example_cell is computed here but discarded, and server_command / bench_command each re-call _first_sweep_coords independently, using the fallback values.

_build_panels (inference_payload.py:199-206) gates the whole Launch panel on launch_server_cmd, which is always present — so the panel always renders. With empty cells the literal output is:

{'launch_bench_cmd': 'vllm bench serve ... --random-input-len 1024 '
                     '--random-output-len 1024 --max-concurrency 128 ...',
 'launch_summary': 'vllm · TP=8 · max_model_len=8192'}
launch_example_cell present? False

Suppressing the label but keeping the commands is the worst of both — the reader gets a concrete-looking command with nothing marking it synthetic.

Suggest omitting launch_server_cmd and launch_bench_cmd entirely (not just launch_example_cell) when no first cell resolves.

For what it's worth the run-card Launch row is fine — provenance.py:125-126 builds it from variant.params only, so it isn't fabricated. The issue is confined to the panel's command blocks.

[atnair-amd]

🟠 This test errors out and never exercises capture_image_provenance.

It was appended inside class TestResolveContainerLifetime (opens at :299), but self._make() is defined on TestContainerOrchestrator (:77). There's no inheritance, mixin, or setUp linking them:

AttributeError: 'TestResolveContainerLifetime' object has no attribute '_make'

It fails on line 358 — the first line of the body — so container.py:551 gets zero coverage, and this is currently the only unit coverage of capture_image_provenance.

It is collected by run_all_unittests.py (813 tests discovered, this among them), so make ut is red on this branch.

The good news: this is purely class placement, not a logic problem. I grafted setUp/_make onto a subclass and ran the body unmodified — it passes, and mutating format_image_display or _ssh_command_output makes it fail, so the assertions genuinely exercise the two-step docker-inspect chain. Just move it into TestContainerOrchestrator; the body needs no changes.

[atnair-amd]

🟠 Both tests in this file fail, and the gate can't see them.

The path is input/config_file/inference/inferencex_atom/, but the only directory on disk is inferencex_atom_single. Both tests raise:

FileNotFoundError: variant config not found:
  cvs/input/config_file/inference/inferencex_atom/mi300x_inferencex-atom-single_deepseek-r1_fp8_smoke_config.json

Worth flagging: the referenced filename exists on no branch. There's a rename commit (ae2b6474) going inferencex_atom_single/ → inferencex_atom/, but it isn't an ancestor of this branch or dev/dtni — it lives on the unmerged multinode branch, and that branch's directory has no *smoke* file. Looks like the new dir name from one branch got combined with the old filename from another.

Separately, these are bare pytest-style functions in a unittests/ directory, which run_all_unittests.py:12 scans with unittest.TestLoader.discover. That collects 0 tests from this module — so make ut stays green while this file has zero effective coverage. That's precisely why the regex/schema mismatch in _first_sweep_coords shipped.

Two fixes needed: inferencex_atom → inferencex_atom_single, and convert to unittest.TestCase so the gate actually runs them. While there, the sibling test_inferencex_atom_config_loader.py anchors its root as Path(__file__).resolve().parents[3] — this file's cwd-relative Path("cvs") breaks when pytest runs from anywhere but the repo root.

[atnair-amd]

Minor / low priority.

pssh._process_output (pssh.py:190-197) appends stdout and stderr into the same output string, with exit_code set independently. So a docker command that exits 0 while warning — e.g. a malformed root ~/.docker/config.json — yields a multi-line image_id. I reproduced this:

rc=0
out=[WARNING: Error loading config file: /tmp/dkrcfg/config.json: invalid character 'N' ...
sha256:90630909f007ad4b58dd7b8271911f7b184aa97368ae35cd90eecce11bd98520]

That multi-line value is then shlex.quoted into the second docker call, which fails — so the digest is silently lost and it falls back to the image id.

Impact is limited: format_image_display truncates sha256:-prefixed values over 19 chars, which decapitates the warning, so the rendered row looks identical to the clean case. The residual is that a short or non-sha256: digest would leak the warning verbatim.

If you want to harden it, take the last non-empty line — docker writes the warning before stdout, so taking the first would be wrong.

Not a blocker: this matches existing repo practice (docker.py:210-218 does the same read-output/strip/check-exit_code without line-splitting), and the call is already wrapped in try/except.

[atnair-amd]

Minor. This row is permanently — for vLLM.

bind_runtime_provenance has exactly one call site in the tree (inference_suite_lifecycle.py:98), and vLLM defines its own test_launch_container (vllm.py:131) rather than importing the shared one — the two function bodies are identical apart from the 6 new provenance lines, so this reads as drift rather than deliberate divergence. The only fallback (inference.py:198) reads run_card.atom_image_pin, and the vLLM VariantConfig has no run_card field at all (it inherits extra="forbid", so it can't even be hand-added to the JSON).

To be clear this is not a regression — vLLM never had an Image row; on dev/dtni provenance_run_card_rows had no image branch and cvs/core/image_display.py didn't exist. vLLM strictly gains a row here (Git commit → Git ref). It's an un-extended feature.

The one new wrinkle is this line: since interactive_viewer defaults to True and vLLM doesn't override it, a large-enough vLLM sweep now renders an Image row that is always an em-dash. An always-empty field is arguably worse cosmetically than no field.

Heads-up if you close the gap: falling back to variant_config.container.image won't work as-is — both shipped vLLM configs have "image": "<changeme>" at :20. It'd need the runtime-resolved tag.