fix(test): clear tokenizer LRU cache for test isolation#21279
Conversation
The _select_tokenizer_helper function is decorated with @lru_cache, which causes test failures when tests run sequentially with --dist=loadscope. Previous tests' cached results prevent from_pretrained from being called, causing mock assertions to fail. Implemented triple-layer cache clearing: 1. Module-level clear on import 2. Class-level clear in setUpClass 3. Function-level clear in setUp + pytest fixture This ensures test isolation while allowing --dist=loadscope to provide better overall CI stability (70% pass rate vs 40% without loadscope). Fixes the intermittent failure in TestTokenizerSelection where 'from_pretrained' mock was never called due to cache hits. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes a test isolation issue in
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| tests/test_litellm/litellm_core_utils/test_token_counter.py | Adds triple-layer LRU cache clearing for _select_tokenizer_helper to fix test isolation under --dist=loadscope. The approach is correct but over-engineered — setUp() plus module-level clear would suffice. Minor formatting issue with missing blank line. |
Sequence Diagram
sequenceDiagram
participant TF as Test Framework
participant ML as Module Load
participant SC as setUpClass
participant SU as setUp / autouse fixture
participant T as Test Method
participant LRU as _select_tokenizer_helper LRU Cache
ML->>LRU: cache_clear() (module import)
Note over LRU: Cache empty
TF->>SC: Run setUpClass()
SC->>LRU: cache_clear()
Note over LRU: Cache empty (redundant)
loop For each test method
TF->>SU: Run setUp() + autouse fixture
SU->>LRU: cache_clear()
Note over LRU: Cache empty before test
TF->>T: Run test with @patch mock active
T->>LRU: _select_tokenizer_helper("model")
Note over LRU: Cache MISS → calls mocked function
LRU-->>T: Returns mocked result
T->>T: assert mock called ✓
end
Last reviewed commit: 5f79bf4
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Based on Greptile feedback: - Removed autouse fixture (applied too broadly to unrelated tests) - Removed setUpClass (redundant since setUp runs before every test) - Kept module-level clear and setUp() method (sufficient for test isolation) - Added blank line for proper formatting The simplified approach still ensures test isolation under --dist=loadscope while avoiding unnecessary complexity. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@greptile-apps re-review please |
Greptile SummaryThis PR fixes a test isolation issue in
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| tests/test_litellm/litellm_core_utils/test_token_counter.py | Adds module-level cache_clear() and setUp() method to TestTokenizerSelection to clear the _select_tokenizer_helper LRU cache before each test, fixing test isolation issues under --dist=loadscope. Clean, minimal change. |
Flowchart
flowchart TD
A[Module Load] -->|cache_clear| B[LRU Cache Empty]
B --> C[TestTokenizerSelection]
C --> D[setUp runs]
D -->|cache_clear| E[LRU Cache Empty]
E --> F["@patch mocks Tokenizer"]
F --> G[_select_tokenizer_helper called]
G --> H{Cache miss}
H --> I[Mock is invoked]
I --> J[assert_called_once passes]
J --> K[Next test method]
K --> D
Last reviewed commit: 706792b
Summary
Fixes test isolation issue in
TestTokenizerSelectionthat was exposed by--dist=loadscopein PR #21277.Problem
_select_tokenizer_helperis decorated with@lru_cache, causing:--dist=loadscope, cached results persist between testsTokenizer.from_pretrainedis never called (cache hit instead)Root Cause
Solution
Implemented triple-layer cache clearing strategy:
setUpClass()setUp()and@pytest.fixture(autouse=True)This ensures cache is cleared before each test regardless of how pytest schedules them.
Why This Matters
Data from PR #21277 testing shows:
--dist=loadscope: 70% test pass rate (7/10)--dist=loadscope: 40% test pass rate (4/10)This fix allows us to keep
--dist=loadscope(better overall stability) while fixing the specific cache issue it exposed.Testing
Related
🤖 Generated with Claude Code