continue
continue copied to clipboard
feat(config-yaml): Add tools configuration to Cache Behavior
Description
Provides inital configuration support for this new feature: https://github.com/continuedev/continue/issues/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 will be done via separate PRs. The documenation to use this feature will be updated on a per model / provider basis.
Note, I'm not planning to make significant updates to the JSON schema to support this feature.
Checklist
- [X] I've read the contributing guide
- [X] The relevant docs, if any, have been updated or created
- [X] 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
cacheBehavior: cacheSystemMessage: true cacheConversation: true cacheToolsConfig: true
Add this configuration to a model and validate through debugging that it's loaded and available to the model / provider in question downstream.
Deploy Preview for continuedev ready!
| Name | Link |
|---|---|
| Latest commit | e29d188333e49f02cb2fa9fc37f0bf3c0044da16 |
| 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...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
@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 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?
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.
@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.
+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
@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 https://github.com/continuedev/continue/issues/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.
@sestinj quick ping on how you would like to proceed?
@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
Should this PR stay open?
@tomasz-stefaniak stand by. I'm going to push an update shortly based on feedback.
@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.