Repository navigation
fix(cli): report GPU inventory and disclose omitted devices - #13070
Dongni-Yang wants to merge 5 commits into
Conversation
Preserve each detected GPU name and memory size in readiness evidence. Keep the minimum-memory observation used by serving-profile admission. Refs #13019 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Code coverage overviewLanguages: TypeScript TypeScript / code-coverage/plugin
View coverage of the 10 most impacted files.
TypeScript / code-coverage/cli
View coverage of the 10 most impacted files.
ℹ️ Calculations represent line coverage. Results may change as calculations are updated. Updated |
Use the current E2E assertion budget and its matching runtime tests. The GPU reporting change stays unchanged. Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Preserve referenced and non-GPU evidence within the existing report limit. Expose bounded omission counts without replacing existing diagnostic details. Closes #13019 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Signed-off-by: Dongni Yang <dongniy@nvidia.com>
|
PR Review Advisor finished for commit Request review only when Require no Advisor blockers is green. |
rsliter
left a comment
There was a problem hiding this comment.
Reviewed latest PR commit 7a25ef923fe433a6a342efed3721952a7965c87a against base 46888639a2bf2727b6703c9beabe0ec8670db1ef.
The change preserves the existing readiness decision and evidence limits while exposing bounded per-GPU name and memory details. Required evidence remains ahead of optional GPU rows, omitted-device counts stay report-owned and validated, repeated sanitization is stable, and the text path receives escaped, length-bounded names. The regression suite covers asymmetric inventories, 255/256/257-device boundaries, required-evidence saturation, malformed summaries, stale or failed collection, and terminal controls. I found no blocking correctness, compatibility, or security issue. All nine security categories pass; the change adds no credential, authorization, dependency, cryptography, policy, or mutation surface.
Required checks pass on this commit. PR Review Advisor evaluated this exact commit and base, reported no findings, and recommended no additional E2E selectors because the changed boundary has deterministic coverage. The exact-commit self-hosted qualification, including the generic NVIDIA GPU lane, also passed.
Outcome
Host probe reports each detected GPU's index, name, and memory in JSON and text.
When the public evidence limit omits device rows, both outputs report how many were collected, listed, and omitted.
Reason
The host projection retained aggregate GPU memory but discarded individual device details.
The first inventory fix exposed those details, but large inventories could silently exceed the public evidence limit.
Related issues
Closes #13019.
Changes
gpuInventorysummary without replacing evidence details or increasing evidence limits.Verification
Current commit:
3fec450eeb0b0a53ff9270728d9e1fac95ca814c.npx vitest run --project cli src/lib/readiness --maxWorkers=1— 335 tests pass across 13 files. The final inventory and contract suites pass all 42 tests.58e6887; 22 controls pass. The fixed focused suites pass. The cases check report ownership, no unrelated-evidence annotation, malformed totals, and repeated projection.npm run typecheck:clipass.Detection gap: the original tests covered small inventories and did not test evidence-limit behavior.
The added boundary cases require explicit omission counts in JSON and text.
Tests inject GPU observations. No physical multi-GPU run is claimed.
The diff contains no secrets, API keys, or credentials.
Review notes
Independent reviews checked evidence priority, count validation, redaction, retained diagnostic details, and unchanged readiness decisions.
The optional report summary fits the existing additive report schema. The contract test verifies the projected result with the published JSON schema.
The final repair addresses the Advisor architecture finding on
58e6887: aggregate GPU counts now belong to the report instead of an unrelated diagnostic entry.The branch includes main's shared image dependency repair and qualification receipts.
At head
3fec450eeb0b0a53ff9270728d9e1fac95ca814c, source CI and self-hosted qualification passed, including the generic NVIDIA GPU lane. CodeRabbit reviewed this exact head and identified no concrete blocker. All nine Advisor specialist artifacts passed digest and head/base/workflow provenance checks. Eight reports were clear. One report requests a separate identity channel for omitted GPUs. That request is excluded from this repair as a deferred enhancement: the reported two-device host fits the current production report, while the synthetic zero-slot case intentionally preserves required evidence and prints exact omission counts. The required-first selector and 256-entry limit are inherited from the recorded base. The Advisor gate remains failed on that report; no check waiver or maintainer approval is claimed.Maintainer approval remains required.
Signed-off-by: Dongni Yang dongniy@nvidia.com
Summary by CodeRabbit