test: restore B200 power CI contract - #1610
Conversation
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 7 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (13)
🧰 Additional context used📓 Path-based instructions (2)Check that tests cover the changed behavior rather than only the happy path.⚙️ CodeRabbit configuration file Files:
Prefer applicable inline comments.⚙️ CodeRabbit configuration file Files:
🪛 OpenGrep (1.27.1)aic-core/rust/aiconfigurator-core/parity_tests/goldens/per_op.json[ERROR] 918-918: Possible credit card number (PAN) detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead. (coderabbit.pii.credit-card-number) 🔇 Additional comments (3)
WalkthroughThe change updates GPT-OSS and Nemotron-NAS per-operation energy goldens, enables energy checks for two TRT-LLM cases, and strengthens power-data invariant tests for paired measurements. ChangesEnergy validation updates
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The updated invariant and energy parity coverage introduce no demonstrated runtime or data risk, so the PR is ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Energy numbers wake in rows Comment |
Arsene12358
left a comment
Arsene12358
left a comment
There was a problem hiding this comment.
Requesting changes for one P2 regression in the power-data invariant: nullable numeric Parquet columns can cause null measurement pairs to pass the check. This violates AIC-1940's explicit fail-closed requirement; the inline comment includes a base/head reproduction and the minimal correction.
Validation: 342 local invariant/compile-engine/engine-step tests passed, Ruff checks passed, and both refreshed per-op golden records exactly matched the repository pinning tool's live computation. Current committed power data is valid; this finding concerns the invariant's handling of invalid input.
| finite = np.isfinite(power) & np.isfinite(power_limit) | ||
| measured = (power > 0.0) & (power_limit > 0.0) | ||
| unmeasured = (power == 0.0) & (power_limit == 0.0) | ||
| return ~(finite & (measured | unmeasured)) |
There was a problem hiding this comment.
[P2] Reject nullable missing values in the invalid-pair mask
When a Parquet file preserves pandas nullable Float64 columns, np.isfinite and the comparisons above propagate pd.NA. This return expression then produces missing booleans, which bad.any() at line 66 skips. Consequently, the invariant accepts null measurement pairs instead of rejecting them.
I reproduced this through the complete invariant function, pointing _DATA_ROOT at a temporary directory containing a real nullable_perf.parquet written from:
frame = pd.DataFrame(
{
"power": [None, 100.0, 100.0],
"power_limit": [1000.0, None, 1000.0],
},
dtype="Float64",
)
frame.to_parquet(tmp_path / "nullable_perf.parquet", engine="pyarrow")After the test's normal pq.read_table(...).to_pandas() call, the mask is [pd.NA, pd.NA, False] and bad.any() is False. Base 77fd077 rejects the two invalid rows; head 7e754583 passes the same file. With ordinary float64, both revisions reject it, which explains why the added synthetic examples miss the regression.
Please treat missing mask entries as invalid and add a nullable-Parquet round-trip regression test. This minimal correction makes the complete invariant reject both invalid rows while preserving the existing synthetic test results:
| return ~(finite & (measured | unmeasured)) | |
| return (~(finite & (measured | unmeasured))).fillna(True) |
This is an introduced regression in the guard required by AIC-1940, not a claim that current shipped data contains null pairs.
2 participants
Summary
power/power_limitmeasurement pair or the exact0.0/0.0unavailable-measurement sentinelLinear: AIC-1940
Related: #1584, #1590, AIC-1934
Why
PR #1584 added matched B200 TRT-LLM
1.3.0rc20power data with 487,422 measured rows and 40,549 intentional0.0/0.0sentinels for unmatched rows. A later invariant test rejected every non-positivepower_limit, while the compile-engine goldens still pinned the pre-power current slot. That merge-order drift mademainfail despite the shipped data satisfying its documented paired-value contract.Validation
pytest tests/unit/sdk/database/test_power_data_invariants.py: 2 passedruff check .: passedruff format --check .: 678 files already formattedenergy_wmsupdates, two provenance SHA updates, zero other case-field changes, zero parquet changesThe broad local Python lane excludes
tests/unit/sdk/database/test_moe_dispatch.pybecause the repository's macOS development environment does not install Torch; hosted container CI remains authoritative for that test.Summary by CodeRabbit
Tests
Chores