Repository navigation
fix(simulation): upgrade AISimulate to 0.12.0.dev1 - #14541
Conversation
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>
|
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 ignored due to path filters (3)
📒 Files selected for processing (25)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughChangesAIC replay and mocker changes
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The dependency and replay configuration updates have no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 35.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 20 files. (5 skipped: 5 unsupported.) Full details: Description checkExplanation The description gives detailed upgrade scope, compatibility changes, package verification, and validation results. However, it omits the required Related Issues section and does not identify where reviewers should start.
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Comment |
MatejKosec
left a comment
There was a problem hiding this comment.
No actionable frontend findings identified in this diff. Reviewed the affected AIC backend-version resolution and mocker adapter paths by source inspection; tests were not run.
MatejKosec
left a comment
There was a problem hiding this comment.
Approved from the frontend review scope. No actionable findings in the affected AIC backend-version resolution and mocker adapter paths. This was source inspection; tests were not run.
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Review: fix(simulation): upgrade AISimulate to 0.12.0.dev1
What this is for: bump the AISimulate Python wheel 0.1.0.dev2 -> 0.12.0.dev1 and the Rust crate 0.1.0-dev.2 -> 0.12.0-dev.1, and carry the Planner/Profiler/Replay/mocker changes the new release's APIs require.
Verified by execution
Installed aisimulate==0.12.0.dev1 from PyPI (macOS arm64 wheel, CPython 3.12) and ran lib/bindings/python/src/dynamo/_internal/aic.py from this PR's tree against it. Separately downloaded aisimulate-core 0.12.0-dev.1 from crates.io; its SHA256 matches all three lockfiles (d4db6a08...b652). cargo test -p dynamo-mocker --no-default-features on macOS arm64: 209 passed, 0 failed (the author reports 213 on Linux with default features).
The premise holds. The PR says older hard-coded versions fail against the new queryable slots. Measured:
estimate_num_gpu_blocks(backend_version=None -> "current") vllm 3948 | sglang 3702 | trtllm 4365
estimate_num_gpu_blocks(backend_version="0.19.0"/"0.5.10"/"1.3.0rc10") all -> ValueError
aiconfigurator_core.sdk.perf_database.resolve_query_version accepts only current/previous/next or a literal already in the slots; aisimulate.aic.DEFAULT_BACKEND_VERSIONS inside the new wheel is still {vllm: 0.19.0, sglang: 0.5.10, trtllm: 1.3.0rc10} and falls back to it when backend_version is None — so the guard added in components/src/dynamo/replay/config.py is load-bearing, not defensive. Good catch.
Other things I checked and found correct, so they are not findings:
"current"resolves on every shipped system that has data for the backend (h100/h200/b200/b300/gb200/gb300/l40s/rtx_pro_6000/a100_sxm/b60-vllm). It raises ona100_pcie,a30,h100_pcie,l4andb60+sglang/trtllm — but those have no perf data for the backend at all, so the pre-PR pins failed there too. No regression.build_aic_engine(lib/bindings/python/rust/llm/aic_callback.rs:185) does not routeNonethroughresolve_backend_version, which looked like a hole. It is not:compile_engine(..., backend_version=None)and... "current"return byte-identical specs (2446 bytes each).ReplayEngineConfig.num_gpu_blocks_is_explicit: Nonematches the upstream struct default; theexecute_jsonfallback (role_capacity_is_explicit) is the intended path and Dynamo's direct-API callers never read it.best_available_overlap_blocks: the eligibility filter matches whatany_eligible_worker_rankenumerates for this router (replay_worker_configgives every workerdata_parallel_start_rank=0,data_parallel_size=dp_size == self.dp_size;pinned_worker/affinity_targetare hard-codedNoneinscheduling_request). Units line up too — in the offline patheffective_overlap_blocksis built 1:1 fromoverlaps.scores, sooverlap_regret_blockscannot go negative-then-saturate.kv_cache_bytes_per_tokenis plumbed but never populated by Dynamo. Upstream only reads it undernative_host_offload, which Dynamo never sets — inert, not a defect.- The new
filterwarningsentry matches the real message (aiconfigurator/deprecation.py:53) forsweep_aggandsweep_disagg, and does not matchsweep_afdorwarn_legacy_cli. That is exactly what the description claims. SweepResult.candidatesislist[CandidateRecord]andCandidateRecord.statusexists, so thetest_simulation_integration.pychange is right.
Findings
Three, all non-blocking, all the same root cause — the hard-coded-version sweep is incomplete. Details inline.
- P2 —
benchmarks/mocker/bench_aic_concurrency.py:42andbench_aic_rust_callback.py:68still pin0.19.0; both reach a real DB lookup and now raise. No CI covers them. - P2 —
--aic-backend-versionhelp text (components/src/dynamo/mocker/args.py:347),mocker-cli-reference.mdx:170andprotocols.rs:596name only versions that are now rejected. - P3 — literal-vs-alias asymmetry in the updated pins, plus a stale dead duplicate of
AIC_PARITY_VERSIONSinlib/bindings/python/tests/replay/replay_utils.py:41.
CI triage — not findings, you already see these
Recording the determination so it does not get re-litigated:
- All four image builds (
planner,vllm-runtime,sglang-runtime,trtllm-runtime) fail identically at therequirements.aisimulate.txtdownload:Could not find a version that satisfies the requirement aisimulate==0.12.0.dev1 (from versions: 0.1.0.dev1, 0.1.0.dev2). The build resolves through the internal index, which has not mirrored the new wheel; public PyPI has it. One qualification on the description: "the container's published-wheel download command ... passed" is true against public PyPI but not against the index CI uses, so "the release-publication dependency blocker is resolved" is not yet true for the build that gates this PR. Mirroring, not a code change. dynamo-runtime / test / sequentialis red on both arches for one root cause —tests/router/test_router_e2e_with_mockers.py:100still pins"aic_backend_version": "0.14.0", which the new slot gate rejects;test_router_decisions_disagg_router_aicis the same failure one hop later. Same class as finding 1, and the reason I am reading the sweep as incomplete rather than as two isolated nits.backend-status-check/dynamo-status-checkare aggregators of the above.
Not approving yet
Nothing I found is P0/P1, and three P2/P3 would normally clear my bar. I am holding approval only because finding 1, the router-test pin, and the index mirroring are one unfinished piece of work rather than independent nits — I would rather see the sweep land complete. Happy to re-review and approve as soon as the remaining constants move; ping me and I will re-run the probes above against the new head.
Reviewed at cfb95d4f3fb636d687ec19e4a14e38af38e50fa4. Where verification stopped: I ran nothing on GPU and did not build any container image, so the runtime-image and GPU-stage behaviour of this bump is unverified by me.
Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approving — round 2
All three findings from my first pass are fixed and re-verified by execution, and 764685344c went further than I asked: it also moved tests/router/test_router_e2e_with_mockers.py off 0.14.0 and collapsed the literal/alias mix to current across every live call site.
Re-verified against aisimulate==0.12.0.dev1 (macOS arm64 / CPython 3.12), for every (system, backend) pair the changed sites use:
h200_sxm vllm current -> 0.24.0 estimate_num_gpu_blocks -> 3948
h200_sxm sglang current -> 0.5.14 -> 3702
gb200 trtllm current -> 1.3.0rc20 -> 4365
Also re-grepped the whole tree: the only remaining literal AIC versions are tests/wheels/smoke_install.py (0.24.0, correctly kept — it globs that literal data directory), and three pure-plumbing fixtures that never reach a database lookup (mocker/tests/unit/test_config.py, replay/tests/test_simulation.py:379, router/tests/test_router_sweep_config_provider.py:284). Those are opaque strings; leaving them is fine. The replay_utils.py deletion is clean — no dangling references, and test_replay_smoke.py's import list touches no removed name.
I got the image-build triage wrong
My summary said the internal index had not mirrored the wheel. That was wrong, and @tedzhouhk's diagnosis is right: 0.1.0.dev2 ships manylinux_2_28 wheels and 0.12.0.dev1 ships manylinux_2_34, so on the 2.28 builder the new wheels were filtered out on platform tag. pip's from versions: ... lists platform-compatible candidates, not index contents — I read it as the latter. It would have reproduced against public PyPI too. Corrected in the thread above.
One new P2, non-blocking
container/templates/wheel_builder.Dockerfile:655 — ${PYTHON_VERSION} is not in scope in runtime_wheel_builder (the ARG is declared in wheel_builder_base and does not cross the FROM; this is why the stage re-declares ARG TARGETARCH at 576). It expands to empty, and pip treats an empty --python-version as "not provided" by design. The staging still works — I ran both forms against PyPI and they fetch the same wheel — so the flag is inert rather than wrong. One line to make the stated guarantee real. Details inline.
Approving with that outstanding: it does not change what gets staged, and I would rather not hold the bump for it.
Where verification stopped
- No GPU, no container image built. The runtime-image and GPU-stage behaviour of this bump is unverified by me, including whether
vllm/vllm-openai-xpu:v0.27.1clears the glibc 2.34 floor (every other base incontainer/context.yamldoes — ubuntu 24.04 / 22.04). - I could not re-run
cargo test -p dynamo-mockeron this head: my local toolchain broke mid-review on an unrelated Xcode licence prompt (ld: library 'c++' not found). It passed 209/209 oncfb95d4f3f, and the onlylib/mockerchange since is the doc comment inprotocols.rs, which cannot change behaviour. - CI status is not part of my bar and the remaining red is yours to land; I have not re-triaged it since
93853600d7.
Summary
Upgrade the published AISimulate Python dependency from
0.1.0.dev2to0.12.0.dev1and the Rust dependency from0.1.0-dev.2to0.12.0-dev.1, with the compatibility changes needed by Planner, Profiler, and Replay. Older hard-coded backend versions fail against the new queryable database slots; the KV geometry and replay API changes also require updates to Dynamo's adapter.manylinux_2_34platform, Python version, and ABI explicitly so themanylinux_2_28builder can stage them.currentslot, including replay capacity estimation and profiler-generated mocker arguments; preserve explicit versions.SweepResult.candidatesand update native AIC/Planner/Profiler regression coverage. DGDR continues using the supported legacy sweep API pending its migration; pytest ignores only the twosweep_agg/sweep_disaggdeprecation messages, while unrelated warnings remain errors.Published package verification
The release-publication dependency blocker is resolved:
aisimulate==0.12.0.dev1and Rustaisimulate-core=0.12.0-dev.1are both published. The wheel's Python/core versions and embedded Rust SBOM agree with this version pair.82d4b96674. This isrelease/0.12.0tip40daf5ea55plus version-stamping/build-test updates, with no additional functional source changes.src/match that source commit.Validation
Review follow-up at
93853600d7:manylinux_2_28container: downloads succeeded for both amd64 and arm64 target wheels. Rendered all four consuming targets (Planner, vLLM, SGLang, TensorRT-LLM).test_router_decisions_disagg[no_bootstrap]control reproduces the same timeout. This is not counted as a pass; CI must recheck the disaggregated path.fern checkhas zero errors. The full-site broken-link check reports an unchanged link indeepseek-v4-pro-0813.mdx:254, outside these changes.Earlier validation of the dependency upgrade:
Fresh validation uses the published Python wheel and a newly built Dynamo runtime linked against the published Rust crate, from this PR's source:
cargo test -p dynamo-mocker --locked: 213 passed.cargo fmt --all -- --check, andgit diff --checkpassed.Full backend-image/GPU CI was not run locally; these counts describe the targeted local regression runs, not full repository CI.
Summary by CodeRabbit
New Features
Updates