Skip to content

feat(config-yaml): Add promptCaching to Default Completion Options and enable Bedrock Tools Caching #5371

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

Merged
merged 4 commits into from
May 12, 2025

Conversation

chezsmithy
Copy link
Contributor

@chezsmithy chezsmithy commented Apr 25, 2025

Description

Provides inital configuration support for this new feature: #5340

Adding the ability to specify if the model / provider should enable caching for the tools Configuration. The changes to enable the use of this configuration in providers will be done via separate PRs. The documenation to use this feature will be updated on a per model / provider basis.

Enabling tools caching for AWS Bedrock Claude Models.

Note, I'm not planning to make significant updates to the JSON schema to support this feature.

Checklist

  • I've read the contributing guide
  • The relevant docs, if any, have been updated or created
  • The relevant tests, if any, have been updated or created

Screenshots

[ For visual changes, include screenshots. Screen recordings are particularly helpful, and appreciated! ]

Testing instructions

defaultCompletionOptions:
promptCaching: true

Add this configuration to a model and validate through debugging that it's loaded and available to the model / provider in question downstream.

@chezsmithy chezsmithy requested a review from a team as a code owner April 25, 2025 21:46
@chezsmithy chezsmithy requested review from tomasz-stefaniak and removed request for a team April 25, 2025 21:46
Copy link

netlify bot commented Apr 25, 2025

Deploy Preview for continuedev ready!

Name Link
🔨 Latest commit e29d188
🔍 Latest deploy log https://app.netlify.com/sites/continuedev/deploys/681e6d74679a6f0008f2d904
😎 Deploy Preview https://deploy-preview-5371--continuedev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Patrick-Erichsen
Copy link
Collaborator

@chezsmithy thanks for splitting up the schema/implementation work here.

I worry that the CacheBehavior is getting a bit too granular. I think the majority of users probably just want everything or nothing to be cached.

What's your take? Do you find value in having it this granular? Is there any scenario where you wouldn't just want the system message, chat, and tool configs to all be cached together?

@Patrick-Erichsen Patrick-Erichsen requested review from Patrick-Erichsen and removed request for tomasz-stefaniak April 28, 2025 03:22
@chezsmithy
Copy link
Contributor Author

@Patrick-Erichsen agree. If you are going to cache you might as well enable all of them. One pain point to adding a global cache setting is testing for the various providers. I don't have access to them all so this kind of change would require a bit of a breaking change. Maybe we add the individual settings and slowly deprecate them in favor of a single setting that replaces them all?

Where would you think that should live? Under env?

@Patrick-Erichsen
Copy link
Collaborator

Patrick-Erichsen commented Apr 28, 2025

I would agree, better to get this shipped and see how it's used vs trying to figure out all the edge cases making it generalizable to all providers.

I approved but just realized there's a small merge conflict from your other caching PR, once that's resolved let's merge 👍

Also, what are your thoughts on implementing the actual behavior in this PR as well? Instead of merging the config and then implementing separately.

@chezsmithy
Copy link
Contributor Author

@Patrick-Erichsen i have the changes ready and tested for the bedrock provider. I can add those to the PR. I don't have access to Vertex or Claude API direct so I'm less inclined to add those as I can't test it.

@sestinj
Copy link
Contributor

sestinj commented Apr 29, 2025

+1 to the idea of a single enable/disable cache setting. If we add all of these granular permissions, we're going to be stuck supporting them across a very large number of providers. Agree that this PR should add the options there, implementation for Bedrock, but can skip implementation for Vertex and Claude API

@chezsmithy
Copy link
Contributor Author

chezsmithy commented Apr 29, 2025

@Patrick-Erichsen I've pushed the changes for review and have added the Bedrock implmentation. I'll open separate issues as enhacement requests for the other providers as a reminder to add them, or looking for a community member to add them.
@sestinj I'll queue up a new change to implement a single place to enable caching. Could you add some ideas to #5340 related to the best place to add that setting and the name for it? Once we have that ironed out I'll submit a new PR to add in the global setting and make Bedrock work with both the gobal setting and individual settings for backwards compatiblity. Then we can move to deprecate the individual settings over time.

@chezsmithy chezsmithy changed the title feat(config-yaml): Add tools configuration to Cache Behavior feat(config-yaml): Add tools configuration to Cache Behavior and Bedrock Tools Caching Apr 30, 2025
@chezsmithy
Copy link
Contributor Author

@sestinj quick ping on how you would like to proceed?

@sestinj
Copy link
Contributor

sestinj commented May 5, 2025

@chezsmithy Sorry if this didn't come across in the last message—I don't think we should merge a specific setting for caching tools at all if we are just going to add a general cache setting right after and then deprecate it.

I think we should skip straight to the creation of the general caching setting. Probably lives in defaultCompletionOptions.promptCaching

@tomasz-stefaniak
Copy link
Collaborator

Should this PR stay open?

@chezsmithy
Copy link
Contributor Author

@tomasz-stefaniak stand by. I'm going to push an update shortly based on feedback.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label May 9, 2025
@chezsmithy chezsmithy force-pushed the feat-cache-tools-config branch from 55dfae4 to 6b8345e Compare May 9, 2025 04:12
@chezsmithy chezsmithy changed the title feat(config-yaml): Add tools configuration to Cache Behavior and Bedrock Tools Caching feat(config-yaml): Add promptCaching to Default Completion Options and enable Bedrock Tools Caching May 9, 2025
@chezsmithy
Copy link
Contributor Author

@tomasz-stefaniak it's ready for review. The e2e tests seem to be failing for all recently pushed PRs for some reason. I noted it on discord.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 12, 2025
@Patrick-Erichsen Patrick-Erichsen merged commit d350549 into continuedev:main May 12, 2025
32 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Issues and PRs May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants