test: remove network-dependent custom_tokenizer proxy tests#29643
test: remove network-dependent custom_tokenizer proxy tests#29643yuneng-berri wants to merge 2 commits into
Conversation
test_custom_tokenizer_bug.py loaded the Xenova/llama-3-tokenizer from HuggingFace Hub at test time, so it flaked on shared CI runners whenever HF returned 429 Too Many Requests; the surfaced LocalEntryNotFoundError made it look like a connectivity bug. These were integration tests in a unit-test path. The only non-network case left, a model without a custom tokenizer, passed with or without the underlying fix, so it gave no regression signal and went with the rest. Also drop the file from the proxy-db workflow's explicit test list.
…itellm_/hopeful-hawking-5a7b34
Greptile SummaryThis PR removes a flaky test file (
Confidence Score: 4/5Safe to merge; the deletions fix a real CI flakiness problem and correct a rule violation, with only a coverage gap as the remaining concern. The custom_tokenizer extraction path that the original tests validated now has no automated regression guard. A future change that accidentally drops the fix would go undetected until a user reports the wrong tokenizer being used in production. tests/proxy_unit_tests/test_custom_tokenizer_bug.py — the deleted file represents a coverage gap that should be addressed with a mocked replacement test.
|
| Filename | Overview |
|---|---|
| tests/proxy_unit_tests/test_custom_tokenizer_bug.py | Entire file deleted — removes four tests that load HuggingFace tokenizers over the network, violating the proxy unit-test no-network-calls rule; leaves zero coverage for the custom_tokenizer extraction bug fix. |
| .github/workflows/test-unit-proxy-db.yml | Removes the explicit pinned reference to the deleted test file; no other workflow logic is touched. |
Comments Outside Diff (1)
-
tests/proxy_unit_tests/test_custom_tokenizer_bug.pyNo mocked replacement shipped alongside the deletion
Removing the four tests leaves the
custom_tokenizerextraction path (the bug this file was written to catch) with zero regression coverage. The PR description offers a mocked follow-up; without it, a future refactor that accidentally re-introduces the original bug — wheremodel_infowas populated but thecustom_tokenizerkey was never read — would have nothing to catch it. The fix itself lives somewhere in the router/proxy path and is now unguarded. Shipping the mockedpatch-based replacement in the same PR, or opening a tracked issue for it, would close the gap.Rule Used: What: Flag any modifications to existing tests and... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/lit..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Relevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
make test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewType
🧹 Refactoring
✅ Test
Changes
tests/proxy_unit_tests/test_custom_tokenizer_bug.pyloadedXenova/llama-3-tokenizerfrom HuggingFace Hub at test time. On shared CI runners HF periodically returns429 Too Many Requests, after whichhuggingface_hubfalls back to an empty local cache and re-raises aLocalEntryNotFoundError("check your connection"); the surfaced error makes it look like a connectivity problem when the real cause is rate limiting. The failures show up on unrelated PRs because the file lands in thetest_[a-j]*.pymatrix group that every PR runs, so the flake is not tied to any one change.Three of the four tests in the file are network-dependent in this way; they are integration tests sitting in a unit-test path. The fourth,
test_model_without_custom_tokenizer_uses_default, exercises the no-custom-tokenizer path that resolves to the bundled OpenAI tokenizer; it passes with or without the originalmodel_info->custom_tokenizerfix, so it carries no regression signal and is removed along with the rest.The file is also dropped from the explicit test list in
.github/workflows/test-unit-proxy-db.yml, where it was pinned by name in addition to being picked up by the legacy workflow's glob.If we want to keep coverage of the custom-tokenizer-from-
model_infoextraction without a live download, the right follow-up is a mocked test that patches the tokenizer load and asserts the configured tokenizer is selected; happy to add that here if preferred.