fix: preserve DCP identity in FPM accuracy evaluation - #271
Conversation
Signed-off-by: Simone Chen <simonec@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/aisimulate/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 12 included reviews per hour; 3 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (3)Require coverage of the changed behavior and its negative or boundary cases.⚙️ CodeRabbit configuration file Files:
Read REVIEW.md before commenting.⚙️ CodeRabbit configuration file Files:
Before making any change under: `python/aisimulate/src/aiconfigurator/generator/**` MUST read: `python/aisimulate/.claude/rules/generator-development.md` Before making any change under `python/aisimulate/collector/**` MUST read: `python/ais...📄 CodeRabbit inference engine (AGENTS.md) Files:
🪛 ast-grep (0.45.3)tests/fpm_accuracy/test_evaluation.py[info] 131-131: use jsonify instead of json.dumps for JSON output (use-jsonify) 🔀 Multi-repo context ai-dynamo/dynamo, ai-dynamo/aiconfiguratorLinked repositories findingsai-dynamo/dynamo
ai-dynamo/aiconfigurator
🔇 Additional comments (4)
📝 SummaryRisk: MediumHuman attention should focus on:
Changes
Evidence
Technical qualityThe changes support separate DCP identity handling, legacy defaulting, fail-closed validation, and preserved regression evaluation. Native DCP support remains intentionally absent. Merge readinessCurrent-head Full CI validation is missing. Reviews do not constitute approval. Merge readiness remains unconfirmed. WalkthroughDCP is now a separate FPM identity field. Legacy inputs default missing DCP to 1. Native FPM rejects DCP values above 1. Dataset validation and evaluation tests cover the new cases. ChangesDecode context parallelism support
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to DCP configurations retain regression coverage while unsupported native predictions are reported safely; no concrete merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
Comment |
simone-chen
left a comment
simone-chen
left a comment
There was a problem hiding this comment.
- Codex review of dd59024: traced manifest DCP through worker identity, sidecar validation, parquet columns, native predictor construction, and the public coverage result. No remaining finding in this diff.
- Missing legacy DCP consistently means one; non-default missing/mismatched values fail closed. Native FPM cannot silently substitute ordinary CP, while worker regression remains independent of the native EngineConfig adapter.
- Validation: 154 FPM tests and 77 Pages tests passed, including 15 new DCP cases that failed before the change. The real failing HF leaf now loads, and all 30 current library pairs validate. The new leaf has zero observations, so this does not claim native DCP prediction accuracy.
- Hosted campaign validation remains in progress: https://github.com/ai-dynamo/aisimulate/actions/runs/35300025904.
|
Closeout review of Two closeout items remain:
I am addressing these items under the requested approval-and-merge closeout; merge remains gated on fresh checks and applicable approval. |
jasonqinzhou
left a comment
jasonqinzhou
left a comment
There was a problem hiding this comment.
Integration review against current main a2eae63 found one compatibility issue that must be fixed before merge.
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
|
@coderabbitai full review Please review current head |
jasonqinzhou
left a comment
jasonqinzhou
left a comment
There was a problem hiding this comment.
Codex review of exact head eaaf52d39683c088e30d9f513c7af7fea8bf13c7 against main a2eae639acd8b38aa67cddf25bc61fc10ad979ca: reviewed the complete eight-file diff, identity validation, native/canonical/older-wheel predictor construction, worker-isolated regression, and public coverage accounting. No remaining actionable finding. The canonical DCP regression integration finding was reproduced by a failing test, fixed in the current commit, and resolved after push.
Validation: 234 local tests plus 132 subtests passed; four compiled-extension cases await installed-wheel Full CI. Ruff lint/format, packaged legal-file consistency, and diff checks passed. Current-head Fast CI succeeded (35387383649 and 35387376079), and codeowners passed. CodeRabbit full review was requested because its incremental attempt skipped the new head. Full CI will follow the completed current-head review. This is review evidence, not a merge-readiness claim.
|
✅ Action performedFull review finished. |
jasonqinzhou
left a comment
jasonqinzhou
left a comment
There was a problem hiding this comment.
Approved following exact-head review of eaaf52d39683c088e30d9f513c7af7fea8bf13c7 against current main a2eae639acd8b38aa67cddf25bc61fc10ad979ca.
The DCP identity handling is strict and backward compatible. The integration issue with canonical regression has been fixed: native FPM alone rejects DCP, while case/worker-scoped regression retains its observations. The negative/compatibility tests reproduce the defect before the fix and pass afterward. No remaining actionable findings or unresolved threads; CodeRabbit's full review covers this SHA and found no actionable comments.
Local validation passed 234 tests and 132 subtests, with four compiled-extension cases deferred to installed-wheel Full CI. Ruff lint/format, legal consistency, whitespace, Fast CI, Pages, and codeowners checks passed. The zero-measurement hosted DCP leaf is correctly documented as campaign/schema evidence, not prediction accuracy.
This approves the code change. Merge must still wait for successful required Full CI on this SHA (run 35388175195) and a current-base check.
jasonqinzhou
left a comment
jasonqinzhou
left a comment
There was a problem hiding this comment.
Full CI caught an inconsistent synthetic topology in the newly expanded compiled-runtime test. This needs a fixture correction before merge.
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
jasonqinzhou
left a comment
jasonqinzhou
left a comment
There was a problem hiding this comment.
Codex re-review of exact head 11d18b7a175713bf22abe40ff323acde1f2c293f: the only change since reviewed head eaaf52d3 makes the synthetic TP=8 fixture's MoE-TP identity agree across its manifest, sidecar, and Parquet rows. This fixes the compiled test's invalid topology without relaxing runtime validation. Reviewed the complete resulting PR diff and both resolved findings; no remaining actionable finding.
Local FPM/Pages validation: 234 passed, 4 compiled-extension skips, 132 subtests passed; Ruff and diff checks passed. The canceled previous-head CI run exposed this test-fixture defect and is not evidence for this head. Fresh current-head Fast CI, CodeRabbit, and Full CI are required before merge.
jasonqinzhou
left a comment
jasonqinzhou
left a comment
There was a problem hiding this comment.
Approved exact head 11d18b7a175713bf22abe40ff323acde1f2c293f after final review and successful validation against current main a2eae639acd8b38aa67cddf25bc61fc10ad979ca.
Both findings are fixed and resolved: canonical worker regression remains usable for DCP while native FPM rejects unsupported DCP, and the compiled-runtime fixture now satisfies the topology invariant across every pinned identity source. No remaining actionable findings; CodeRabbit reviewed this SHA successfully.
Fast CI passed (35389435983). Full CI passed (35390051097), with all 54 validation jobs successful and only the inapplicable staging job skipped. On both ARM64 and AMD64, contract tests passed 2,021 tests and 132 subtests, including all four compiled-extension cases skipped locally; the five remaining skips belong to the separate integration suite. Package/compatibility checks also passed 149 tests per architecture. Pages, codeowners, DCO, local lint/format, and whitespace checks passed.
The hosted zero-measurement DCP leaf remains campaign/schema evidence only; no native DCP prediction-accuracy claim is made.
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
jasonqinzhou
left a comment
jasonqinzhou
left a comment
There was a problem hiding this comment.
Codex exact-head review of a0cb4e4bd64b46edbf1f3a041240ea1042b20816 against main e7c3ccac45fd89d98a217a7e5bc90d53298b3f6e: main advanced with a README/docs change after the prior Full CI completed. This synchronization merges those four upstream files cleanly. All FPM evaluator code, tests, and documentation are byte-identical to the fully reviewed and Full-CI-passing 11d18b7a head; the intended PR diff remains eight files.
No remaining actionable finding and both existing threads are resolved. Fresh local FPM/Pages tests passed 234 tests plus 132 subtests (four native-extension cases skipped locally); four inherited static-documentation tests also passed. The native cases passed on both architectures in prior run 35390051097. New-head Fast CI, CodeRabbit, and Full CI will be verified before the SHA-guarded merge.
|
@coderabbitai full review Please refresh coverage for exact head |
|
✅ Action performedFull review finished. |
jasonqinzhou
left a comment
jasonqinzhou
left a comment
There was a problem hiding this comment.
Approved exact head a0cb4e4bd64b46edbf1f3a041240ea1042b20816 against current main e7c3ccac45fd89d98a217a7e5bc90d53298b3f6e after Codex re-review and CodeRabbit's successful full review. No remaining actionable findings; both review threads are resolved.
The synchronization only brings in main's README/docs update. FPM source and tests are byte-identical to 11d18b7a, which passed all 54 Full CI validation jobs, including the compiled DCP regression cases on ARM64 and AMD64. Fresh local FPM/Pages tests passed 234 tests and 132 subtests, and the four inherited static-doc tests passed. Current-head Fast CI, Pages, and codeowners checks passed.
This approves the current code. Merge remains conditional on required Full CI completing successfully for this SHA and the branch still being current with main.
|
/ok to test a0cb4e4 |
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
jasonqinzhou
left a comment
jasonqinzhou
left a comment
There was a problem hiding this comment.
Codex current-head review: reviewed bd26eb5 against main 72f3f44. No remaining actionable findings. The upstream merge brings #261's backend max_model_len limits; all FPM evaluator, FPM tests, and FPM Pages paths are byte-identical to the previously reviewed a0cb4e4, whose admitted Full CI passed all 54 validation jobs. Both reported findings remain fixed and resolved. Local current-head FPM/Pages validation: 234 passed, 4 compiled-extension-dependent cases skipped, 132 subtests passed. Fast CI is green on this head. Refreshing CodeRabbit coverage before admitting this exact commit to Full CI.
|
@coderabbitai full review Please refresh coverage for bd26eb5 against current main 72f3f44. This is an upstream merge only; the eight PR files remain unchanged from your previous full review. |
|
✅ Action performedFull review finished. |
jasonqinzhou
left a comment
jasonqinzhou
left a comment
There was a problem hiding this comment.
Approved at the user's explicit request after reviewing bd26eb5 against current main 72f3f44. Both reported findings are fixed and resolved; no remaining actionable findings. CodeRabbit's full review covers this exact commit and reports no actionable comments. Fast CI and Pages passed, and local FPM/Pages tests passed (234 passed, 4 local compiled-extension skips, 132 subtests). Merge remains conditional on successful required Full CI for this same head, a current base, and resolved conversations.
|
/ok to test bd26eb5 |
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
jasonqinzhou
left a comment
jasonqinzhou
left a comment
There was a problem hiding this comment.
Codex current-head review: reviewed 7455422 against main 8e763d4. No remaining actionable findings. The main update merges #289's replay scheduling, telemetry, and resource-retention changes; it touches none of this PR's FPM paths. All eight PR files are byte-identical to the previous reviewed head bd26eb5, whose admitted Full CI has now passed all 54 validation jobs, including both native contract suites (2,036 passed, 5 unrelated integration-assigned skips, and 132 subtests on each architecture). Both reported findings remain fixed and resolved. Current-head local tests: 234 passed, 4 local compiled-extension skips, 132 subtests. Refreshing exact-head review and required CI after this upstream-only merge.
|
@coderabbitai full review Please refresh exact-head coverage for 7455422 against main 8e763d4. This is an upstream-only merge of #289; the eight PR files are unchanged from your review of bd26eb5. |
|
✅ Action performedFull review finished. |
jasonqinzhou
left a comment
jasonqinzhou
left a comment
There was a problem hiding this comment.
Approved at the user's explicit request after current-head review of 7455422 against main 8e763d4. CodeRabbit's full review covers this exact head with no actionable comments. Fast CI and Pages passed, current local FPM/Pages tests passed, and both reported findings remain fixed and resolved. The upstream merge leaves all eight PR files unchanged. Merge remains conditional on successful required Full CI on this exact head, resolved conversations, and an up-to-date base.
|
/ok to test 7455422 |
2 participants
Why and what changed
The production FPM campaign aborted on the newly published Kimi-K3
tp8-dcp8library because its sidecar included an unrecognized DCP identity field. This change preserves DCP separately from CP across the manifest, sidecar, Parquet rows, and evaluator worker configuration. Missing legacy DCP means one; missing or mismatched non-default identity fails closed.Native FPM reports
dcp > 1as unsupported while retaining measured coverage and worker regression. The native-only guard is applied in the predictor adapter so current main's canonical regression constructor remains usable. The branch includes main at8e763d4edfd8555240157c2794aa61a27d73b8f8, including CI dependency setup fixes.Review map
scripts/fpm_accuracy/hf/dataset.pyfor identity validation;models/aic_predictors.pyfor native-FPM capability handling;tests/fpm_accuracy/test_evaluation.pyfor coverage and canonical/older-wheel dispatch.decode_context_parallel_size=1; public coverage uses the existingunsupported_predictorstate. No native DCP capability is introduced.Evidence
Reviewed head:
745542254adcdda82d6ba425dca7f4b8c00ed670.python -m pytest -q -p no:timeout tests/fpm_accuracy tests/test_pages_site.py— 234 passed, 4 skipped, 132 subtests passed. Four cases need the compiled extension; both hosted installed-wheel contract suites passed them on this exact head in admitted Full CI (2,040 passed, 5 unrelated integration-assigned skips, and 132 subtests per architecture; package/compatibility checks added 149 passes per architecture).ruff check --config python/aisimulate/pyproject.toml scripts/fpm_accuracy tests/fpm_accuracy;ruff format --check --config python/aisimulate/pyproject.toml scripts/fpm_accuracy/models/aic_config.py scripts/fpm_accuracy/models/aic_predictors.py tests/fpm_accuracy/test_evaluation.py;git diff --check;python3 scripts/check_packaged_legal_files.py— all passed.745542254adcdda82d6ba425dca7f4b8c00ed670. The trusted copy SHA matches the reviewed PR head. Release staging was correctly skipped for this PR run.745542254adcdda82d6ba425dca7f4b8c00ed670— full review completed with no actionable comments.745542254adcdda82d6ba425dca7f4b8c00ed670; no remaining actionable findings; all review threads are resolved.{ "dcp": 8, "measured_count": 12, "before": {"regression_status": "unsupported_predictor", "predicted_count": 0, "unavailable_count": 12}, "after": {"regression_status": "evaluated", "predicted_count": 7, "unavailable_count": 5}, "native_fpm": {"status": "unsupported_predictor", "predicted_count": 0, "unavailable_count": 12, "error_count": 0} }Modeling or data provenance
The earlier hosted campaign completed all 21 configurations at HF revision
6fad3f9a0df5a24603108dcea0d201259254b904, using evaluatorfc58a3f007aaf9458b345bbd162ae3c8b8c45edfwith the original DCP identity fix. Its downloadable artifact includes the machine-readable summary, qualification record, artifact identities, and coverage counts. The original validation covered 30 real Parquet/sidecar pairs.The Kimi-K3
tp8-dcp8configuration has zero measurements. That hosted result establishes campaign/schema handling, not DCP prediction accuracy. The current canonical-adapter follow-up is separately covered by fixture tests and current-head CI; the earlier campaign is not represented as a run of this new head. No latency model, performance dataset, or numerical golden is changed.Tracking