continue icon indicating copy to clipboard operation
continue copied to clipboard

feat(config-yaml): Add tools configuration to Cache Behavior

Open chezsmithy opened this issue 7 months ago • 3 comments

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.

chezsmithy avatar Apr 25 '25 21:04 chezsmithy

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...

QR Code

Use your smartphone camera to open QR code link.

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

netlify[bot] avatar Apr 25 '25 21:04 netlify[bot]

@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 avatar Apr 28 '25 03:04 Patrick-Erichsen

@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?

chezsmithy avatar Apr 28 '25 06:04 chezsmithy

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 avatar Apr 28 '25 22:04 Patrick-Erichsen

@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.

chezsmithy avatar Apr 29 '25 14:04 chezsmithy

+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

sestinj avatar Apr 29 '25 17:04 sestinj

@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.

chezsmithy avatar Apr 29 '25 19:04 chezsmithy

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

chezsmithy avatar May 05 '25 17:05 chezsmithy

@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

sestinj avatar May 05 '25 22:05 sestinj

Should this PR stay open?

tomasz-stefaniak avatar May 06 '25 22:05 tomasz-stefaniak

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

chezsmithy avatar May 08 '25 13:05 chezsmithy

@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.

chezsmithy avatar May 09 '25 04:05 chezsmithy