Repository navigation
test(litellm): pin region and rerun the cache test - #5624
Merged
Merged
Conversation
The prompt-cache tests write a cache entry on one turn and assert a read of it on the next. Bedrock serves that read back reliably in us-east-1 and us-east-2 but only ~88% of the time in us-west-2, so the region decides whether the assertions are stable. conftest only setdefault()s the region, leaving a developer's own exported AWS_REGION in control; pinning it in the fixtures makes these tests behave the same everywhere. Read-back is still best effort, so the multi-turn cache test reruns instead of being skipped. It uses pytest.mark.flaky rather than retry_on_flaky because the latter wraps the test in a sync function, which stops pytest-asyncio from collecting an async test. Refs strands-agents#5315
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
zastrowm
marked this pull request as ready for review
October 28, 2026 16:42
zastrowm
enabled auto-merge (squash)
October 28, 2026 16:42
Contributor
|
Assessment: Approve Small, well-scoped test-reliability change: the region pin via Notes
Nicely documented investigation — the measured read-back rates make the region pin easy to trust. |
opieter-aws
approved these changes
Oct 28, 2026
gautamsirdeshmukh
pushed a commit
to gautamsirdeshmukh/harness-sdk
that referenced
this pull request
Nov 2, 2026
Co-authored-by: Mackenzie Zastrow <zastrowm@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human Overview
Caching is region dependent and testing indicated and this test ended up flaky; to reduce flakiness pin the region for this test only + re-enable tests. See the description for the trials ran to determine flakiness.
Description
The litellm prompt-cache tests are unreliable in some regions, and
test_cache_read_tokens_multi_turnis currently skipped because of it. This pins the fixtures to a region where the cache behaves, and re-enables the test with reruns instead of a skip.The test writes a cache entry on turn 1 and asserts a read of it on turn 2. Bedrock serves that read back consistently in us-east-1 and us-east-2, but only about 88% of the time in us-west-2 — measured over 130 trials there against 162 in us-east-1 and 61 in us-east-2, with all three sampled in the same two-minute window. So the region, not the code path, decides whether the assertion holds. Raw
Converseand theLiteLLMModelpath behave identically within a region.conftest.pyonlysetdefault()sAWS_REGION, so a developer with their ownAWS_REGIONexported keeps it — which is how this surfaced. Pinning in the fixtures makes the tests behave the same for everyone.Read-back is best effort even in a good region, so the multi-turn cache test reruns rather than staying skipped. It uses
pytest.mark.flakyand not the existingretry_on_flakyhelper: that helper wraps the test in a sync function, so pytest-asyncio stops recognising anasync deftest and collection fails with "async def functions are not natively supported". Adding async support toretry_on_flakywould be a reasonable follow-up, and would let the retry be scoped toAssertionErrorinstead of any exception.us-east-2 measured equally clean, so it's noted in the comment as the alternative if us-east-1 ever has trouble.
Related Issues
Refs #5315, which tracks the underlying cache-read behaviour. That is not resolved here — the test is made usable, not the behaviour explained.
Documentation PR
None needed.
Type of Change
Bug fix (test reliability)
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.