Skip to content
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

default maxTokens setting for autocomplete #4448

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

2 participants