-
Notifications
You must be signed in to change notification settings - Fork 240
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
Autocomplete: Mark Ollama support experimental #3077
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a non blocker question inline. lgtm
if (autocompleteAdvancedProvider === 'unstable-fireworks') { | ||
autocompleteAdvancedProvider = 'fireworks' | ||
let autocompleteAdvancedProvider = config.get< | ||
Configuration['autocompleteAdvancedProvider'] | 'unstable-ollama' | 'unstable-fireworks' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we set the default value to experimental-ollama
as true if unstable-ollama
is available, so that we don't have to check for both names in the code repeatedly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow but isn't this exactly happening here? only experimental-ollama
is supported but here we handle unstable-ollama
explicitly in just one area: https://github.com/sourcegraph/cody/pull/3077/files/89672b78997a6d8dbc58dd354afa8247fab8a4e8#diff-85c89382076aeb49c8645dd41e6422552fba7bbf25575701c1a9253027164a76R62-R63
😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh i mean to replace the null
in (CONFIG_KEY.autocompleteAdvancedProvider, null)
with experimental-ollama
if unstable-ollama
is available so you don't have to check for both in create-provider
?
cody/vscode/src/completions/providers/create-provider.ts
Lines 52 to 53 in 89672b7
case 'experimental-ollama': | |
case 'unstable-ollama': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh my diff view got cut off, i saw what you meant now lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh gotcha the null is actually or the case when no provider is set and needs to work too, independent of this change
- `fireworks` became stable in sourcegraph/cody#1363 - `unstable-codegen` was removed in sourcegraph/cody#1364 - `unstable-ollama` was renamed to `experimental-ollama` in sourcegraph/cody#3077 ## Test plan Tested manually: ``` echo 'cody.autocomplete.advanced.provider=fireworks' > ~/.sourcegraph-jetbrains.properties ``` Open a file and generate an autocompletion. The Cody log should contain: `CodyCompletionProvider:initialized: fireworks/starcoder-hybrid`
Test plan
"cody.autocomplete.advanced.provider": "unstable-ollama"
and"cody.autocomplete.advanced.provider": "experimental-ollama"
in the settings