[None][test] Enable session prefetch for all test stages - #16770
Conversation
Signed-off-by: qgai <qgai@nvidia.com>
WalkthroughSession prefetching now enables by default outside xdist workers, with ChangesSession prefetch default activation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
tests/unittest/llmapi/test_session_prefetcher.py (1)
96-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the new test functions.
Add parameter and return annotations to comply with the repository’s Python requirements, for example
stage_name: str | None,monkeypatch: pytest.MonkeyPatch, and-> None.As per coding guidelines, every Python function must be annotated.
🤖 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/unittest/llmapi/test_session_prefetcher.py` around lines 96 - 106, Annotate the new test functions test_enabled_by_default_in_all_stages and test_explicit_env_overrides_default with the required parameter and return types, including pytest.MonkeyPatch, stage_name: str | None where applicable, and -> None.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 `@tests/unittest/llmapi/test_session_prefetcher.py`:
- Around line 86-111: Add a regression test for SessionPrefetcher that sets
PYTEST_XDIST_WORKER while leaving prefetch otherwise enabled, and assert
SessionPrefetcher().enabled is false. Keep the existing non-xdist default and
explicit TRTLLM_TEST_PREFETCH_SESSION override tests unchanged.
---
Nitpick comments:
In `@tests/unittest/llmapi/test_session_prefetcher.py`:
- Around line 96-106: Annotate the new test functions
test_enabled_by_default_in_all_stages and test_explicit_env_overrides_default
with the required parameter and return types, including pytest.MonkeyPatch,
stage_name: str | None where applicable, and -> None.
🪄 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: 6a5680b7-98df-42d6-b19e-d1e7091817e8
📒 Files selected for processing (2)
tests/test_common/session_prefetcher.pytests/unittest/llmapi/test_session_prefetcher.py
|
/bot run --disable-fail-fast |
|
PR_Github #61174 [ run ] triggered by Bot. Commit: |
|
PR_Github #61174 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61456 [ run ] triggered by Bot. Commit: |
|
PR_Github #61456 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61479 [ run ] triggered by Bot. Commit: |
|
PR_Github #61479 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61515 [ run ] triggered by Bot. Commit: |
|
PR_Github #61515 [ run ] completed with state |
4 participants
Summary
TRTLLM_TEST_PREFETCH_SESSION=0as the kill switch and keep prefetch disabled in pytest-xdist workers.Changes
tests/test_common/session_prefetcher.py: remove the canary stage allowlist and enable prefetch by default for every test stage.tests/unittest/llmapi/test_session_prefetcher.py: cover canary, non-canary, future, and local/no-stage execution plus explicit env overrides.Test plan
python3 -m pytest llmapi/test_session_prefetcher.py -v -p no:cacheproviderin the TensorRT-LLM development container (42 passed)git diff --checkDev Engineer Review
TRTLLM_TEST_PREFETCH_SESSION=0as an explicit kill switch and continued disabling prefetch in pytest-xdist workers.git diff --check.QA Engineer Review
Test functions added:
test_enabled_by_default_in_all_stagestest_explicit_env_overrides_defaultTest functions removed:
test_canary_enabled_on_canary_stage_groupstest_canary_disabled_elsewheretest_explicit_env_overrides_canary_gateThese test-code changes are not listed in
tests/integration/test_lists/test-db or QA files.Verdict: needs follow-up — CI/manual QA test-list coverage is not represented for the updated test functions.