[1/4] feat(power): add the DCGM power artifact data layer - #288
Conversation
d3fa71d to
f45c9a2
Compare
Signed-off-by: Wenyao Gao <wgao11@u.rochester.edu>
f45c9a2 to
203fa17
Compare
|
@edwingao28 -- thanks for the PR. I'm taking a look and running initial reviews. I'm planning to have feedback for you sometime Monday or early on Tuesday; I'm feeling under the weather today. Does that timeline work for you? |
Yes, that timeline works for me. Thanks for taking a look, and hope you feel better soon! |
Code ReviewConfirmed Bugs
Plausible Bugs
Minor
Integration Gap (series-level)None of the four PRs in this series (#287–#291) touch the vendored Is there a tracking issue for the stamping integration? And is there a plan to block |
|
Oh -- I hadn't seen the note about vendor integration being a follow-on where eventually it seems like |
Thanks for confirming. Yes, that is the intended split: this series lands the artifact contract, collector, measurement-window plumbing, and validator, while the benchmark-side adapter will follow the migration to the centrally sourced benchmark implementation |
Signed-off-by: Wenyao Gao <wgao11@u.rochester.edu>
Thanks for the detailed review @FrankD412 . I addressed all eight code findings in fca5e2b: |
Signed-off-by: Wenyao Gao <wgao11@u.rochester.edu>
Signed-off-by: Wenyao Gao <wgao11@u.rochester.edu>
Signed-off-by: Wenyao Gao <wgao11@u.rochester.edu>
Normalize malformed exposition failures across the supported prometheus-client range, including IndexError from 0.20.x, while leaving BaseException control flow untouched. Signed-off-by: Wenyao Gao <wgao11@u.rochester.edu>
Reject boolean and non-finite result timing fields before comparing them with the completed measurement window. Signed-off-by: Wenyao Gao <wgao11@u.rochester.edu>
Reject regular files with non-JSON suffixes during the exhaustive window scan instead of accepting them as valid v1 window records. Signed-off-by: Wenyao Gao <wgao11@u.rochester.edu>
Code Review (second pass)Correctness Bugs
saw_power_sample = True # set here, before MIG check
if "GPU_I_ID" in sample.labels or "GPU_I_PROFILE" in sample.labels:
reasons.append(Reason.MIG_INSTANCE_UNSUPPORTED)
continue # sample discardedEvery scrape cycle returns
if len(distinct) == 0:
return {}, (Reason.NO_DEVICE_ASSIGNMENT,) # or equivalent
if len(distinct) != 1:
return {}, (Reason.CONFLICTING_WORKER_ROLES,)
Fragility
Test Coverage
Simplification
|
Signed-off-by: Wenyao Gao <wgao11@u.rochester.edu>
thanks for the review @FrankD412
Verification: 69 artifact tests passed; Ruff, format, Please let me know if the fix looks good to you, thanks! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #288 +/- ##
=======================================
Coverage ? 69.97%
=======================================
Files ? 76
Lines ? 10068
Branches ? 0
=======================================
Hits ? 7045
Misses ? 3023
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What
Data layer for multinode DCGM power artifacts: the file formats, parsing, and validation that a follow-up collector runtime writes and an offline validator audits. No runtime behavior changes — nothing imports this package yet; it lands first so the collector PR stays reviewable.
Contents (
src/srtctl/core/power/)contract.py— schema version, filenames, reason codes, sample-gap limit (MAX_SAMPLE_GAP_SECONDS), finite-number checks. Single source of truth for the on-disk contract.parser.py— Prometheus scrape → per-GPU power readings viaprometheus_client.parser(new dependencyprometheus-client>=0.20.0; parsing the exposition format by hand is strictly worse).samples.py— append-only CSV sample stream: writer, reader, observed-device derivation.topology.py— expected-device construction from the Slurm process topology; device identity keys; role/het-group resolution.manifest.py—manifest.jsonwriter: producer identity, expected windows, validation rollup, atomic writes.windows.py— measurement-window validation: coverage against the sample stream, gap checks, running→interrupted conversion.Evidence
The full series (this contract plus the collector and windows that follow) has been running in production CI on real multinode deployments:
Happy to attach a sample artifact bundle (samples CSV + windows + manifest) to the PR if useful for review.
Tests
tests/test_power_artifacts.py— 45 tests over the contract: round-trips, malformed-input reasons, device-set mismatches. Window coverage/gap edge cases are exercised by the consumers' suites in [3/4] and [4/4], which drive the realvalidate_expected_windows(including boolean-typedconcurrency/schema_versiontraps —boolis anintsubclass and must not key as concurrency 1).pytest tests/passes (same 3 pre-existing environment-dependent failures asmainon my machine: fingerprint CPU probe, apply_mock CPU allocation, mcp_spec explain-field).Series
fix(processes): guard the post-kill wait in ManagedProcess.terminate(independent fix, used by [3/4])