Repository navigation
feat(cli): add native timing and source detail reports - #272
Conversation
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com> # Conflicts: # tests/sweeper/test_search_providers.py
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
…mator-api Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
… data Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
…nfig Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
… identity Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
…r API Integrate PR #242 and retain the remaining AIC-1771 controls through canonical identity, KV sizing, candidate serialization, and native replay. Reuse existing context and memory controls and reject unsupported combinations. Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
Build AIC-1771 on the canonical estimator API at 1291c77. Preserve the existing control integration, reconcile migration-guide coverage, and document both public Rust literal updates. Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 selected for processing (9)
🔗 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; 8 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (9)Preserve the Rust single oracle: Python may describe operations, load raw data, orchestrate, and present results, but must not compute per-op performance values.⚙️ CodeRabbit configuration file Files:
Check unified CLI, Replay, Sweeper, and orchestration behavior together.⚙️ CodeRabbit configuration file Files:
Enforce the single-oracle and golden-diff rules in python/aisimulate/.claude/rules/rust-core/parity.md.⚙️ CodeRabbit configuration file Files:
Review scheduler state transitions and event ordering, including cancel, preempt, drain, retry, duplicate, empty, and terminal paths.⚙️ CodeRabbit configuration file Files:
Treat top-level exports and bindings as public and release boundaries.⚙️ CodeRabbit configuration file Files:
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:
Do not reintroduce them.📄 CodeRabbit inference engine (python/aisimulate/.claude/rules/rust-core/parity.md) 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)python/aisimulate/src/aisimulate/runner.py[info] 426-430: use jsonify instead of json.dumps for JSON output (use-jsonify) python/aisimulate/tests/cross_package/test_core_public_api.py[info] 373-383: 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 (9)
📝 SummaryRisk level: Medium Three areas need human attention:
Changed behavior and public contracts
Technical qualityThe supplied evidence reports 1,503 Rust tests and 422 Python checks. Review severity counts are unavailable because no current finding list was supplied. The shell results did not provide additional diff or test evidence. Merge readinessMerge readiness is not established. Exact-head Fast CI was pending, and Full CI was deferred pending parent change WalkthroughAdds end-to-end static operation diagnostics from the Rust engine through replay and the Python CLI. It defines SOL and fallback evidence, validates and accumulates timing details, exposes time and source sections, and updates schemas, tests, and documentation. ChangesStatic performance diagnostics
Priority: ➖ Normal Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
Comment |
|
@coderabbitai full review |
|
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
|
@coderabbitai review |
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
|
The parent branch advanced to 7301939 with the DeepSeek V4.1 changes from #159. This introduces a content conflict in crates/core/src/python.rs at the native replay boundary. I am integrating the new parent and will retain both its architecture-specific timing/storage behavior and this PR’s opt-in diagnostic capture, then refresh validation on the resulting head. |
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
…272-review-closeout Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com> # Conflicts: # crates/core/src/python.rs
|
@coderabbitai review |
|
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
jasonqinzhou
left a comment
There was a problem hiding this comment.
Codex fresh self-review of 4cb1eae2a7059b5c8aade2e7ef1ed07fef558faa against current parent 7301939e0efc2c919b090102306240b9c64789e1 found no remaining actionable issue. The parent introduces DeepSeek V4.1 support and changed the native replay binding. I resolved all three conflicts by preserving both diagnostic_model and decoder_replay, then checked the submitted diff and the affected canonical-model, phase-query, decode-coordinate, prefill-validation, and report paths.
A new focused regression proves that diagnostics and SOL retain the parent's bounded decoder-prefill geometry for queries around its window boundary, while decode work remains unchanged. It uses independent memory-probe token counts. All 1,503 Rust library tests passed (1 ignored), including this regression; the native extension rebuilt successfully. The expanded Python CLI/replay/AFD/estimator/API/SDK/DeepSeek-bridge validation is running.
All earlier bot/self findings remain fixed and resolved; the human stub finding remains fixed and answered for owner closure. Current-head Fast CI and CodeRabbit review precede the next Full CI dispatch. Earlier successful CI runs are historical evidence and do not replace validation after this parent integration.
…272-review-closeout Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
|
@coderabbitai review |
jasonqinzhou
left a comment
There was a problem hiding this comment.
Codex fresh self-review of ef9d16f55d5b4cac18b6f5830ce4059baa00ef89 against parent 6eab22fc8a2c38fd941d98344e98318be6c77c29 found no remaining actionable issue. I re-read the submitted timing/source diff and public/native/adapter/schema/documentation boundaries, and reviewed the latest parent changes for decoder-replay validation. This merge changes only four inherited Python files; the Rust tree is identical to the one with 1,503 passing tests (one ignored), and the native extension remains current.
The expanded preceding Python run passed 422 tests. On this head, all 286 affected runner, DeepSeek bridge, and CLI checks pass, together with Ruff checks and formatting. Existing bot/self threads are fixed and resolved. The human type-stub thread is fixed and answered, awaiting its owner's re-review/closure.
Exact-head Fast CI and CodeRabbit review are pending. Full CI is intentionally deferred until parent #269 settles, per the requested sequencing. This is a review checkpoint, not a readiness or merge verdict.
✅ Action performedReview finished.
|
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
jasonqinzhou
left a comment
There was a problem hiding this comment.
Codex fresh integration review of 92866bf7917444269539af99fa6e28e16819b1b1 against parent 5735cd22d21f329f42eefcdca43d05e36c359ed1: no remaining actionable finding. Parent #269 completed Full CI successfully at this base, so its final lightweight-import fix is now integrated. The complete feature diff against the parent is byte-identical to the previously reviewed diff (SHA-256 e657bf480b3baa32004685915d88b813a09469b5d81f8ec0721b882ca745f45b).
I rechecked the inherited change and the affected configuration, supervision, decoder-replay validation and CLI boundaries. Importing supervision/argument/output setup remains lightweight; requesting decoder replay still validates the canonical model/backend. All 287 affected Python tests pass, including the subprocess import-contract test, together with Ruff lint/format and diff hygiene. Rust source is unchanged from the 1,503-pass, one-ignored run and rebuilt native extension.
Existing bot/self findings remain fixed and resolved. The human stub thread remains fixed and answered for owner closure. Exact-head Fast CI and CodeRabbit review are requested. Full CI will follow their completion; this is not yet a readiness verdict.
|
PR #269 has squash-merged as I will record the squash commit in this branch's ancestry while preserving the complete tested tree, verify that the timing/source diff remains identical, and refresh exact-head review and CI. Full CI on the preceding |
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
jasonqinzhou
left a comment
There was a problem hiding this comment.
Codex exact-head ancestry/integration review of ebc898ec6aa43181c17b085cbe81287426e452e5 against main 7cfa4f20d9cda458eaa1febad7e82a488784518b: no remaining actionable finding. The main squash commit has the exact tree of the already integrated parent. Recording its ancestry preserves the complete tested tree (d8af2f0545821430a3e3535d61fc1ff525a5ff1c) and the complete 25-file feature diff (SHA-256 e657bf480b3baa32004685915d88b813a09469b5d81f8ec0721b882ca745f45b). Current main is now an ancestor of this head.
The preserved tree passed Full CI 35412879730 (54 successful jobs, one expected release-staging skip), 287 affected Python checks, and the previously recorded native/expanded Python validation. The live native method/type-stub contract test was rerun on this head and passed. No code or output behavior changed in this ancestry repair. Exact-head Fast CI and review are being refreshed before Full CI; historical checks are not substituted for required current-head checks.
The human type-stub finding is fixed in the shipped native stub and covered by the live signature/return regression. I verified normal platform permission to resolve that addressed thread; all applicable human/CODEOWNER approval requirements remain in force. This review is not an approval.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/ok to test ebc898e |
|
Merged and verified: tested head Trusted Fast CI 35414892097 and Full CI 35414892260 passed on the exact head. The manual complete-matrix Full CI 35413948520 also passed. All review threads are resolved and required approval/CODEOWNERS gates were accepted. The description now records the final validation evidence, and the merge guard has been stopped. |
Behavior
predict --detail timeadds native accumulated prefill/decode and per-operation timing, SOL latency/compute/memory comparisons, and latency/SOL ratios alongside serving latency statistics.--detail sourceadds operation source tags and executed MoE measurement substitutions.allselects all five sections;--detail-top-nlimits tables while JSON stdout andprediction.jsonretain complete, matching evidence.Evidence comes through the canonical
ForwardPassPerfModelAPI and is captured only when requested. Missing SOL implementations preserve actual timing and provenance with an explicit reason. Whole-model FPM, fixed/polynomial timing, analytical overlays, and adapters without the native export report unavailable operation evidence.Review map and contracts
crates/core/src/perfmodel/engine/runtime.rsandfpm/model.rs, thencrates/core/src/python.rs,engine/timing.rs, andpython/aisimulate/src/aisimulate/detail.py.static_phase_diagnostics,ReplayOutputRequirements.capture_performance_diagnostics, timing/source JSON, and optionalTimingOperationEvidence.details. Existing Rust struct literals initializedetails: None; the constructor does so automatically. The Python wrapper follows the current parent package location,aisimulate_core.--diagnostics-top-nremains an alias. No performance data or frozen golden values changed.Validation
Final reviewed head:
ebc898ec6aa43181c17b085cbe81287426e452e5. Squash-merged into main as7ae483e942b0a2593d55e4e30957f94640580441on 2026-09-19 at 02:23:16 UTC. The live main tip is the merge commit, and its tree exactly matches the tested head (d8af2f0545821430a3e3535d61fc1ff525a5ff1c).Modeling boundaries
Rust remains the only estimator. Operation totals represent accumulated scheduled rank-local work, not request latency, critical-path duration, or whole-deployment GPU time. SOL compares the same scheduled shapes and is not scaled by synthetic speedup. Source records are native classifications and executed substitutions, not full file/row lineage or an estimator-construction audit. Energy publication behavior is unchanged.
Related: #269 (merged prerequisite), #242 (canonical API), #19 (superseded).