Skip to content

default maxTokens setting for autocomplete #4448

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ferenci84
Copy link
Contributor

Description

Default maxTokens at 256 for autocomplete if there is no overriding user setting for the model. Added autoCompleteMaxTokens

Change from this comment:
#3994 (comment)

Checklist

  • The relevant docs, if any, have been updated or created - let's create a separate issue for this once the PR is approved and autoCompleteMaxTokens setting is kept in the completion options.
  • The relevant tests, if any, have been updated or created - no relevant tests as far as I know

Testing instructions

I tested by directly putting a log message into the Ollama._streamFim() method.

Copy link

netlify bot commented Mar 3, 2025

Deploy Preview for continuedev ready!

Name Link
🔨 Latest commit 9d26892
🔍 Latest deploy log https://app.netlify.com/sites/continuedev/deploys/67d73d7a954f600008b5f915
😎 Deploy Preview https://deploy-preview-4448--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.

Copy link
Contributor

@sestinj sestinj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferenci84 rather than adding even more options to the config, I would much rather allow users to set maxTokens in the completionOptions section of their config for the model

@ferenci84
Copy link
Contributor Author

ferenci84 commented Mar 16, 2025

The main part of this modification is not the new config option, but the ability to set different default for autocomplete (possibly for individual providers). autocompleteMaxTokens is a technical thing (to let us set default maxTokens for autocomplete for the model without knowing whether that model will be used for autocomplete or something different; and it also allows providers to set maxTokens setting specifically for autocomplete use), the question is whether to document or keep it hidden from users, that's why I didn't add it to the documentation without knowing whether the core team want users being able to tinker with this option or just keep it hidden and/or unavailable (there is an option for both, see my next comment).

@ferenci84 ferenci84 requested a review from a team as a code owner March 16, 2025 21:07
@ferenci84 ferenci84 requested review from Patrick-Erichsen and removed request for a team March 16, 2025 21:07
@ferenci84
Copy link
Contributor Author

@sestinj Please look at this: these are possible places to set defaults for maxTokens for autocomplete:

This is setting for global default:
Screenshot 2025-03-16 at 21 45 35

There may also be defaults for individual models:
Screenshot 2025-03-16 at 21 44 05

This is how the final maxTokens is set when making the request. As you can see, maxTokens user setting will always take precedence, the user don't even have to know about the extra key in the BaseCompletionOptions type:
Screenshot 2025-03-16 at 21 49 10

There may be a misunderstanding, I think it's better if the additional key exist just in the type, but not open to users (i.e. hidden, not documented), however they should be used for setting individual defaults for providers, there may be fast and possibly more capable providers that can output more lines of completion, while slower, or those that tend to go into repetition, should be limited.

I believe that adding this key to the BaseCompletionOptions type is a good way to keep it simple for us, developers. If you want to keep it simple for users too, and limit their options, there is a way for us to ignore this additional setting that comes from the user config:
Screenshot 2025-03-16 at 22 02 18

Any way, please let me know if you have an idea to make it better.

@ferenci84 ferenci84 requested a review from sestinj March 16, 2025 21:12
@halfline
Copy link
Contributor

out of curiosity, do you know how many tokens on average you're using for autocomplete? Autocomplete pulls in snippets from various parts of the code and it can get kind of lengthy (especially with the hole filler models). on my system, just doing a completion in the quickstart tutorial uses north of 800 tokens.

@ferenci84
Copy link
Contributor Author

@halfline I do not have such stats. You are talking about the contextLength setting. This modification is about maxTokens that is the limit on the output tokens.

1 similar comment
@ferenci84
Copy link
Contributor Author

@halfline I do not have such stats. You are talking about the contextLength setting. This modification is about maxTokens that is the limit on the output tokens.

Copy link
Collaborator

@RomneyDa RomneyDa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferenci84 let's hold on changes since not sure about this parameter yet, but some notes:

  • Would need to add YAML support (add to defaultCompletionOptions in YAML schema). See previous JSON-only PR and resulting issue
  • It seems like with this implementation maxAutocompleteTokens is redundant of maxTokens, since maxTokens will be ignored if maxAutocompleteTokens is present. E.g. it's not detecting if the model is currently being used for autocomplete so isn't different than just using maxTokens (correct me if I'm wrong?)
  • The default should probably live in BaseLLM or constants.js

@Patrick-Erichsen Patrick-Erichsen removed their request for review March 31, 2025 23:59
@ferenci84
Copy link
Contributor Author

  • E.g. it's not detecting if the model is currently being used for autocomplete so isn't different than just using maxTokens (correct me if I'm wrong?)

@RomneyDa, maxAutocompleteTokens is only used when the model is used for autocomplete. See that the maxAutocompleteTokens value is used only in the provideInlineCompletionItems function.

The bottom line: This modification is NOT to add a new parameter, but to keep the default maxToken parameter low, in the following conditions:

  1. The user doesn't set maxTokens explicitly
    AND
  2. The model is used for autocomplete

I was asked to submit a PR for this issue:
#3994 (comment)

The reason to add a new key to the BaseCompletionOptions, and store the default there, is to avoid ovecomplicating things with new types. It can be a different decision whether it's good idea to add a new parameter or not (I would vote NOT), it's not part of this modification, so json or yaml config schema shouldn't be changed.

The default should probably live in BaseLLM or constants.js

I agree. It's possible to add a default per provider. Just as we have default cls.defaultOptions?.completionOptions?.maxTokens, we also have default cls.defaultOptions?.completionOptions?.autoCompleteMaxTokens, except that I added 256 fallback if it's not set (as it will not be set for most providers anyway). We can move the 256 default to any other place.

Please let me know if we are on the same page about the other two questions before I make any further modifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants