Repository navigation
Conversation
… [AIC-1811] Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@nvidia.com>
…811] Signed-off-by: Harry Lee <harrli@nvidia.com>
…811] Signed-off-by: Harry Lee <harrli@nvidia.com>
…-1811] Signed-off-by: Harry Lee <harrli@nvidia.com>
…IC-1811] Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@nvidia.com>
Synchronize the stacked branch with the coordinated 0.13 version boundary and compile exhaustive downstream report literals. Record the unresolved methodology attribution gate without changing runtime behavior or claiming license clearance. Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@nvidia.com>
Resolve driver imports and the CLI parser relocation while preserving snapshot identity/timing and all tests from both branches. Adapt the public API import to the renamed Rust crate and give the snapshot CLI test a real trace for the new resource preflight.
Resolve Rust, CLI, release, and CI conflicts while preserving the prepared warmup/P-D contract and upstream resource and batch reporting. Retain preparation admission evidence until the barrier and measure bounded profile reports from the profile epoch. Cover summary/detailed report parity, bounded reclamation, and canonical model identities.
Preserve warmup, barrier, offline P/D, and bounded profile reporting while merging main 3bfdbf8, including the squashed AIC-1811 prerequisite. Retain both sides of the capability/API/test additions and update the fixed-timing regression for the removed external decode 1 ms floor. Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@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
📜 Recent review details
📝 Summary
Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check |  |
The changeset includes substantial agentic-profile implementation, allocation-reuse work, source-digest changes, and broad packaging and CI changes beyond the stated primary objective of native conver… | Split unrelated agentic-profile, allocation-reuse, and independent infrastructure changes into separate pull requests, or expand the PR objectives and review map to justify each change and define its contract with the Dynamo-routing work. |
| Description check | |
The description explains the intended routing behavior and includes evidence, but it does not follow the required template completely. It omits the Modeling or data provenance and Tracking sections, a… | Add all required template sections and fields. Include Modeling or data provenance with N/A if not applicable; add Tracking with issue links and closing keywords; complete the Review map fields; and list exact test commands and results, Fas… |
-
Check name Status Explanation Cross-Layer Contract No stale cross-layer contract was found. The router and affinity schema is validated in Python and Rust, carries through the adapter and native runner, and is covered by provider and installed-CLI tes… Modeling And Data Evidence The PR supplies reproducible before/after evidence and held-out comparisons for its model/selection logic claims. Evidence provided for selection logic and cache behavior: 1. **Parametrized tests … Compatibility Boundaries The pull request maintains compatibility boundaries for Python and Rust public surfaces, schemas, versions, bindings, and documented defaults. Evidence confirms: Version Synchronization: - Python … Review Evidence The PR description names relevant commands and results: cargo +1.96.1 test -p aisimulate-core --lib(1708 passed),cargo +1.96.1 test --manifest-path crates/dynamo-policy/Cargo.toml --lib(9 passe…Linked Issues check The description identifies relevant dependency issues, related work, and pull requests, including the required Dynamo admission barrier and optional compatibility work. Title check The title clearly identifies the main behavioral change: running AgentX with native Dynamo routing interfaces. It is specific and avoids vague wording. Full details: Cross-Layer Contract
-
Explanation
No stale cross-layer contract was found. The router and affinity schema is validated in Python and Rust, carries through the adapter and native runner, and is covered by provider and installed-CLI tests. Profile defaults and constraints align between Python and Rust, and the profile report is serialized and retained by the Python runner; CLI, runtime, and documentation consumers are present. Conversation lineage is carried through snapshot identities, checks the schema before sibling routing, and preserves legacy JSON through a serialization test. The new placement lifecycle hooks are documented and tested in aggregated and disaggregated replay, including wakeup and failure paths.
Full details: Modeling And Data Evidence
-
Explanation
The PR supplies reproducible before/after evidence and held-out comparisons for its model/selection logic claims. Evidence provided for selection logic and cache behavior: 1. Parametrized tests with held-out controls:
test_yaml_selects_native_dynamo_with_real_affinity_and_cache()establishes a baseline by running the identical configuration with physical prefix caching disabled, then validatesfirst_admission_prefix_cache_reused_ratio == 0with all requests showing zero reused tokens. This held-out comparison (cold run) directly demonstrates cache functionality. 2. Machine-readable data summaries: The test assertions validate actual JSON report fields includingoverlap_blocks,best_available_overlap_blocks,physical_kv_events,decision_count,decisions_by_role, anddynamo_revision. The Rust tests verifyassert_eq!(chosen.cache_sample.unwrap().overlap_blocks, 4)andassert_eq!(next.cache_sample.unwrap().best_available_overlap_blocks, 0)— concrete numbers tied to physical cache state transitions. 3. Independent oracle via engine signals: The test compares routing decisions against actual engine routing history:assert decision["overlap_blocks"] == route["selected_overlap_blocks"]andassert decision["worker_id"] == route["logical_worker_id"], validating that native selection predictions match engine-executed choices. 4. Explained observations: The quickstart documentation explicitly discloses that the claimed numbers (219 successes, 95.65% prefix reuse, 482 decisions, 3,042 KV events, 258 MB peak) are "functional validation observations, not fixed expected counts, a general memory bound, or hardware accuracy claims; native stochastic selection can change the results." It provides the exact reproducible configuration (two-play Weka subset, seed 42, AIC timing, 4GB budget, two lanes, vLLM/SGLang) and invites repeated runs as comparison baseline. 5. Before/after coverage for compatibility fixes: The PR description documents specific corrections: "omitted native TTL previously failed deserialization and now runs real selection/cache reuse in both topologies" and "failed dispatch callbacks previously retained accepted engine ownership" — indicating the changes enable working behavior that was broken. Tests verify the affinity TTL defaults to 3600 seconds when omitted. What is NOT present but not required: The custom check requires provenance for "formulas, selection logic, model/performance data, or predicted outputs." The PR does not claim statistical models or hardware performance predictions; it uses deterministic engine simulation with stochastic routing decisions. The documentation appropriately disclaims hardware accuracy and invites users to run their own experiments with the provided reproducible configuration. The 9 native policy tests exercise affinity TTL handling, worker binding lifecycle, cache index observation, and clock precision without requiring external oracles. CLI acceptance tests parametrize across backends (vLLM/SGLang), topologies (aggregated/disaggregated), and affinity modes (session/sibling_group) across 16 cells, validating that native selection integrates correctly in all combinations.
Full details: Compatibility Boundaries
-
Explanation
The pull request maintains compatibility boundaries for Python and Rust public surfaces, schemas, versions, bindings, and documented defaults. Evidence confirms: Version Synchronization: - Python and Rust versions are synchronized at 0.13.0 for both base (
aisimulate,aisimulate-core) and optional (aisimulate-dynamo-policy,aisimulate-dynamo-policy) packages. - Rustcrates/dynamo-policy/Cargo.tomlpinsaisimulate-corewithversion = "=0.13.0"(exact). - Pythonpython/aisimulate-dynamo-policy/pyproject.tomlpinsaisimulate==0.13.0(exact). - Build scripts validate exact version matches before building wheels. Public API Surface Synchronization: -AgenticProfileOptionsis public in both Rust (crates/core/src/replay/loadgen/profile.rs) and Python (python/aisimulate/src/aisimulate/config/traffic.py). - Rust exportsAgenticIdleShiftandAgenticProfileReportviapub use profile::*incrates/core/src/replay/loadgen/mod.rs. - Python re-exportsAgenticProfileOptionsinaisimulate.config.__init__.pyvia__all__. -AgenticConversationLineagestruct andAGENTIC_CONVERSATION_LINEAGE_SCHEMA_V1constant are re-exported publicly incrates/core/src/replay/mod.rs. Documented Defaults Synchronized: - Rust defaults (fromcrates/core/src/replay/loadgen/profile.rs):duration_seconds: 3600.0,response_grace_seconds: 30.0,cancel_drain_seconds: 10.0,tree_idle_cap_seconds: 300.0,global_idle_cap_seconds: 10.0. - Python defaults (frompython/aisimulate/src/aisimulate/config/traffic.py): identical field defaults with matching types and ranges. - Affinity TTL default: both Rust (crates/dynamo-policy/src/lib.rsline 55-56:default_affinity_ttl_seconds() -> 3600.0) and Python (python/aisimulate-dynamo-policy/src/aisimulate_dynamo_policy/provider.py:ttl_seconds: float = Field(default=3600, ...)). AISimulate Remains Standalone from Dynamo: - Core Rust crate (crates/core/Cargo.toml) has no Dynamo dependency. - Base Python wheel (python/aisimulate/pyproject.toml) has no dynamo-policy dependency. - Dynamo dependency (dynamo-kv-routerat revd9eb42db1168131fdae318eef77255637e4d3495) is isolated tocrates/dynamo-policyonly. -crates/dynamo-policyis excluded from the root workspace:exclude = ["crates/tests/public-api", "crates/dynamo-policy"]. - Dynamo-policy has its own separate lockfile atcrates/dynamo-policy/Cargo.lock. Artifact Contract Updated Correctly: -docs/artifact-contract.mdis updated to document the new optionalaisimulate-dynamo-policywheel as a separately built adapter. - Documentation clarifies that the base release remains unchanged and dynamo-policy is source-built (not published to an index). - Contract specifies exact version alignment between base and optional wheels, matching Rust versions, and synchronized source digests. - Build scripts (scripts/build_dynamo_policy.py,scripts/apply_dev_version.py,scripts/build_release_artifacts.py) validate all version and dependency contracts before writing artifacts. API Contract Versioning: -native_replay_contract()in core exportsapi_version: 1,core_version, andcore_source_sha256. - Dynamo-policy wrapper validates both core and plugin contracts report matching API version 1, identical versions, and identical core source SHA256. - Build-time source digest (AISIMULATE_CORE_SOURCE_SHA256) is computed incrates/core/build.rsand compared at runtime. Bindings Synchronized: - Python runner (python/aisimulate-dynamo-policy/src/aisimulate_dynamo_policy/runner.py) validates all version synchronization at import time before any replay execution. - Entry points registered correctly:aisimulate.runner_factories:dynamo-policyandaisimulate.config_adapters:dynamo-policy.router. All explicit compatibility requirements are met; no unintended cross-layer dependencies exist, and the artifact contract boundaries are preserved with deliberate documentation updates.
Full details: Review Evidence
-
Explanation
The PR description names relevant commands and results:
cargo +1.96.1 test -p aisimulate-core --lib(1708 passed),cargo +1.96.1 test --manifest-path crates/dynamo-policy/Cargo.toml --lib(9 passed), installed wheels (60/60 + 28/28 tests), container (88 tests), and manual execution (219 successes). Negative and boundary validation is extensive. The PR documents 17 invalid stamp inputs, 12 commit-failure cases, 2 engine-reject/abort cases, and tests for invalid/mixed installs, wakeup timing, profile cutoff, retracted work, cancellation, dispatch failure atomicity, and invalid manifests/pins/locks. Evidence is categorized by testing tier: local unit tests (cargo test), installed wheel integration tests (CLI/provider/admission), container system tests (clean archive), and manual end-to-end execution (Weka trace). The PR does not cite inactive CI workflows as evidence. Although the evidence section mentions "Fast CI / Full CI: final 90551ea checks are running" and acknowledges "Final-head hosted checks... were still being verified or obtained," it does not present specific CI test results as proof. All actual evidence cited—1708 passed, 9 passed, 60/60, 28/28, 88 tests, 219 successes—comes from local, installed, container, or manual sources, not CI workflows.
-
Full details: Out of Scope Changes check
-
Explanation
The changeset includes substantial agentic-profile implementation, allocation-reuse work, source-digest changes, and broad packaging and CI changes beyond the stated primary objective of native conversation-aware Dynamo routing.
Resolution
Split unrelated agentic-profile, allocation-reuse, and independent infrastructure changes into separate pull requests, or expand the PR objectives and review map to justify each change and define its contract with the Dynamo-routing work.
Full details: Description check
-
Explanation
The description explains the intended routing behavior and includes evidence, but it does not follow the required template completely. It omits the Modeling or data provenance and Tracking sections, and the Review map does not provide the required public-contract and compatibility details. Several required evidence fields are also missing, including exact commands, Fast CI, Full CI, and Codex-reviewed commit information.
Resolution
Add all required template sections and fields. Include Modeling or data provenance with N/A if not applicable; add Tracking with issue links and closing keywords; complete the Review map fields; and list exact test commands and results, Fast CI status, Full CI status, CodeRabbit-reviewed commit, Codex-reviewed commit, negative or boundary cases, expected-value derivations, and before/after outputs where applicable.
Comment @coderabbitai help to get the list of available commands.
Signed-off-by: Harry Lee <harrli@nvidia.com>
Harrilee
changed the title
Preserve direct snapshot compilation, shared hash storage and streamed report serialization alongside native lifecycle and conversation lineage hooks. Carry prediction provenance through typed execution results and repair stack propagation in the extracted resource estimator. Signed-off-by: Harry Lee <harrli@nvidia.com>
Retain complete report, request and lifecycle digests for all 16 scenarios, and assert fixture ancestry and the lineage schema across snapshot, preparation and request identities. Raw output comparison against main and the pre-merge PR confirms that the only additions are lineage fields; all previous values and lifecycle bytes are unchanged. Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@nvidia.com>
Harrilee
changed the title
Harrilee
changed the title
Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@nvidia.com>
Arsene12358
left a comment
Arsene12358
left a comment
There was a problem hiding this comment.
Request changes: 4 merge blockers
Thanks for the lineage and lifecycle hooks. On the built-in engine path they are regression-free. I compared this head with the merge base 88c7d426 on 13 end-to-end aisimulate predict configs (synthetic, Mooncake, agentic, disaggregated, AFD and Weka workloads): every config produces the same per-request results and the same prediction.json apart from the new lineage fields. The tools/simulation_perf_gate scenarios, run locally on macOS, are flat (geometric mean head/base 0.985). The native-router quickstart also runs end to end with only aisimulate installed: 42 requests, 64 routing decisions per pool, no dispatch aborts.
It still can't merge because of the four issues below. B1–B3 each come with a failing example, and B4 needs a maintainer decision. Everything was reproduced on head e407510d.
B1: the wheel only builds against an unmerged Dynamo draft
crates/python/Cargo.toml:27-29 pins dynamo-kv-router, dynamo-custom-policy-builtin and dynamo-tokens to 85ce615a, and the wheel is built from that crate (python/aisimulate/pyproject.toml:114). 85ce615a is the head of ai-dynamo/dynamo#16490: a draft with no reviews, one commit on top of Dynamo main's f5c36597 on branch harrli/aic-1817-admission-barrier. The router calls the method that draft adds (crates/python/src/native_router/router.rs:733).
Failing example: pin the same three crates to any merged Dynamo revision, either the commit #16490 is based on or current Dynamo main:
sed -i '' 's/85ce615a409d61144c5b94ce74dae3f7291573dd/f5c36597428cae20a0c4a0c7cecf73eec79e1c0b/g' crates/python/Cargo.toml
cargo check -p aisimulate-pythonerror[E0599]: no method named `admission_barrier` found for struct `Arc<LocalScheduler<NoopSequencePublisher, ReplayWorkerConfig, DefaultWorkerSelector>>` in the current scope
--> crates/python/src/native_router/router.rs:733:42
|
733 | .block_on(self.scheduler.admission_barrier())??;
| ^^^^^^^^^^^^^^^^^ method not found in `Arc<LocalScheduler<NoopSequencePublisher, ReplayWorkerConfig, DefaultWorkerSelector>>`
error: could not compile `aisimulate-python` (lib) due to 1 previous error
Dynamo rev in crates/python/Cargo.toml |
cargo check -p aisimulate-python |
|---|---|
85ce615a (draft #16490, this PR) |
OK |
f5c36597 (Dynamo main, parent of #16490) |
E0599 admission_barrier |
df2ab3a4 (current Dynamo main) |
E0599 admission_barrier |
If this merges now, every main and nightly wheel is built from Dynamo code nobody has reviewed. And if #16490 changes during its own review, this repo keeps building the old draft without noticing.
Fix: land ai-dynamo/dynamo#16490 first, re-pin these three crates to its merge commit on Dynamo main, then re-run the native-router validation. The PR body and quickstart should then name the merged revision.
B2: the PR's own test fails, so Full CI is red
tests/test_resource_supervision.py::test_supervisor_reports_default_or_explicit_routing_stack[None-dynamo] fails on both contracts shards for this head (amd64, arm64), with 1 failed, 4451 passed. Local repro:
$ pytest "tests/test_resource_supervision.py::test_supervisor_reports_default_or_explicit_routing_stack" -q
> assert plan["stack"] == expected
E AssertionError: assert 'engine' == 'dynamo'
FAILED tests/test_resource_supervision.py::test_supervisor_reports_default_or_explicit_routing_stack[None-dynamo]
1 failed, 2 passed
Cause: select_stack now returns "engine" whenever --stack is omitted (python/aisimulate/src/aisimulate/cli_args.py:139-143), but the parametrize at tests/test_resource_supervision.py:350 still expects "dynamo". With the one-line suggestion inline, all three cases pass.
B3: a policy wakeup is dispatched before same-instant engine work is settled
The new drain-loop code calls advance_clock and dispatches the released placements before settle_internal_work runs for the same timestamp: crates/core/src/replay/agg.rs:899-901 and crates/core/src/replay/disagg.rs:2627-2631. The existing comment right below the new block (agg.rs:902-904, "Settle idle deadlines first…") already describes the intended order. PlacementPolicy::next_wakeup_ms documents future wakeups as the normal case (crates/core/src/replay/core/mod.rs:132-139). So any policy whose wakeup lands on an engine-internal deadline, such as a native host-offload transfer completing, hits this.
Failing example: the test file below uses only the public API: run_replay_with_composition plus a minimal PlacementPolicy that holds one request until its wakeup. The control and coincident runs differ only in the wakeup time, which moves by 0.25 ms. Drop the file into crates/core/tests/ and run cargo test -p aisimulate-core --test pr306_wakeup_ordering -- --nocapture:
aggregated wakeup 5.25 ms (control): Ok completed=5 ["1=Completed", "2=Completed", "3=Completed", "4=Completed", "5=Completed"]
aggregated wakeup 5.00 ms (coincident): Err engine error: engine internal work is due at 5ms by command time 5ms; process internal work and retry the command
P/D wakeup 3.25 ms (control): Ok completed=2 ["1=Completed", "2=Completed"]
P/D wakeup 3.00 ms (coincident): Ok completed=1 ["1=Completed", "2=Failed"]
test result: FAILED. 0 passed; 2 failed
In aggregated mode the whole replay aborts. In P/D mode, request 2 is reported as Failed and no error is raised. The built-in native router only returns same-time wakeups, and its affinity mode refuses host/G3 offload, so the shipped CLI path doesn't reach this today. Any composition that uses the documented future-wakeup contract does, and those compositions are what these neutral hooks are for.
Fix: settle first, then advance the policy clock (inline on agg.rs and disagg.rs). With both changes applied on this head:
- the repro passes (2/2);
cargo test -p aisimulate-coreis green (2,028 lib tests plus all integration tests);cargo test -p aisimulate-pythonpasses 40/40.
crates/core/tests/pr306_wakeup_ordering.rs (public-API repro)
// Repro for ai-dynamo/aisimulate#306: a PlacementPolicy wakeup that lands on the
// same instant as engine-internal work (native host-offload transfers) is
// dispatched before that internal work is settled.
//
// Public API only. Drop into crates/core/tests/ and run:
// cargo test -p aisimulate-core --test pr306_wakeup_ordering -- --nocapture
use std::collections::VecDeque;
use aisimulate_core::engine::{EngineConfig, NativeHostOffloadConfig, TimingModelConfig};
use aisimulate_core::replay::loadgen::ReplayRequestPayload;
use aisimulate_core::replay::{
DirectRequest, NoEngineEvents, NoReplayMetadata, Placement, PlacementDecision,
PlacementEffects, PlacementPolicy, ReplayAdapters, ReplayCaptureOptions, ReplayComposition,
ReplayEngineConfig, ReplayEngineFactory, ReplayReport, ReplayResult, ReplayRoleConfig,
ReplayRuntimeInput, ReplaySpec, ReplayTopology, WorkerPoolSpec, WorkerTopology,
run_replay_with_composition,
};
use uuid::Uuid;
/// Places every request immediately on `route(id)`, except `queued`, which it
/// holds and releases to `target` at the absolute replay time `wakeup_ms`.
struct WakeupPolicy {
queued: Uuid,
target: usize,
wakeup_ms: f64,
route: fn(Uuid) -> usize,
pending: Option<Uuid>,
}
fn placement(request_id: Uuid, scheduler_id: usize) -> Placement {
Placement {
request_id,
scheduler_id,
reported_overlap_tokens: 0,
cache_sample: None,
placement_replica_id: None,
}
}
impl PlacementPolicy<ReplayRequestPayload> for WakeupPolicy {
type Metadata = NoReplayMetadata;
type Observation = ();
fn place(
&mut self,
request: &ReplayRequestPayload,
_: NoReplayMetadata,
_: Option<String>,
_: f64,
) -> anyhow::Result<PlacementEffects> {
let id = request.metadata().uuid.unwrap();
let decision = if id == self.queued {
self.pending = Some(id);
PlacementDecision::Queued
} else {
PlacementDecision::Immediate(placement(id, (self.route)(id)))
};
Ok(PlacementEffects { decision, released: Vec::new() })
}
fn next_wakeup_ms(&self) -> Option<f64> {
self.pending.map(|_| self.wakeup_ms)
}
fn advance_clock(&mut self, now_ms: f64) -> anyhow::Result<Vec<Placement>> {
if now_ms < self.wakeup_ms {
return Ok(Vec::new());
}
Ok(self.pending.take().into_iter().map(|id| placement(id, self.target)).collect())
}
fn observe(&mut self, _: (), _: f64) -> anyhow::Result<Vec<Placement>> {
Ok(Vec::new())
}
fn cancel_pending(&mut self, id: Uuid) -> bool {
self.pending.take_if(|pending| *pending == id).is_some()
}
fn request_terminal(&mut self, _: Uuid, _: f64) -> anyhow::Result<Vec<Placement>> {
Ok(Vec::new())
}
fn prefill_completed(&mut self, _: Uuid, _: f64) -> anyhow::Result<Vec<Placement>> {
Ok(Vec::new())
}
fn pending_count(&self) -> usize {
usize::from(self.pending.is_some())
}
fn worker_ready(&mut self, _: WorkerTopology, _: f64) -> anyhow::Result<Vec<Placement>> {
Ok(Vec::new())
}
fn worker_draining(&mut self, _: WorkerTopology, _: f64) -> anyhow::Result<Vec<Placement>> {
Ok(Vec::new())
}
fn worker_removed(&mut self, _: WorkerTopology, _: f64) -> anyhow::Result<Vec<Placement>> {
Ok(Vec::new())
}
fn topology_settled(&mut self, _: f64) -> anyhow::Result<Vec<Placement>> {
Ok(Vec::new())
}
}
/// Aggregated runs use `policy`; disaggregated runs use it for prefill and a
/// pass-through policy for decode.
struct OnePolicy(Option<WakeupPolicy>);
impl ReplayComposition for OnePolicy {
type Metadata = NoReplayMetadata;
type Observation = NoEngineEvents;
type AggregatedPlacement = WakeupPolicy;
type DisaggregatedPlacement = WakeupPolicy;
fn create_aggregated_placement(
&mut self,
_: u32,
_: Vec<WorkerTopology>,
) -> anyhow::Result<WakeupPolicy> {
Ok(self.0.take().unwrap())
}
fn create_disaggregated_placements(
&mut self,
_: u32,
_: Vec<WorkerTopology>,
_: u32,
_: Vec<WorkerTopology>,
) -> anyhow::Result<(WakeupPolicy, WakeupPolicy)> {
let decode = WakeupPolicy {
queued: Uuid::nil(),
target: 0,
wakeup_ms: f64::INFINITY,
route: |_| 0,
pending: None,
};
Ok((self.0.take().unwrap(), decode))
}
}
fn request(id: u128, arrival_ms: f64, tokens: Vec<u32>, max_output_tokens: usize) -> DirectRequest {
DirectRequest {
tokens,
max_output_tokens,
uuid: Some(Uuid::from_u128(id)),
arrival_timestamp_ms: Some(arrival_ms),
..DirectRequest::default()
}
}
fn run(
topology: ReplayTopology,
engine: ReplayEngineConfig,
requests: Vec<DirectRequest>,
policy: WakeupPolicy,
) -> ReplayResult<ReplayReport> {
let spec = ReplaySpec {
version: 1,
topology,
engine: serde_json::to_value(engine).unwrap(),
adapters: ReplayAdapters::default(),
max_sim_time_ms: None,
max_in_flight: None,
record_per_request: true,
sla: Default::default(),
requests: Vec::new(),
};
run_replay_with_composition(
spec,
ReplayEngineFactory::new(),
Some(ReplayRuntimeInput::Requests(VecDeque::from(requests))),
OnePolicy(Some(policy)),
ReplayCaptureOptions::default(),
None,
)
}
// Aggregated, 2 workers. Worker 1 offloads SEED to host when EVICT arrives,
// and RESTORE (same prompt as SEED) restores it at 4.0 ms, which leaves an
// engine-internal deadline on worker 1 at 5.0 ms. The policy releases
// QUEUED_FOR_B to worker 1 at `wakeup_ms`.
fn run_aggregated(wakeup_ms: f64) -> ReplayResult<ReplayReport> {
const SEED: u128 = 1;
const EVICT: u128 = 2;
const RESTORE: u128 = 3;
const COMPLETING_A: u128 = 4;
const QUEUED_FOR_B: u128 = 5;
let engine = ReplayEngineConfig {
rank: EngineConfig {
num_gpu_blocks: 1,
block_size: 4,
max_num_seqs: 2,
max_num_batched_tokens: 8,
enable_prefix_caching: true,
kv_cache_bytes_per_token: Some(250_000),
native_host_offload: Some(NativeHostOffloadConfig::new(2).with_bandwidths(1.0, 1.0)),
timing_model: TimingModelConfig::Fixed { prefill_ms: 1.0, decode_ms: 0.0 },
..EngineConfig::default()
},
..ReplayEngineConfig::default()
};
run(
ReplayTopology::aggregated(2),
engine,
vec![
request(SEED, 0.0, vec![1, 2, 3, 4], 0),
request(EVICT, 2.0, vec![5, 6, 7, 8], 0),
request(RESTORE, 4.0, vec![1, 2, 3, 4], 0),
request(COMPLETING_A, 4.0, vec![11, 12, 13, 14], 0),
request(QUEUED_FOR_B, 4.0, vec![21, 22, 23, 24], 0),
],
WakeupPolicy {
queued: Uuid::from_u128(QUEUED_FOR_B),
target: 1,
wakeup_ms,
route: |id| usize::from(id.as_u128() <= RESTORE),
pending: None,
},
)
}
// Prefill/decode, 1P + 1D. r1's prefill-side D2H to the cluster-shared host
// pool leaves an engine-internal deadline on the prefill worker at 3.0 ms.
// The policy holds r2 (arrives 2.5 ms) and releases it at `wakeup_ms`.
fn run_disaggregated(wakeup_ms: f64) -> ReplayResult<ReplayReport> {
let rank = EngineConfig {
block_size: 4,
num_gpu_blocks: 16,
max_num_seqs: 4,
max_num_batched_tokens: 64,
enable_prefix_caching: true,
kv_cache_bytes_per_token: Some(250_000),
timing_model: TimingModelConfig::Fixed { prefill_ms: 1.0, decode_ms: 1.0 },
..EngineConfig::default()
};
let mut prefill_rank = rank.clone();
prefill_rank.native_host_offload = Some(
NativeHostOffloadConfig::new(8).with_bandwidths(1.0, 1.0).cluster_shared("tp1"),
);
let engine = ReplayEngineConfig {
prefill: Some(ReplayRoleConfig { rank: prefill_rank, ..ReplayRoleConfig::default() }),
decode: Some(ReplayRoleConfig { rank, ..ReplayRoleConfig::default() }),
..ReplayEngineConfig::default()
};
run(
ReplayTopology::Disaggregated {
prefill: WorkerPoolSpec::default(),
decode: WorkerPoolSpec::default(),
handoff_latency_ms: 0.0,
},
engine,
vec![
request(1, 0.0, (1..10).collect(), 1),
request(2, 2.5, (100..109).collect(), 1),
],
WakeupPolicy {
queued: Uuid::from_u128(2),
target: 0,
wakeup_ms,
route: |_| 0,
pending: None,
},
)
}
fn describe(result: &ReplayResult<ReplayReport>) -> String {
match result {
Ok(report) => {
let statuses: Vec<_> = report
.per_request
.iter()
.map(|r| format!("{}={:?}", &r.uuid[r.uuid.len() - 1..], r.terminal_status))
.collect();
format!("Ok completed={} {statuses:?}", report.request_counts.completed_requests)
}
Err(error) => format!("Err {error:#}"),
}
}
#[test]
fn aggregated_wakeup_at_internal_deadline() {
let control = run_aggregated(5.25);
let coincident = run_aggregated(5.0);
eprintln!("aggregated wakeup 5.25 ms (control): {}", describe(&control));
eprintln!("aggregated wakeup 5.00 ms (coincident): {}", describe(&coincident));
assert_eq!(control.unwrap().request_counts.completed_requests, 5);
assert_eq!(coincident.unwrap().request_counts.completed_requests, 5);
}
#[test]
fn disaggregated_wakeup_at_internal_deadline() {
let control = run_disaggregated(3.25);
let coincident = run_disaggregated(3.0);
eprintln!("P/D wakeup 3.25 ms (control): {}", describe(&control));
eprintln!("P/D wakeup 3.00 ms (coincident): {}", describe(&coincident));
assert_eq!(control.unwrap().request_counts.completed_requests, 2);
assert_eq!(coincident.unwrap().request_counts.completed_requests, 2);
}B4: the dependency-boundary change needs an explicit maintainer decision
Re-adding crates/python makes the aisimulate wheel compile Dynamo's routing crates (crates/python/Cargo.toml:27-29, built via python/aisimulate/pyproject.toml:114). Dynamo main already depends on AISimulate: aisimulate-core = { version = "=0.13.0-dev.2" } in its workspace dependencies (Cargo.toml:62 at df2ab3a4), consumed by lib/mocker. So the two repos now depend on each other, and installing aisimulate alone provides Dynamo KV routing: the quickstart above ran in a venv without any Dynamo package.
That reverses what the open reviews asked for:
- tedzhouhk's change request (#306 (review)): "Please keep the neutral lineage/lifecycle hooks and the one-way dependency boundary".
- dreamtalen's thread (#306 (comment)) asked to keep the generic lineage and lifecycle extensions here and move the Dynamo-specific adapter, provider/runner and entry-point registration to Dynamo.
60fd1bb7("keep Dynamo routing in the existing Dynamo integration") did that, and this head re-adds it. REVIEW.md:134-137: "AISimulate is standalone. Dynamo is a downstream compatibility target, not a required runtime dependency. The release surface remains oneaisimulatewheel and oneaisimulate-corecrate unless the artifact contract is changed deliberately."
Fix: either keep the Dynamo adapter in Dynamo as requested, or record a deliberate decision: sign-off on this PR from the reviewers above, plus updates to REVIEW.md:134-137 and the PR body's release-surface notes.
Non-blocking
python/aisimulate/src/aisimulate/resources.py:910keys the router-memory guard ("Native routing retains additional state outside the engine estimate") onstack == "dynamo". Native routing now runs on theenginestack, so it gets the engine-only plan. On the quickstart corpus,--stack engineis planned asweka-materialized-v1and admitted at parallelism 4, while the same snapshot workload on--stack dynamogetsdynamo-unqualified-v1and is refused or serialized.- The advisory Forward Prediction Performance job flagged
qwen3-32bwarm generation (+11%). I reran the same harness (tools/forward_perf_gate/run.py, base88c7d426vs this head) on macOS arm64 without CPU pinning: PASS, 0 regressions in 128 comparisons,qwen3-32bwarm generation +0.3% / -0.2%. I'm treating the CI flag as runner noise.
Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@nvidia.com>
|
Addressed the review fixes in ddb93c8 and merged current main in 88e3aae, resolving all four source conflicts. B2 and both B3 threads are resolved; B1 (merged Dynamo pin) and B4 (reviewer agreement on the deliberate installation boundary) remain open. The non-inline memory fix applies unknown-peak, supervised-serial admission to engine-stack native routing at CLI preflight (including --set), per-run guards, and whole-wave admission. Initial materialization refusals and lower bounds remain enforced, and observed trace peaks cannot requalify parallel native-router runs. The main merge preserves tokenization across default and native/shared execution, with dispatch committed only after engine acceptance. Cancellation before engine ownership and delayed commit failure/rollback have regressions for vLLM and SGLang. Main's planned performance-model folding implementation is retained unchanged. Current merged-source validation uses a freshly compiled native extension (SHA-256 bc65a14ec2be0d484d6365fd5808f85845ba5eafa2501210e69e63c535ed40d1):
New hosted CI results are pending; earlier successful runs are explicitly labeled with their source revision in the PR description. |
Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@nvidia.com>
Resolve the prediction CLI conflict by retaining generic speculation validation and native-router resource planning. Validation: 2,356 Rust tests pass (one existing ignored); 1,239 Python tests pass with the freshly built native extension. Rust formatting, focused Python lint/format, and packaged legal-file checks pass. Signed-off-by: Harry Lee <harrli@nvidia.com>
|
Re-checked at 8b00c03. Main's tip c9e7363 is the merge-base, so this tree is the squash-merge result for today's main. B1, B2, B3 and the memory-guard note are fixed, and I verified each one:
There is no regression on the built-in engine path. Compared with main, all 13 e2e engine configs give identical results, and all 12 configs that accept B4 is the only open item, and my change request now stands only on it. The review asked for sign-off on the new boundary from @tedzhouhk and @dreamtalen, and |
Signed-off-by: Harry Lee <harrli@nvidia.com>
Harrilee
changed the title
Signed-off-by: Harry Lee <harrli@nvidia.com>
Signed-off-by: Harry Lee <harrli@nvidia.com>
Harrilee
changed the title
Signed-off-by: Harry Lee <harrli@nvidia.com>
Harrilee
changed the title
Arsene12358
left a comment
Arsene12358
left a comment
There was a problem hiding this comment.
Re-checked at 8a0b0e8. The merge-base is c9e7363, and main's newer commit 7815f40 changes CI files only. Thank you for moving Dynamo routing out of the wheel. B4 is resolved: the wheel builds from crates/core, Cargo.lock has no Dynamo or Git package, and REVIEW.md matches main. I am resolving that thread.
What I verified at this head:
- B3 still holds. The repro passes 2/2, and it fails 0/2 with only the reorder reverted.
- The memory guard still applies to
router.policy=kv_router. - Engine predictions match main: 13/13 e2e configs, and 12/12 per-request with
engine.tokenization. - With a runtime built from ai-dynamo/dynamo#16854 (b5a8eab3) and no
ai-dynamopackage,tests/test_conversation_routing_cli.pypasses 26/26 withAISIM_REQUIRE_DYNAMO_ROUTER=1. - I ran four AgentX variants with
canonical_v1determinism: aggregated and P/D, each withsessionandsibling_group. In all four, this head and 8b00c03 send every request to the same worker.requests.jsonlandprediction.jsonare identical. The bridge reproduces the old in-process routing exactly.
Three new problems block the merge. The inline comments have the repros and fixes.
- The bridge reads
next_wakeup_mswith serde_json's default float parser, which does not round correctly. When the policy echoes the clock and the value comes back one ULP low, the replay aborts. kv_routerwithout affinity accepts native host offload, then aborts on the first host-tier KV event. 8b00c03 completed the same run.resources.py:910changes the memory plan for--stack dynamousers who configure no router. This line came in with 60fd1bb, was removed in bd5d1a9 and came back in e524d48. My earlier reviews missed it.
Two merge gates remain:
- CI is red because
tests/test_conversation_routing_cli.pyhas no owner. Add it to the replay area paths and to the shared globs in.github/codeowners/areas.yaml, next totests/test_runner.py, then regenerateCODEOWNERSwithemit_codeowners.py. With that change, both strict ownership checks pass locally. - The feature needs ai-dynamo/dynamo#16854, which is open with no reviews. After it merges, please rerun the acceptance suite against the merged revision.
Non-blocking:
- Speed. I timed the full AgentX quickstart profile, with medians of 3 interleaved runs. Replay time is 7.14 s with the bridge and 2.12 s with the in-process routing at 8b00c03. Without a router it is 1.52 s. One 600 s run makes 177,890 policy calls for 128 placements. Skipping empty
observebatches, repeatedadvance_clockcalls at the same time and decode KV capture removes most of them. 8b00c03 turned decode capture off. - CI coverage. All 26 routing tests skip in hosted CI. After #16854 merges, a job that installs a pinned runtime and sets
AISIM_REQUIRE_DYNAMO_ROUTER=1closes the gap. - Docs.
docs/reference/cli.md:108-109and:373still sendrouterto--stack dynamo.docs/replay/agentic/quickstart.md:298links to an anchor that bd5d1a9 removed.quickstart.md:326still says that canonical seeds need areplay-benchbuild, but this head acceptscanonical_v1in the standard wheel.
| .extract(py) | ||
| })?; | ||
| let value: Value = | ||
| serde_json::from_str(&encoded).context("invalid external policy response JSON")?; |
There was a problem hiding this comment.
NB1: serde_json here runs without float_roundtrip, so its float parsing does not round correctly. router.py:444 returns next_wakeup_ms=self.now_ms while a request waits, and runtime_utils.rs:105-109 requires wakeup_ms >= now_ms. When the echoed clock is read one ULP low, the run aborts.
Repro: take the tree from _conversation_prediction(tmp_path, "vllm", topology, "sibling_group"), set branch-b's not_before_ms to 20 so that it spawns together with branch-a, and add engine.tokenization: {workers: 1, base_ms: 0.13309929789368105, per_input_token_us: 1.3}. Aggregated and P/D both fail:
replay invariant violated: aggregated placement policy wakeup must be finite and not precede replay time 20.299499297893682ms; got 20.29949929789368ms
Six base times of this kind fail 12/12. With serde_json = { version = "1", features = ["float_roundtrip"] } in the root Cargo.toml, the same 12 cases pass, and the 13 engine e2e configs stay identical to main. Returning the wakeup as a Python float through PyO3 is the local alternative. Either way, please add a regression test with a non-integer clock and a waiting sibling. The acceptance suite uses integer timings, so it cannot reach this case.
| return {"provider": "round_robin", "config": None} | ||
| if spec.execution_mode != "offline": | ||
| raise ValueError("engine.router requires offline execution") | ||
| if adapter.config.get("affinity") is not None: |
There was a problem hiding this comment.
NB2: the host and G3 offload check runs only when router.affinity is set. Without affinity, a kv_router run with host_offload is accepted. Then router.py:419-421 raises on the first host-tier event, because the bridge always captures engine KV events (external_policy.rs:278).
Repro: AgentX quickstart with one aggregated worker, capacity: {type: fixed, blocks: 4500}, host_offload: {num_host_blocks: 16384, d2h_bandwidth_gbps: 32.0, h2d_bandwidth_gbps: 32.0}, router: {policy: kv_router} and a 600 s profile. At this head it fails with external aggregated policy "observe": ValueError: routing requires device KV events with matching rank. At 8b00c03, and with no router, the same run completes 47 requests.
Please run the device-only KV check for every kv_router run before execution, not only under affinity. Alternatively, skip and count non-device events, as 8b00c03 did, and document that the policy sees device-tier overlap only.
| # lifecycle rows. Their count depends on simulated completion times, so | ||
| # neither the finite corpus nor duration alone bounds the full profile. | ||
| profile = workload.get("agentic_profile") is not None | ||
| if profile or stack == "dynamo": |
There was a problem hiding this comment.
NB3: or stack == "dynamo" changes the plan for --stack dynamo runs that configure no router. With 4 workers requested on the same host, main plans weka-two-plays.jsonl with agentic_snapshot as trace-json-metadata-v1 (537 MB): admitted unsupervised, and parallelism 4 under supervision. At this head it becomes dynamo-unqualified-v1: refused unsupervised, and parallelism 1 under supervision. Snapshot plus warmup, agentic-mooncake.jsonl and dynamo-agentic.jsonl change the same way. build_plan already relabels engine-stack routing through native_router, so this restores main's behavior:
| if profile or stack == "dynamo": | |
| if profile: |
If the change is intended for Dynamo's own replay, please document it in docs/reference/local-resources.md and in the PR description. This line came in with 60fd1bb. bd5d1a9 removed it and e524d48 put it back. My earlier reviews missed it.
This branch has not been deployed
4 participants
Why and what changed
Run conversation-aware AgentX simulation in AISimulate's default
enginestack while calling independently installed Dynamo policy interfaces. AISimulate owns traffic, lineage, virtual time, engine execution and reports. Dynamo owns worker selection, cache indexing, native admission and session-affinity state.The
engine.routeradapter translates AISim events into native scheduler/affinity calls and uses Dynamo's existingRadixTreeand hash functions. AISim's wheel/core has no Dynamo build dependency or copied routing algorithms. The neutral Python callback bridge reuses existing replay execution; unused native-consumer wrappers, duplicate wire types and their dedicated tests have been removed.Review map
Risk: medium. Start with
python/aisimulate/src/aisimulate/router.py,crates/core/src/python/external_policy.rs, andtests/test_conversation_routing_cli.py. Check dispatch commit/abort ownership, virtual-time wakeups and cleanup with the GIL. Missing native interfaces fail explicitly. The interface has not been performance-benchmarked.Dependency order
Only AISimulate and the API-capable
ai-dynamo-runtimeare needed. Theai-dynamosimulation plugin is absent from acceptance. Existing #15631/#16490 remain unchanged; no release backport is requested.Evidence
AISim revision
8a0b0e8ee141cf5b308ac1aacb4607d3612bcc7c; Dynamo API revisionb5a8eab3d62b26ed3bc5ff341574658c920aefb9.AISIM_REQUIRE_DYNAMO_ROUTER=1. Covers vLLM/SGLang, aggregated/P/D, session/sibling binding, cache reuse/off, tokenization, decode transfer, snapshot/warmup/profile and Weka lineage. Import guards prohibit delegation to Dynamo replay/router/planner Python modules.Fixtures use authored traffic and fixed engine timings. Functional acceptance does not establish hardware timing accuracy. Adapted schema/test/host-translation provenance is recorded in packaged notices. GitHub CI was not monitored or claimed green.
Part of #38. Required API: ai-dynamo/dynamo#16854; optional update: ai-dynamo/dynamo#15240.