-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix: Vertex SDK system prompt caching compatibility #7711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,13 +163,7 @@ describe("VertexHandler", () => { | |
| model: "claude-3-5-sonnet-v2@20241022", | ||
| max_tokens: 8192, | ||
| temperature: 0, | ||
| system: [ | ||
| { | ||
| type: "text", | ||
| text: "You are a helpful assistant", | ||
| cache_control: { type: "ephemeral" }, | ||
| }, | ||
| ], | ||
| system: "You are a helpful assistant", // System remains as plain string | ||
| messages: [ | ||
| { | ||
| role: "user", | ||
|
|
@@ -364,16 +358,10 @@ describe("VertexHandler", () => { | |
| expect(textChunks[0].text).toBe("Hello") | ||
| expect(textChunks[1].text).toBe(" world!") | ||
|
|
||
| // Verify cache control was added correctly | ||
| // Verify cache control was added correctly - system remains string, only messages have cache_control | ||
| expect(mockCreate).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| system: [ | ||
| { | ||
| type: "text", | ||
| text: "You are a helpful assistant", | ||
| cache_control: { type: "ephemeral" }, | ||
| }, | ||
| ], | ||
| system: "You are a helpful assistant", // System remains as plain string | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same suggestion here - could we be more explicit about the SDK compatibility requirement in the comment? |
||
| messages: [ | ||
| expect.objectContaining({ | ||
| role: "user", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,23 +75,22 @@ export class AnthropicVertexHandler extends BaseProvider implements SingleComple | |
| * 1. Maximum of 4 blocks can have cache_control | ||
| * 2. Only text blocks can be cached (images and other content types cannot) | ||
| * 3. Cache control can only be applied to user messages, not assistant messages | ||
| * 4. System prompt must remain as a plain string - Vertex SDK does not support array format | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excellent documentation update! The comment clearly explains the SDK limitation. Would it be worth adding a TODO about potentially re-enabling system prompt caching if future Vertex SDK versions support it? This could help future maintainers understand the limitation is SDK-specific rather than an API constraint. |
||
| * | ||
| * Our caching strategy: | ||
| * - Cache the system prompt (1 block) | ||
| * - System prompt cannot be cached directly (must remain as plain string) | ||
| * - Cache the last text block of the second-to-last user message (1 block) | ||
| * - Cache the last text block of the last user message (1 block) | ||
| * This ensures we stay under the 4-block limit while maintaining effective caching | ||
| * This ensures compatibility with Vertex SDK while maintaining effective caching | ||
| * for the most relevant context. | ||
| */ | ||
| const params: Anthropic.Messages.MessageCreateParamsStreaming = { | ||
| model: id, | ||
| max_tokens: maxTokens ?? ANTHROPIC_DEFAULT_MAX_TOKENS, | ||
| temperature, | ||
| thinking, | ||
| // Cache the system prompt if caching is enabled. | ||
| system: supportsPromptCache | ||
| ? [{ text: systemPrompt, type: "text" as const, cache_control: { type: "ephemeral" } }] | ||
| : systemPrompt, | ||
| // System must remain as plain string for Vertex SDK compatibility | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good change here. The comment clearly indicates this is for SDK compatibility rather than a design choice. |
||
| system: systemPrompt, | ||
| messages: supportsPromptCache ? addCacheBreakpoints(messages) : messages, | ||
| stream: true, | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make the comment more explicit about why the system remains as a plain string? Something like:
// System remains as plain string for Vertex SDK compatibility