Repository navigation
[VideoGenX] add offline GPU job and run comparison tools / [VideoGenX] 添加离线 GPU 作业与运行对比工具 - #4659
edwingao28 wants to merge 10 commits into
Conversation
ec6498b to
830f229
Compare
57ae0da to
64b152c
Compare
830f229 to
f45993b
Compare
64b152c to
29dc022
Compare
f45993b to
c7570bd
Compare
755e148 to
ddbf379
Compare
3097220 to
b997639
Compare
ddbf379 to
7702419
Compare
b997639 to
f728cc5
Compare
7702419 to
a90a255
Compare
f728cc5 to
b6961d6
Compare
a90a255 to
3192565
Compare
b6961d6 to
c659df9
Compare
3192565 to
07850c6
Compare
07850c6 to
b7edc5f
Compare
c659df9 to
d9353c8
Compare
|
Claude finished @edwingao28's task in 1m 45s —— View job Review: PR #4659
I found one blocking issue and one warning, both in
No perf-changelog entry is needed: this PR only touches |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return [] | ||
| result = [] | ||
| for index in range(1, warmups + 1): | ||
| result.append({**cases[(index - 1) % len(cases)], "phase": "warmup", "repetition": 0, "slot_id": f"warmup-{index:03d}"}) |
There was a problem hiding this comment.
🔴 BLOCKING: _planned ignores plan["warmup_case"], so its schedule differs from runner._slots (runner.py:254) and compare.py:281, which both use plan.get("warmup_case") or cases[...].
Why it matters: runner.validate_plan requires warmup_case for every fairness/H3 set, and it must be disjoint from the measured cases. For those plans, each warmup slot here gets the first measured case's case_id, prompt and seed. Line 481 then sees a mismatch and marks every warmup invalid_record, which adds an issue. That forces the report status to incomplete, downgrades any recorded CI pass to inconclusive, and makes the warmup ledger show failures even when verify_measurement_job (which uses _slots) accepted the job.
Fix:
| result.append({**cases[(index - 1) % len(cases)], "phase": "warmup", "repetition": 0, "slot_id": f"warmup-{index:03d}"}) | |
| result.append({**(plan.get("warmup_case") if isinstance(plan.get("warmup_case"), dict) else cases[(index - 1) % len(cases)]), "phase": "warmup", "repetition": 0, "slot_id": f"warmup-{index:03d}"}) |
Please also add a test_gpu_report.py case whose plan includes a warmup_case.
| declared, observed = spec.get(label), metadata.get("source_identity") | ||
| reasons = [] | ||
| if (not isinstance(declared, dict) or not isinstance(observed, dict) | ||
| or any(not isinstance(declared.get(field), str) or observed.get(field) != declared[field] for field in ("source", "revision", "source_sha256", "python")) |
There was a problem hiding this comment.
🟡 WARNING: This compares observed["source"] to the declared spec[label]["source"] exactly. The observed value comes from source_file_manifest, which stores str(Path(source).resolve(strict=True)) (gpu_job.py:325,355). validate_gpu_job only requires an absolute path, not a canonical one.
Why it matters: Suppose the operator declares the source through a symlinked path, which is common on clusters (for example /home → /data/home). The job still passes verify_measurement_job, which checks only revision and source_sha256. This report, however, flags a provenance mismatch and withholds all GPU timings for both arms. Unlike source, sglang_module stays unresolved because PYTHONPATH uses the declared path.
Fix: Pick one of these:
- Compare against the resolved declared path here, using the recorded value only as a string.
- Reject non-canonical
sourcepaths invalidate_gpu_job, so the mismatch is caught before any GPU time is used.
| return [] | ||
| result = [] | ||
| for index in range(1, warmups + 1): | ||
| result.append({**cases[(index - 1) % len(cases)], "phase": "warmup", "repetition": 0, "slot_id": f"warmup-{index:03d}"}) |
There was a problem hiding this comment.
🔴 Operators reporting any fairness plan get a report marked "incomplete", with the warmup flagged invalid and a recorded CI pass shown as inconclusive. _planned builds warmup slots from cases[(index - 1) % len(cases)] and ignores the plan's warmup_case. runner._slots uses warmup_case when present, and fairness plans must have one. Fix: build the report schedule exactly as runner._slots does, preferring plan.get("warmup_case") for warmup slots, or import and reuse _slots so the two cannot drift.
Why this was flagged
runner.validate_plan (runner.py:244) requires warmup_case for every plan that has fairness. runner._slots (runner.py:254) then gives warmup-001 the warmup_case's case_id, prompt and seed, and the client records those values. The supervisor's verifier verify_measurement_job uses _slots, so the job passes there. write_gpu_report -> _role -> _planned (gpu_report.py:164-165) instead expects cases[0] for warmup-001. At gpu_report.py:481 the record differs on case_id/prompt/seed, so the row becomes "invalid_record" and an issue is logged. That issue moves report status from "complete" to "incomplete" (gpu_report.py:773-774) and downgrades a recorded regression "pass" to "inconclusive" (gpu_report.py:761-768), for a job the supervisor verified as clean.
Verification: Trigger: any gpu-job whose frozen plan has a warmup_case. Every fairness plan must have one (runner.py:244). runner._slots (runner.py:254) builds warmup slots from plan.get("warmup_case"), but gpu_report._planned (lines 164-165) never reads warmup_case. At line 481 warmup-001 mismatches its slot, so a recorded pass becomes "inconclusive" (lines 763-768) and the report "incomplete" (lines 773-774).
| raise ValueError("run failure/success denominator does not match raw outcomes") | ||
| measurement = run.get("measurement", {}) | ||
| wall = measurement.get("wall_seconds") | ||
| if not _equal(config.get("serving"), spec.get("serving")): |
There was a problem hiding this comment.
🔴 Closed-loop serving jobs with a whole-number delivery_deadline_seconds (e.g. 300) always fail evidence verification. Real regressions are then reported as inconclusive instead of failing. gpu_job.py:1156 passes the value to the client CLI via str(), the client parses it as a float, and run.json stores 300.0. gpu_evidence.py:171 compares canonical JSON bytes, so "300" never matches "300.0". Fix: make the spec and the client agree on one numeric form (e.g. normalize the deadline with float() in validate_gpu_job), or compare the serving settings by value, as gpu_report.py:439 does with ==.
Why this was flagged
The trigger is a spec with "serving": {"concurrency": 2, "delivery_deadline_seconds": 300}, run through vgbench gpu-job --execute. validate_gpu_job (gpu_job.py:175) keeps the int 300 via serving.settings. The client parser uses type=float (cli.py:198), so run_plan writes configuration.serving.delivery_deadline_seconds = 300.0 (runner.py:582, 659). verify_measurement_job checks _equal(config.get("serving"), spec.get("serving")) (gpu_evidence.py:171), and canonical_json_bytes serializes these as 300 vs 300.0. run_gpu_job catches this (gpu_job.py:1419-1421), sets regression_status "inconclusive" with "acceptance evidence could not be verified: ValueError", and skips _gate. A comparison overall_status of "fail" or a memory-gate failure is therefore never recorded as "fail", and the CLI exits 2 instead of 1. test_gpu_job.py:985 uses 300, but saved_job copies spec["serving"] straight into the run configuration (test_gpu_job.py:569), so the float conversion is never exercised.
Verification: Severity: normal. gpu_job.py:1156 passes "300" to the client, which parses it with type=float (cli.py:198), so run.json holds 300.0. gpu_evidence.py:171 compares canonical_json_bytes against the spec's int 300 and raises. gpu_job.py:1411-1421 then sets regression_status="inconclusive", so a real regression is reported as inconclusive. The fixture at test_gpu_job.py:983-987 writes run.json directly.
| _check_deadline(deadline) | ||
| timeout = self.timeout if deadline is None else min(self.timeout, max(0.001, deadline - time.monotonic())) | ||
| rows = {row["gpu"]: row for row in smi("static", timeout)} | ||
| gpus = [] | ||
| for key in self.devices: | ||
| limits = rows[self.identity[key]["gpu"]]["limit"] | ||
| if "ppt0" in limits: | ||
| limits = limits["ppt0"] | ||
| configured, maximum = "socket_power_limit", "max_power_limit" | ||
| else: | ||
| configured, maximum = "socket_power", "max_power" | ||
| gpus.append({"uuid": key, | ||
| "configured_limit_w": number(limits.get(configured), "W", optional=True), | ||
| "maximum_limit_w": number(limits.get(maximum), "W", optional=True), | ||
| "enforced_limit_w": None, "default_limit_w": None}) | ||
| return {"observed_at": _now(), "query": "amd-smi static --json", "status": "recorded", "gpus": gpus} | ||
|
|
There was a problem hiding this comment.
🔴 On AMD, one transient amd-smi failure after a role has measured and cleaned up can fail the whole job. It also skips the run-hash update and the receipt write. AmdGpuProbe.power_configuration raises on any smi() error, deadline or missing limit key. GpuProbe.power_configuration catches those errors and returns status: "unavailable". Fix: give the AMD probe the same contract, catching RuntimeError/ValueError/OSError/KeyError/TimeoutExpired/TimeoutError and returning a status of unavailable with an error. That keeps the power_configuration_after call in _role's finally from raising. [also at: experimental/video-generation/evaluator/gpu_job.py:1207 - On AMD runs, a failed or late amd-smi static query after a role finishes can throw away a completed measurement and replace the real failure reason.]
Why this was flagged
_role calls probe.power_configuration(deadline=supervisor.total_deadline) inside its finally block (gpu_job.py:1207). That call comes before the partial run.json rehash (gpu_job.py:1208-1211) and the _write of gpu-job.json (gpu_job.py:1212). With gpu_vendor: "amd", the probe is AmdGpuProbe, whose power_configuration (amd_gpu.py:160-176) has no try/except. A non-zero amd-smi static exit, a subprocess.TimeoutExpired, a passed deadline or a missing limit key escapes the finally block. It replaces any original exception, so the receipt records an amd-smi error instead of the real cause. In the success path it turns a clean, fully measured role into status: "failed" (exit code 1). The NVIDIA GpuProbe.power_configuration (gpu_job.py:733-758) swallows these errors and records status: "unavailable", so only AMD jobs hit this.
Verification: AmdGpuProbe.power_configuration (amd_gpu.py:159-175) calls _check_deadline(deadline), then smi("static", timeout), then rows[...]["limit"], with no try/except. gpu_job.py:1207 calls probe.power_configuration(deadline=supervisor.total_deadline) inside _role's finally block. A raise there escapes the finally block and replaces any earlier exception.
| MAX_JSON_BYTES = 8 * 1024 * 1024 | ||
| MAX_MEDIA_BYTES = 512 * 1024 * 1024 | ||
| MAX_TOTAL_MEDIA_BYTES = 2 * 1024 * 1024 * 1024 | ||
| MAX_TELEMETRY_BYTES = 16 * 1024 * 1024 |
There was a problem hiding this comment.
🔴 Valid long or many-GPU jobs can get a report with GPU timing withheld and a recorded CI pass shown as "inconclusive", because the report reads at most 16 MiB of telemetry. The job and verifier accept files up to 128 MiB and 100000 samples. MAX_TELEMETRY_BYTES = 16 * 1024 * 1024 makes _verify_telemetry reject any larger file. Fix: set the report's telemetry limit to the producer's bounds (128 MiB / 100000 samples in gpu_evidence.py:183,196), or share one constant between the two modules.
Why this was flagged
Each telemetry line stores every selected GPU's record plus owned compute apps with process identities, roughly 0.5 KB per GPU per sample. At telemetry_interval_seconds: 1 on 8 GPUs, a role longer than about an hour writes more than 16 MiB of supervisor/<role>/telemetry.jsonl. verify_measurement_job accepts this, since it allows up to 128 MiB and 100000 samples (gpu_evidence.py:183,196). In gpu-report, _Tree.open(relative, MAX_TELEMETRY_BYTES) raises "Input exceeds the viewer byte limit" (gpu_report.py:104-105,268). file_sha256_verified therefore stays False, and _provenance_reasons withholds GPU timing for that role. Because issues is now nonempty, a supervisor-recorded pass becomes ci_status "inconclusive" and the status becomes "incomplete" (gpu_report.py:761-776). The result is a degraded dashboard for a job the supervisor itself verified and accepted.
Verification: gpu_report.py:27 sets MAX_TELEMETRY_BYTES = 16 * 1024 * 1024. _verify_telemetry raises "Telemetry exceeds the viewer byte limit" (gpu_report.py:268-271), while verify_measurement_job accepts up to 128 * 1024 * 1024 bytes (gpu_evidence.py:183). GPU timing is withheld (lines 447-452), and a recorded regression_status == "pass" is changed to "inconclusive" (lines 761-768).
| declared, observed = spec.get(label), metadata.get("source_identity") | ||
| reasons = [] | ||
| if (not isinstance(declared, dict) or not isinstance(observed, dict) | ||
| or any(not isinstance(declared.get(field), str) or observed.get(field) != declared[field] for field in ("source", "revision", "source_sha256", "python")) |
There was a problem hiding this comment.
🔴 When a spec's source path contains a symlink or a trailing slash, the report withholds GPU timing for a job the supervisor accepted. _provenance_reasons compares the observed source, which source_file_manifest resolves at gpu_job.py:325, against the raw declared string. _source_identity compares resolved paths and accepts such a spec. Fix: compare resolved forms in the report (or check only the revision and digest fields), or make validate_gpu_job reject non-canonical source paths so both sides agree.
Why this was flagged
The condition is an operator-supplied baseline.source or candidate.source such as /home/u/sglang, where /home is a symlink, or a value ending in /. validate_gpu_job only requires an absolute, traversal-free path (gpu_job.py:146-150,198). source_file_manifest records "source": str(source) after Path(source).resolve(strict=True) (gpu_job.py:325,355). The job's own checks compare revision and source_sha256, plus resolved module paths (gpu_job.py:439,447-449), so the run succeeds and can be CI-accepted. In gpu_report.py:343, observed.get(field) != declared[field] for source fails on the resolved-versus-declared spelling. That adds "Observed source/Python identity does not match the pinned runtime specification" and withholds GPU timing for both roles. The issue also downgrades a recorded pass to "inconclusive" (gpu_report.py:761-768).
Verification: The trigger is a baseline.source or candidate.source that goes through a symlink or ends in a slash. source_file_manifest resolves it (gpu_job.py:325), and _source_identity checks only revision and source_sha256 (line 439), so the job is accepted. _provenance_reasons (gpu_report.py:343) compares the resolved path against the declared string, so GPU timing is withheld.
| "driver_version": static["driver"]["version"], "mig_mode": "N/A", | ||
| "vendor": "amd", "pci_bdf": self.identity[key]["bdf"]}) | ||
| for item in by_gpu[index]: |
There was a problem hiding this comment.
🔴 If amd-smi lists an idle GPU with a placeholder process_info string instead of an empty list, every AMD job fails at the idle check before launch. snapshot assumes each process_info is a dict and calls proc.get("pid"). A string raises AttributeError, which escapes _role's pre-launch probe.snapshot() (gpu_job.py:1096). Fix: treat a non-dict process_info with no PID as no process. Still reject malformed dict entries, which the existing check does.
Why this was flagged
The condition is an AMD host where a selected GPU has no resident GPU context, for example no gpuagent monitor context on that device. amd-smi process --json then reports process_list with a placeholder entry such as {"process_info": "No running processes detected"}. I cannot confirm that output from this checkout. The only fixture (test_amd_gpu.py:28) always includes a pid-42 monitor context. At amd_gpu.py:202-204 the loop does proc = item["process_info"] and then proc.get("pid"), which raises AttributeError on a string. _role calls probe.snapshot() at gpu_job.py:1096, before any runtime starts, so the job is recorded as failed with AttributeError. That happens exactly when the GPUs are idle, which is the state the check is meant to accept. Base had no AMD path.
Verification: Trigger: amd-smi gives an idle GPU's process_list the placeholder {"process_info": "No running processes detected"}. At experimental/video-generation/evaluator/amd_gpu.py:207, proc.get("pid") raises AttributeError, not the intended RuntimeError. In gpu_job.py _role, probe.snapshot() (line 1096) runs before the idle check, so an idle GPU makes the job fail before launch.
| _ROLES = ("baseline", "candidate") | ||
| _SHA = re.compile(r"[0-9a-f]{64}") | ||
| _REV = re.compile(r"[0-9a-f]{40}") | ||
| _GPU = re.compile(r"GPU-[0-9a-fA-F]{8}(?:-[0-9a-fA-F]{4}){3}-[0-9a-fA-F]{12}") |
There was a problem hiding this comment.
🟡 (optional) Uppercase NVIDIA GPU UUIDs pass validation but can never run, and they fail only after the full model-weight hash with a misleading MIG/inventory error. _GPU accepts [0-9a-fA-F]. nvidia-smi and cuda_devices emit lowercase UUIDs, so the exact row[1] not in self.devices check at gpu_job.py:773 always fails. Fix: accept lowercase only in _GPU, as _AMD_GPU already does, or lowercase gpu_uuids during validation. That keeps the snapshot check and the child check at _LAUNCH (gpu_job.py:52) consistent.
Why this was flagged
Trigger: an NVIDIA spec with gpu_uuids such as "GPU-ABCDEF01-...". validate_gpu_job accepts it (gpu_job.py:190 with _GPU at :44). run_gpu_job runs the calibration preflight, then _model_manifest, which hashes every weight file (gpu_job.py:1363). _role then runs the source identity probe. Only after that does the first probe.snapshot() at gpu_job.py:1096 compare nvidia-smi's lowercase UUID against the uppercase spec, at gpu_job.py:773. It raises "GPU inventory is incomplete, changed, or MIG-enabled". Even past that point, _LAUNCH's devices() != VGBENCH_GPU_UUIDS check would fail, because cuda_devices formats UUIDs with str(uuid.UUID(...)), which is lowercase. Preview reports such a spec as valid.
Verification: gpu_job.py:44 defines _GPU = re.compile(r"GPU-[0-9a-fA-F]{8}..."), which accepts uppercase. nvidia-smi prints UUIDs in lowercase hex. GpuProbe.snapshot compares exactly with row[1] not in self.devices (:773) and raises "GPU inventory is incomplete, changed, or MIG-enabled". It costs a full model hash before failing with the wrong diagnosis.
|
|
||
| @pytest.mark.parametrize("change", [ | ||
| "start_ticks", | ||
| "memory", | ||
| ], ids=['start_ticks', 'memory']) | ||
| def test_monitor_exception_rejects_changed_or_active_context(monkeypatch, change): | ||
| identity = {"pid": 42, "start_ticks": 50, "executable": "/opt/gpuagent/gpuagent", "uid": 0} | ||
| receipt = {"status": "verified", "process": identity} | ||
| app = {"pid": 42, "memory_used_mib": 0} | ||
| observed = dict(identity) | ||
| if change == "memory": | ||
| app["memory_used_mib"] = 1 | ||
| elif change == "unverified": | ||
| receipt["status"] = "unverified" | ||
| elif change == "executable": | ||
| observed[change] = "/tmp/other" |
There was a problem hiding this comment.
🟡 nit (optional): the parametrize lists cover only start_ticks and memory, so the unverified and executable branches can never run. Likewise legacy_static covers only True, so the default ppt0 power-limit path is never asserted. Fix: add the missing values to the parametrize lists, or delete the dead branches, as the AGENTS.md test rules require.
Why this was flagged
test_monitor_exception_rejects_changed_or_active_context is parametrized with only "start_ticks" and "memory" (test_amd_gpu.py:80-83). Its elif change == "unverified" and elif change == "executable" branches (lines 89-92) are therefore dead code. test_snapshot_preserves_zero_memory_process_and_measured_power is parametrized with only True (lines 32-34). So the "ppt0" in limits branch of AmdGpuProbe.power_configuration (amd_gpu.py:166-167), which the default observations() shape uses, is never tested. A regression in either untested path passes CI. AGENTS.md says a test must assert on what the real code computes and must not carry unreachable branches.
Verification: The monitor test is parametrized only with "start_ticks" and "memory" (test_amd_gpu.py:81-84). So the unverified and executable branches (lines 92-95) never run. The snapshot test is parametrized as [True] only (lines 32-34). So the if "ppt0" in limits branch in amd_gpu.py:166-167 is never reached by any test. Production behaviour is not broken.
| drained. That unreaped leader reserves the group ID; PID/start-time/session | ||
| checks precede every group signal. Escaped or invisible GPU processes are a | ||
| cleanup/attribution failure, never permission to kill an unrelated process. | ||
| """ |
There was a problem hiding this comment.
🟡 nit (optional): each new module opens with a docstring describing the file, which AGENTS.md's mandatory comment rule forbids ("No header comments describing a file"), though older evaluator modules do the same. sweep:^""" in evaluator/gpu_job.py, gpu_evidence.py, gpu_manifest.py, gpu_report.py, amd_gpu.py. Example: gpu_job.py:1-12 and gpu_report.py:1-7. Fix: drop the file-level docstrings, or reduce them to short line-level technical notes next to the code they explain.
Why this was flagged
AGENTS.md lines 54-58 say comments are mandatory in every file, with "No header comments describing a file, topology, config, recipe, override, or section". The diff adds file-describing module docstrings at gpu_job.py:1-12, gpu_evidence.py:1-6, gpu_manifest.py:1-6, gpu_report.py:1-7 and amd_gpu.py:1. The only effect is a convention breach, with no runtime impact. Existing modules such as runner.py, compare.py and serving.py already do this, which is why this is only a nit.
Verification: AGENTS.md:54-56 says: "No header comments describing a file, topology, config, recipe, override, or section." The repo-root CLAUDE.md:3 sends all guidance to AGENTS.md, so this rule applies here. Every new module opens with a file-describing docstring, for example gpu_job.py:1-12 and gpu_report.py:1-7. Pre-existing modules follow the same pattern. Runtime behavior is unaffected.
edwingao28
changed the title
Summary
Parked offline tools for supervised H3 runs. The native CI sweep (#2916) does not call them, so they stay out of the v0.1 stack.
gpu-manifest,gpu-job,gpu-report: hash runtime and model files, run baseline and candidate on the same owned NVIDIA or AMD GPUs with power calibration, and render the evidence.Testing: Local suite: 183 passed, 2 skipped, on the pre-cleanup core. Port onto current #4658, which now holds comparison and PSNR, before reviving.
Manual
srt-yaml-serve/srt-yaml-clientcommands were removed from #2916 in 0fccd68.PR checklist and change type
Type of Change
Checklist
python -m infx.matrix.changed --base origin/mainlists affected config keys, I have appended one new entry covering them to the physical end ofinferencex-e2e/perf-changelog.yaml; if it lists none, I have added none. I have not edited historical entriesinferencex-e2e/perf-changelog.yamlentry, it carries exactly one primary sweep label (a maintainer applies it on fork PRs):full-sweep-fail-fast(recommended),full-sweep-enabled, ornon-canary-full-sweep-enabled. Optional modifiersall-evals,evals-only, andagentx-fastrequire a primary label; the last two block reuse while applied.OWNER/MEMBER/COLLABORATOR) has commented/use <run_id>(or the legacy/reuse-sweep-run) on this PR. Do this only once there is a final full sweep that is all green with evals passing, since after this comment the primary sweep label will no longer automatically kick off new sweeps. Remove and re-add the primary sweep label to force a new sweep.