refactor(replay): unify planner runtime boundary [DYN-3850] - #12334
Merged
Conversation
Signed-off-by: PeaBrane <yanrpei@gmail.com>
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Signed-off-by: PeaBrane <yanrpei@gmail.com>
Contributor
WalkthroughChangesOffline replay scaling policy
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
components/src/dynamo/planner/offline/replay_adapter.py (1)
314-318: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInitialize
_pending_tickin__init__instead of probing withhasattr.
_pending_tickis created only insidestart(), soinitial_tick_mshas to guess at object state. Declaringself._pending_tick: Optional[ScheduledTick] = Nonein__init__and testingis Nonemakes the lifecycle explicit and keeps type checkers happy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/src/dynamo/planner/offline/replay_adapter.py` around lines 314 - 318, Initialize self._pending_tick as Optional[ScheduledTick] = None in the class __init__, then update initial_tick_ms to check whether _pending_tick is None instead of using hasattr. Preserve the existing start() behavior and ensure the tick is non-None before accessing at_s.components/src/dynamo/replay/main.py (1)
585-602: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider constructing the adapter inside the scope's
try.
_prepare_planner_replaybuilds theReplayPlannerAdapter(which opens an event loop in__init__) and then runs the AIC bootstrap. If any bootstrap step raises an exception type not caught there, the adapter is never entered and its loop is never closed. Wrapping the post-construction bootstrap in thewith adapter:scope — or atry/except: adapter.close(); raise— closes that gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/src/dynamo/replay/main.py` around lines 585 - 602, Update _planner_replay_adapter so exceptions during _prepare_planner_replay’s post-construction bootstrap still close the ReplayPlannerAdapter event loop. Construct or retain the adapter within a try/finally or with-scope that guarantees adapter.close() before re-raising, while preserving normal context-manager cleanup and yielded adapter behavior.lib/bindings/python/tests/replay/test_replay_policy_plumbing.py (1)
52-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting
finalizeruns beforeclose.
finalize()now depends on the adapter still being open (it callsself._recorder.finalize()beforeclose()shuts the loop down). Recording the order in_FakePlannerAdapter— e.g. setself.finalized_before_close = not self.closedinsidefinalize— would lock in that contract, which is exactly what the try/finally removal inreplay_adapter.pychanged.Also applies to: 101-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/bindings/python/tests/replay/test_replay_policy_plumbing.py` around lines 52 - 67, Update _FakePlannerAdapter to record whether finalize is called while the adapter remains open, such as by setting a finalized_before_close flag from finalize based on closed; initialize the flag in __init__ and assert it in the affected replay tests to enforce finalize-before-close ordering.lib/bindings/python/src/dynamo/_core.pyi (1)
2587-2601: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument
scaling_policyin the docstrings.The parameter is offline-only (
ValueErrorotherwise) and must exposeinitial_tick_ms() -> floatandon_tick(metrics) -> dict. A one-line mention (or aProtocolalias instead ofAny) makes the contract discoverable from the stub.Also applies to: 2631-2639
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/bindings/python/src/dynamo/_core.pyi` around lines 2587 - 2601, Update the replay function docstrings around the scaling_policy parameter to document that it is offline-only and raises ValueError otherwise, and that the policy must provide initial_tick_ms() -> float and on_tick(metrics) -> dict. Apply the same documentation to the additional affected docstring, or replace Any with an appropriate Protocol alias that exposes this contract.components/src/dynamo/replay/api.py (1)
103-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why
_planner_replay_adapteris imported inside the function.
dynamo.replay.mainimports from this module, so a top-level import would cycle — and the late lookup is what makesmonkeypatch.setattr(replay_main, "_planner_replay_adapter", ...)work intest_replay_policy_plumbing.py. A one-line comment prevents a future "move imports to top" cleanup from breaking both.As per coding guidelines: "Keep imports at the top of the file; always flag
importstatements inside function bodies, methods, or classes as they hide dependencies and make modules harder to understand".♻️ Proposed comment
+ # Imported lazily: dynamo.replay.main imports this module, and tests + # monkeypatch the attribute on the module object. from dynamo.replay.main import _planner_replay_adapterAlso applies to: 188-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/src/dynamo/replay/api.py` at line 103, Add concise comments at both in-function imports of _planner_replay_adapter explaining that the local import avoids the circular dependency from dynamo.replay.main importing this module and preserves monkeypatch.setattr behavior in test_replay_policy_plumbing.py. Keep the imports local.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/src/dynamo/planner/offline/replay_adapter.py`:
- Around line 251-258: Update __exit__ cleanup handling so KeyboardInterrupt and
asyncio.CancelledError raised by close() are always re-raised, including when an
exception from the managed body is already in flight; only log ordinary cleanup
failures and preserve the existing return behavior.
In `@components/src/dynamo/replay/main.py`:
- Around line 895-903: Update the planner trace replay call to pass the
trace-specific arguments for shared-prefix ratio and prefix-group count, using
the corresponding trace option symbols rather than args.shared_prefix_ratio and
args.num_prefix_groups. Keep the remaining run_trace_replay arguments unchanged
and align these values with the non-planner trace replay call.
In `@lib/bindings/python/tests/replay/test_replay_planner_load_modes.py`:
- Around line 199-220: Update
test_normal_replay_releases_gil_for_background_python_thread to avoid the fixed
20 ms sleep and wall-clock duration assumption: have the background thread loop
and count iterations until a stop signal is set, signal that stop after replay
completes, join the thread, and assert the counter recorded any progress.
In `@lib/mocker/src/replay/offline/agg.rs`:
- Around line 266-267: Remove the stale SLA-threshold doc comment above
with_scaling_policy, leaving only the documentation describing the scaling
policy and tick-scoped FPM collection.
---
Nitpick comments:
In `@components/src/dynamo/planner/offline/replay_adapter.py`:
- Around line 314-318: Initialize self._pending_tick as Optional[ScheduledTick]
= None in the class __init__, then update initial_tick_ms to check whether
_pending_tick is None instead of using hasattr. Preserve the existing start()
behavior and ensure the tick is non-None before accessing at_s.
In `@components/src/dynamo/replay/api.py`:
- Line 103: Add concise comments at both in-function imports of
_planner_replay_adapter explaining that the local import avoids the circular
dependency from dynamo.replay.main importing this module and preserves
monkeypatch.setattr behavior in test_replay_policy_plumbing.py. Keep the imports
local.
In `@components/src/dynamo/replay/main.py`:
- Around line 585-602: Update _planner_replay_adapter so exceptions during
_prepare_planner_replay’s post-construction bootstrap still close the
ReplayPlannerAdapter event loop. Construct or retain the adapter within a
try/finally or with-scope that guarantees adapter.close() before re-raising,
while preserving normal context-manager cleanup and yielded adapter behavior.
In `@lib/bindings/python/src/dynamo/_core.pyi`:
- Around line 2587-2601: Update the replay function docstrings around the
scaling_policy parameter to document that it is offline-only and raises
ValueError otherwise, and that the policy must provide initial_tick_ms() ->
float and on_tick(metrics) -> dict. Apply the same documentation to the
additional affected docstring, or replace Any with an appropriate Protocol alias
that exposes this contract.
In `@lib/bindings/python/tests/replay/test_replay_policy_plumbing.py`:
- Around line 52-67: Update _FakePlannerAdapter to record whether finalize is
called while the adapter remains open, such as by setting a
finalized_before_close flag from finalize based on closed; initialize the flag
in __init__ and assert it in the affected replay tests to enforce
finalize-before-close ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 27fb3f18-19f9-4b03-90ed-19ce349fba8b
📒 Files selected for processing (26)
components/src/dynamo/mocker/__init__.pycomponents/src/dynamo/planner/offline/replay_adapter.pycomponents/src/dynamo/planner/tests/offline/test_replay_adapter_fpm.pycomponents/src/dynamo/replay/api.pycomponents/src/dynamo/replay/main.pylib/bindings/python/rust/lib.rslib/bindings/python/rust/llm/replay.rslib/bindings/python/src/dynamo/_core.pyilib/bindings/python/tests/replay/test_replay_planner_load_modes.pylib/bindings/python/tests/replay/test_replay_planner_scaling.pylib/bindings/python/tests/replay/test_replay_policy_plumbing.pylib/mocker/src/replay/collector.rslib/mocker/src/replay/entrypoints.rslib/mocker/src/replay/mod.rslib/mocker/src/replay/offline/agg.rslib/mocker/src/replay/offline/components/engine.rslib/mocker/src/replay/offline/disagg.rslib/mocker/src/replay/offline/disagg_tests.rslib/mocker/src/replay/offline/entrypoints.rslib/mocker/src/replay/offline/events.rslib/mocker/src/replay/offline/executor.rslib/mocker/src/replay/offline/mod.rslib/mocker/src/replay/offline/runtime_utils.rslib/mocker/src/replay/offline/scaling.rslib/mocker/src/replay/planner_handle.rslib/mocker/src/replay/validate.rs
💤 Files with no reviewable changes (3)
- lib/mocker/src/replay/planner_handle.rs
- lib/bindings/python/rust/lib.rs
- components/src/dynamo/mocker/init.py
Signed-off-by: PeaBrane <yanrpei@gmail.com>
sachalmalick
approved these changes
Jul 29, 2026
Signed-off-by: PeaBrane <yanrpei@gmail.com>
…ner-replay-20260728 Signed-off-by: PeaBrane <yanrpei@gmail.com>
Signed-off-by: PeaBrane <yanrpei@gmail.com>
tedzhouhk
approved these changes
Jul 29, 2026
PeaBrane
enabled auto-merge (squash)
July 29, 2026 16:45
PeaBrane
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ReplayScalingPolicy; normal mode suppliesNone.ReplayPlannerAdapterthe context-managed Python owner and accept an injectableEngineProtocol, leaving that protocol as the future local-versus-IPC planner boundary.Reviewer guide
lib/mocker/src/replay/offline/{executor,scaling,agg,disagg}.rsand the shared entrypoint dispatch.lib/bindings/python/rust/llm/replay.rscontains the optional Python scaling-policy boundary; the no-policy path still releases the GIL.components/src/dynamo/planner/offline/replay_adapter.pyowns planner bootstrap, execution, report finalization, and cleanup throughEngineProtocol.Online/live replay is intentionally unchanged. The independently investigated KVBM replay ordering fix and its campaign harness are not part of this diff; the narrow fix is tracked separately in #12326.
Validation
Byte parity: 11/11 completed semantic rows produced one stable digest per revision across two separate processes and then matched byte-for-byte between baseline and candidate:
Planner lifecycle: aggregated vLLM and SGLang exercised
1 → 2 → 3 → 2 → 1; disaggregated prefill exercised1 → 2 → 1; disaggregated decode exercised1 → 2 → 3 → 2 → 1. Every fixture completed every request and retained ordered scaling events plus active/starting/draining observations.Paired performance: all 8/8 rows passed after 5 warmups and 30 randomized adjacent pairs with a fixed-seed, one-sided 95% bootstrap over 200,000 draws. Median candidate/baseline ratios and 95% upper bounds were:
The pass gate was an upper bound of 1.05; the worst observed upper bound was 1.02083.
Size: native replay binary ratio 1.00130 (
.text1.00172); PyO3 extension ratio 0.99897 (.text0.99941), both below the 1.05 gate.Local suites: 176 offline replay Rust tests passed; 4 one-worker KV-router validator tests passed; 104 Python binding replay tests passed with 9 expected skips; 24 planner offline tests passed; all changed files passed pre-commit.
The frozen native SGLang-disaggregated 5,000-row semantic row remains a bounded wall-time blocker, not a parity exception: its baseline process stayed CPU-active through independent 30- and 45-minute limits without emitting a report. The same configuration completed 1,000/1,000 requests in 0.528 seconds during qualification.
The remote campaign used the same temporary deterministic replay fixture in both revisions to isolate this planner refactor. That fixture, the KVBM ordering change, and the parity driver are excluded from this PR.
Summary by CodeRabbit