fix(timing): carry has_forks onto continuation turns#1123
Conversation
…e and user-centric strategies TurnToSend.from_previous_credit accepts next_meta and derives has_forks from it, but the fixed-schedule and user-centric strategies called it without next_meta, so every continuation turn was issued with has_forks=False. Under dag_jsonl FORK datasets this prematurely evicts fork-bearing parent entries from the sticky router instead of deferring eviction until the DAG children drain. Pass the already-computed next_meta in fixed_schedule and look up the next turn's metadata in user_centric_rate, and pin both paths with regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anthony Casagrande <acasagrande@nvidia.com>
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@60b3245a00f85cf50527982ce3f840f15c977ff3Recommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@60b3245a00f85cf50527982ce3f840f15c977ff3Last updated for commit: |
WalkthroughBoth ChangesContinuation metadata fix
Estimated code review effort: 2 (Simple) | ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/unit/timing/strategies/test_fixed_schedule.py`:
- Line 235: The test setup in make_strategy unpacks scheduler but never uses it,
which triggers the static analysis warning. Update the assignment in the test to
avoid binding the unused scheduler value, or otherwise remove that unpacked
variable while keeping the existing strategy and placeholder return from
make_strategy intact.
🪄 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: c0a33633-bb3c-4bc3-981f-98a31e78fd81
📒 Files selected for processing (4)
src/aiperf/timing/strategies/fixed_schedule.pysrc/aiperf/timing/strategies/user_centric_rate.pytests/unit/timing/strategies/test_fixed_schedule.pytests/unit/timing/strategies/test_user_centric_rate.py
| continuation turn (the sticky router defers parent-entry eviction until | ||
| DAG children drain). Regression: from_previous_credit(credit) was called | ||
| without next_meta, so every continuation turn got has_forks=False.""" | ||
| strategy, scheduler, _ = make_strategy([(0, "c1"), (100, "c1")]) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unused scheduler unpacked variable.
Static analysis flags scheduler as unused in this test.
🧹 Proposed fix
- strategy, scheduler, _ = make_strategy([(0, "c1"), (100, "c1")])
+ strategy, _scheduler, _ = make_strategy([(0, "c1"), (100, "c1")])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| strategy, scheduler, _ = make_strategy([(0, "c1"), (100, "c1")]) | |
| strategy, _scheduler, _ = make_strategy([(0, "c1"), (100, "c1")]) |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 235-235: Unpacked variable scheduler is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 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 `@tests/unit/timing/strategies/test_fixed_schedule.py` at line 235, The test
setup in make_strategy unpacks scheduler but never uses it, which triggers the
static analysis warning. Update the assignment in the test to avoid binding the
unused scheduler value, or otherwise remove that unpacked variable while keeping
the existing strategy and placeholder return from make_strategy intact.
Source: Linters/SAST tools
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Problem
TurnToSend.from_previous_credithas a two-argument form (src/aiperf/credit/structs.py) that accepts the next turn'sTurnMetadataand deriveshas_forksfrom it. The fixed-schedule and user-centric timing strategies called it withoutnext_meta, so every continuation turn (turn index >= 1) was issued withhas_forks=Falseregardless of what the dataset declared.Under
dag_jsonlFORK datasets,has_forksis what tells the sticky router (#891) to defer parent-entry eviction until the DAG children drain (src/aiperf/credit/issuer.py,src/aiperf/workers/worker.py). With the flag dropped, a parent whose later turn declares FORK branches is treated as fork-free: its sticky-router entry is evicted the moment its terminal turn returns, prematurely tearing down the pin that FORK children need to land on the parent's worker.Notably,
fixed_schedule.pyalready computednext_metaon the line above the call — it just never passed it.Fix
src/aiperf/timing/strategies/fixed_schedule.py: pass the already-computednext_metatofrom_previous_credit.src/aiperf/timing/strategies/user_centric_rate.py: look up the next turn's metadata viaConversationSource.get_next_turn_metadataand pass it.Intended behavior change: continuation turns now carry the dataset-declared
has_forksflag, so the sticky router keeps fork-bearing parents pinned until their children drain. First turns were already correct (SampledSession.build_first_turnreads turn-0 metadata).Tests (revert-verified pins)
tests/unit/timing/strategies/test_fixed_schedule.py::TestFixedScheduleCreditReturn::test_continuation_turn_carries_has_forkstests/unit/timing/strategies/test_user_centric_rate.py::TestUserCentricCreditReturn::test_continuation_turn_carries_has_forks(new — the user-centric path had no pin)Each source hunk was independently reverted (
git apply -R) and its test confirmed to fail, then re-applied:test_fixed_schedule.py::test_continuation_turn_carries_has_forksFAILED (1 failed, 49 passed)test_user_centric_rate.py::test_continuation_turn_carries_has_forksFAILED (1 failed, 49 passed)Evidence
uv run pytest -n auto tests/unit/timing/strategies/-> 147 passedruff format --check/ruff checkon all four touched files: cleanruff-baselined: OK (118 total, 118 baselined, 0 new),ergonomics: OK (58 total, 58 baselined, 0 new)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests