Repository navigation
fix: include tool_choice in ChatCompletionCache cache key - #8146
Closed
MOHAMMED WASIM KHAN (wasim-builds) wants to merge 1 commit into
Closed
MOHAMMED WASIM KHAN (wasim-builds) wants to merge 1 commit into
MOHAMMED WASIM KHAN (wasim-builds) wants to merge 1 commit into
Conversation
Previously, calls with identical messages/tools but different tool_choice values would incorrectly return cached results. Now tool_choice is included in the cache key computation. Fixes microsoft#7968
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new Tool-valued tool_choice cache-key path is currently untested and the cache-key construction can be simplified to avoid double-serializing tool schemas.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a correctness issue in autogen-ext’s ChatCompletionCache where cached results could be incorrectly reused when only tool_choice differs, by incorporating tool_choice into the cache key so calls that change tool invocation behavior no longer collide.
Changes:
- Extend
ChatCompletionCache._check_cacheto accepttool_choiceand incorporate it into the cache-key payload. - Update
create()andcreate_stream()to passtool_choicethrough to cache checking. - Add a regression test that demonstrates cache misses when
tool_choicechanges.
File summaries
| File | Description |
|---|---|
| python/packages/autogen-ext/src/autogen_ext/models/cache/_chat_completion_cache.py | Adds tool_choice into cache-key computation and threads it through create/create_stream cache checks. |
| python/packages/autogen-ext/tests/models/test_chat_completion_cache.py | Adds a regression test ensuring differing tool_choice values don’t share cache entries. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+197
to
+202
| tool_choice_data: str | None = None | ||
| if tool_choice is not None: | ||
| if isinstance(tool_choice, str): | ||
| tool_choice_data = tool_choice | ||
| elif isinstance(tool_choice, Tool): | ||
| tool_choice_data = json.dumps(tool_choice.schema, sort_keys=True) |
Comment on lines
+961
to
+982
| @pytest.mark.asyncio | ||
| async def test_cache_key_includes_tool_choice() -> None: | ||
| """Test that different tool_choice values produce different cache keys.""" | ||
| from autogen_core import InMemoryStore | ||
| from autogen_core.models import UserMessage | ||
| from autogen_ext.models.cache import ChatCompletionCache | ||
| from autogen_ext.models.replay import ReplayChatCompletionClient | ||
|
|
||
| responses = ["response 0", "response 1"] | ||
| replay_client = ReplayChatCompletionClient(responses) | ||
| replay_client.set_cached_bool_value(False) | ||
| cached_client = ChatCompletionCache(replay_client, InMemoryStore()) | ||
|
|
||
| message = [UserMessage(content="hi", source="user")] | ||
|
|
||
| r0 = await cached_client.create(message, tool_choice="auto") | ||
| assert not r0.cached | ||
| assert r0.content == "response 0" | ||
|
|
||
| r1 = await cached_client.create(message, tool_choice="none") | ||
| assert not r1.cached # Should be cache miss due to different tool_choice | ||
| assert r1.content == "response 1" |
Author
|
Superseded by #8185 |
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.
Summary
Fixes #7968
Previously, calls with identical messages/tools but different tool_choice values would incorrectly return cached results. Now tool_choice is included in the cache key computation.
Changes
Test plan