Repository navigation
test(timing): pin global idle-cap shift test to the virtual loop clock - #1608
Conversation
test_global_idle_cap_shifts_real_delayed_continuation_to_ten_seconds failed intermittently in CI (6 runs, incl. main pushes, Sep 28-Oct 16) with signatures like 'assert 89.99989932800008 == 90.0 +- 9.0e-05' and 'assert 9.983999999999924 == 10.0 +- 0.01'. Root cause: the test's expectations are only exact if zero loop time passes between three internal sample points. The mechanism's actual contract is that cap_pending_delay retargets the pending continuation timer to enforcement_time + cap: the skipped amount is delay - (t_cap - t_arm) - cap, where t_cap - t_arm is the loop-time elapsed between the timer's arming inside handle_credit_return and the enforcement's loop.time() sample. Any host preemption in that window (CI observed 100us-16ms) is subtracted from the asserted constants, breaking approx(90.0) (+-90us) and, for the post-condition handle.when() - loop.time(), the test's own later clock read drifts by the same unbounded amount, breaking approx(10.0, abs=0.01). The cap itself fires exactly on time; the test encoded a zero-elapsed assumption the mechanism never promised. Fix: run the test on the virtual loop clock (looptime via the repo's time_traveler_no_patch_sleep fixture, the established pattern in the two neighboring idle-cap tests). The arm-to-cap-to-assert path never yields to the event loop, so the elapsed loop time is exactly zero and both asserted quantities become deterministic mechanism truths. No sleeps are involved, so the no-patch-sleep variant is the right fixture.
I, Anthony Casagrande <acasagrande@nvidia.com>, hereby add my Signed-off-by to commit 10f2e3a (test(timing): pin global idle-cap shift test to the virtual loop clock). I certify that the contents of the referenced commit are in accordance with the Developer Certificate of Origin, per CONTRIBUTING.md. 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@04621cec5fc10ff8bdf871053ae573996aa42599Recommended 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@04621cec5fc10ff8bdf871053ae573996aa42599Last updated for commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe replay timing test now accepts the ChangesReplay timing test
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely identifies the timing test change and the use of the virtual loop clock. It matches the main purpose of the pull request. |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the timer’s pace
Virtual time keeps steps in place
No yield shifts the waiting span
The test now follows its plan
I nibble carrots, pleased with grace
Comment @coderabbitai help to get the list of available commands.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Reviewed all three bot comments. The installation instructions reference the current head, CodeRabbit reports no actionable findings, and Codecov reports coverage of modified coverable lines. Source inspection confirms the test uses |
FrankD412
left a comment
FrankD412
left a comment
There was a problem hiding this comment.
Root cause checks out, and this fixes the mechanism instead of loosening the tolerance.
Verified locally on 04621ce:
-
Full
test_agentic_replay.py: 78 passed. -
Injected a real
time.sleep()betweenhandle_credit_returnandenforce_system_idle_capand ran the old (real-clock) and new versions side by side:Injected delay Old (real clock) New (looptime) 0 fail, skipped=89.99988 (ε≈123µs from test overhead alone) pass 200µs fail, 89.99967 pass 20ms fail, 89.97983 pass (30/30) The old test fails here even with no injected delay, so the ±90µs
approx(90.0)was always tight against normal test overhead. Under the fixture,loop.time()andtime.monotonic()both advance by exactly 0.0 across a 20mstime.sleep, which matches the docstring's "exactly zero" claim. -
The other two idle-cap tests that drive a real
LoopScheduler(..._watchdog_bounds_persistent_control_plane_work,..._rechecks_after_barrier_defers_near_timer) already usetime_traveler_no_patch_sleep, so this brings the last one into line. No other sibling has the same exposure.
Non-blocking nit: with elapsed time now exactly zero, approx(10.0, abs=0.01) is far looser than needed. Default approx(10.0) (or exact equality) would catch a regression in the retarget arithmetic that a 10ms window could hide. Fine as-is if you'd rather keep the margin.
LGTM.
Root cause
CI flake analysis (55 failed "Run Integration Tests" + 107 failed "Run Unit Tests" runs, Sep 23 – Oct 23):
test_global_idle_cap_shifts_real_delayed_continuation_to_ten_secondsfailed 6× (incl. main pushes): the test's±90µs/±10msapproximations encoded a zero-elapsed assumption between timer arming and cap enforcement that real host preemption breaks — CI signatures were exactlyconstant − εwith ε = 92µs–16ms. Fixed here by pinning the test to the repo's looptime virtual clock (time_traveler_no_patch_sleep), making the arm→cap→assert path exactly zero elapsed — mechanism-true, no tolerance bump.Related failures investigated and dispositioned (not in this PR):
test_runtime_idle_cap_handles_independent_stream_clock_drift(42 runs) — root-caused by open PR fix(timing): keep the trace idle cap past timers that issue nothing #1602 (do-nothing join-deadline timer in the root's scheduler group absorbs the cap's earliest-timer shift; watchdog never re-arms). Mechanism independently verified; 40/40 passes of this PR's test on the fix(timing): keep the trace idle cap past timers that issue nothing #1602 branch confirm no overlap.test_cache_warmup_handoff_preserves_order_and_spawn_joinsSPAWN_JOIN sub-ms timestamp inversions (9 runs) — already fixed on main by fix(workers): derive request wall timestamps from one per-process anchor #1531 (per-process anchored wall timestamps); all failing jobs predate the merge, zero recurrences in 13 days, mechanism demonstrated by deterministic probe.Verification
AI-assisted fix (investigation + review swarm), verified on SLURM compute.
Summary by CodeRabbit