fix: add prompt_cache_key and prompt_cache_retention fields to maintain OpenAI chat completion request interface - #1767
hustxiayang wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1767 +/- ##
=======================================
Coverage 84.27% 84.27%
=======================================
Files 117 117
Lines 12862 12865 +3
=======================================
+ Hits 10839 10842 +3
+ Misses 1379 1378 -1
- Partials 644 645 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nacx
left a comment
There was a problem hiding this comment.
In PRs #1396 and #1681, support for configuring caching for Anthropic backends was already introduced.
Now that we're adding these fields, should we probably take them into account when translating to those backends? (cc @alexagriffith)
|
Without translation like @nacx mentioned, adding fields alone like the current change does nothing |
|
@mathetake it's for openai, and does not need any translations |
|
I am saying it's not translating meaning that this unmarshaling result is not used anywhere. could you read the source code and try to understand exactly where your change here affects the runtime behavior. It doesn't do anything |
|
Also please do not claim that you introduce the runtime behavior change without having any unit test like this. It's not how we develop a feature or any fix. otherwise it's only your word that we can trust regarding the change, which is completely unreliable and fragile |
|
@mathetake I think it's just to maintain |
|
Then what you are saying now is different from what you described you were trying to do. Could you explain why is that
|
|
@mathetake ah, sorry that it might be a bit ambiguity in this sentence. The meaning I want to express is that |
|
No, I am saying this doesn't allow users to do anything. The description is completely wrong |
|
@mathetake |
|
so then this PR itself is useless and not necessary to merge unless you actually use these fields and introduce the runtime change as I suggested. Until then keeping this as draft |
|
Hi, I was trying to encourage users to use this field as it allows |
|
with regards to @nacx's comments, I guess it's about making gcp anthropic cache control to be compatible/unified with OpenAI's |
|
The contribution is valuable, but as @mathetake says, it is incomplete, and this PR can't be merged as-is. The OpenAI model you're modifying provides a common interface that AI Gateway can forward directly to OpenAI, but also translates to other endpoints, such as Bedrock, GCP Vertex, etc. The caching feature was already made available to Bedrock and GCP in the PRs I referenced, but those PRs enable the feature at a different place of the model, in some provider-specific fields. Now you're adding this feature in the common-API, and it is important that translation takes this into account to provide a proper experience to users. For this PR to progress we need:
|
|
|
✅ Deploy Preview for theagentrouter canceled.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Description
add prompt_cache_key and prompt_cache_retention, which are used to allow users to influence the cache behaviour.
The api: https://platform.openai.com/docs/api-reference/chat/create
doc: https://platform.openai.com/docs/guides/prompt-caching