8000 Bedrock Prompt Caching Improvements by ronyblum · Pull Request #3099 · RooCodeInc/Roo-Code · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Bedrock Prompt Caching Improvements #3099

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 23, 2025
Merged

Bedrock Prompt Caching Improvements #3099

merged 1 commit into from
May 23, 2025

Conversation

ronyblum
Copy link
@ronyblum ronyblum commented May 1, 2025 •
edited
Loading

Related GitHub Issue

Closes: #3836

Description

This PR fixes an issue with system prompt caching on the first message when using AWS Bedrock. The problem was that the system prompt wasn't being properly cached because we were returning a new system block without cache points instead of using the cache strategy's result.

The fix changes the convertToBedrockConverseMessages method in bedrock.ts to use cacheResult.system instead of creating a new system block. This ensures that any cache points determined by the cache strategy are properly included in the system prompt when sending the request to Bedrock.

Key implementation details:

  • Changed the return statement to use cacheResult.system which includes both the system prompt text and any cache points
  • This ensures consistent caching behavior between the first message and subsequent messages

Test Procedure

  1. Configure AWS Bedrock with prompt caching enabled
  2. Send a message with a system prompt that meets the minimum token threshold for caching
  3. Verify that the system prompt is properly cached on the first message by checking the usage metrics for cacheReadTokens and cacheWriteTokens
  4. Send additional messages and verify that caching continues to work correctly

Type of Change

  • 🐛 Bug Fix: Non-breaking change that fixes an issue.
  • ✨ New Feature: Non-breaking change that adds functionality.
  • 💥 Breaking Change: Fix or feature that would cause existing functionality to not work as expected.
  • ♻️ Refactor: Code change that neither fixes a bug nor adds a feature.
  • 💅 Style: Changes that do not affect the meaning of the code (white-space, formatting, etc.).
  • 📚 Documentation: Updates to documentation files.
  • ⚙️ Build/CI: Changes to the build process or CI configuration.
  • 🧹 Chore: Other changes that don't modify src or test files.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Code Quality:
    • My code adheres to the project's style guidelines.
    • There are no new linting errors or warnings (npm run lint).
    • All debug code (e.g., console.log) has been removed.
  • Testing: 8000
    • New and/or updated tests have been added to cover my changes.
    • All tests pass locally (npm test).
    • The application builds successfully with my changes.
  • Branch Hygiene: My branch is up-to-date (rebased) with the main branch.
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Changeset: A changeset has been created using npm run changeset if this PR includes user-facing changes or dependency updates.
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

N/A - This is a backend fix with no UI changes.

Documentation Updates

  • No documentation updates are required.

Additional Notes

The fix is minimal and focused on ensuring the system prompt caching works correctly on the first message. The change is a one-line modification that uses the existing cache strategy's result instead of creating a new system block.

return {
-   system: systemMessage ? [{ text: systemMessage } as SystemContentBlock] : [],
+   system: cacheResult.system,
    messages: messagesWithCache,
}

Get in Touch

Discord: ronyblum

Important

Fixes system prompt caching issue in convertToBedrockConverseMessages in bedrock.ts by using cacheResult.system for consistent caching behavior.

  • Behavior:
    • Fixes system prompt caching issue in convertToBedrockConverseMessages in bedrock.ts by using cacheResult.system instead of creating a new system block.
    • Ensures consistent caching behavior between the first message and subsequent messages.
  • Misc:
    • Minor change in return statement to include cache points in system prompt.

This description was created by Ellipsis for 30c439b. You can customize this summary. It will automatically update as commits are pushed.

@ronyblum ronyblum requested review from mrubens and cte as code owners May 1, 2025 19:40
@changeset-bot changeset-bot
Copy link
changeset-bot bot commented May 1, 2025

⚠️ No Changeset found

Latest commit: 3e572c0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels May 1, 2025
ellipsis-dev[bot]
ellipsis-dev bot reviewed May 1, 2025
View reviewed changes
@ronyblum ronyblum marked this pull request as draft May 1, 2025 19:51
@hannesrudolph hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap May 5, 2025
@vagadiya
Copy link
vagadiya commented May 7, 2025

Hi Rony,

Hope you are well mate. Sent you a message on discord, but don't think you have seen.

I came across this PR...

I have a custom Roo version that I integrated this change into yesterday (it is used in-house at our company, so unfortunately also has many other bespoke tweaks), I can help test your change.

Anything that I should be aware of, pay special attention to?

  1. Will the change work for initial messages when size = 1?
  2. Does the conversationId need to be unique across conversations? Or is this parameter no longer used?

@ronyblum
Copy link
Author
ronyblum commented May 7, 2025

Hi @vagadiya
Thanks a lot for reach out and suggesting help in testing.
I modified the PR and the changes done. I'm still thinking if the current way is efficient enough or should be improved.

The current PR doesn't work with multi-point-strategy which I restored in another version.

My thought was to have the first system prompt also cached, which is not cached for the moment.
But it all depends on the users way of usage.

To your question -

  • Will the change work for initial messages when size = 1?
    Yes. For the moment it's length > 1
  • Does the conversationId need to be unique across conversations? Or is this parameter no longer used?
    conversationId is unique across the conversations and is in use

@vagadiya
Copy link
vagadiya commented May 8, 2025

Hi Rony, are you active on discord? We could continue the conversation there faster?

@hannesrudolph hannesrudolph moved this from PR [Pre Approval Review] to PR [Draft/WIP] in Roo Code Roadmap May 10, 2025
@hannesrudolph hannesrudolph moved this from New to PR [Draft/WIP] in Roo Code Roadmap May 20, 2025
@ronyblum ronyblum force-pushed the bedrock-prompt-caching branch 2 times, most recently from 9a41e1d to 3bd79fd Compare May 22, 2025 05:12
@ronyblum ronyblum marked this pull request as ready for review May 22, 2025 05:25
@ronyblum ronyblum marked this pull request as draft May 22, 2025 05:29
@ronyblum ronyblum force-pushed the bedrock-prompt-caching branch from 3bd79fd to 55d3b23 Compare May 22, 2025 05:41
@ronyblum ronyblum force-pushed the bedrock-prompt-caching branch from 55d3b23 to 30c439b Compare May 22, 2025 15:46
@ronyblum ronyblum marked this pull request as ready for review May 22, 2025 15:51
adamhill added a commit to adamhill/Roo-Code that referenced this pull request May 22, 2025
@adamhill
Added Rory's cache fix from PR RooCodeInc#3099
cc6bbb1 
PR RooCodeInc#3009 has an important fix, alerted to me by @jbbrown

It was a one liner so I pulled it in.

This brings up a question can we merge PR's in the GH UI?
@mrubens mrubens merged commit 5ab78b3 into RooCodeInc:main May 23, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this from PR [Draft/WIP] to Done in Roo Code Roadmap May 23, 2025
@github-project-automation github-project-automation bot moved this from PR [Draft/WIP] to Done in Roo Code Roadmap May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellipsis-dev ellipsis-dev[bot] ellipsis-dev[bot] left review comments

@mrubens mrubens mrubens approved these changes

@cte cte Awaiting requested review from cte cte is a code owner

Assignees
No one assigned
Labels
enhancement New feature or request size:L This PR changes 100-499 lines, ignoring generated files.
Projects
Roo Code Roadmap
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bedrock Prompt Caching not caching the first system prompt
3 participants 364C
@ronyblum @vagadiya @mrubens
0