-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
adding finetuned model setup #62838
adding finetuned model setup #62838
Conversation
@@ -170,7 +188,7 @@ func (c *fireworksClient) makeRequest(ctx context.Context, feature types.Complet | |||
} | |||
|
|||
payload := fireworksRequest{ | |||
Model: requestParams.Model, | |||
Model: c.updateModelStringIfFinetunedModelId(requestParams.Model), |
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.
Hi @rafax
My local cody gateway setup is not working, can you please verify this re-routing logic for the fine tuning model string.
You want |
Ah yes, sorry I did not run it since my local gateway is not working, I updated the commands in the description |
internal/completions/types/types.go
Outdated
@@ -65,6 +65,7 @@ type CompletionRequestParameters struct { | |||
Model string `json:"model,omitempty"` | |||
Stream *bool `json:"stream,omitempty"` | |||
Logprobs *uint8 `json:"logprobs"` | |||
LanguageId string `json:"languageId,omitempty"` |
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.
Hi @rafax
I added this new parameter for language level routing to fireworks models. Although it should empty by default, could you confirm this shouldn't break any existing functionality ?
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.
@rafax do we still need changes here after shifting them to 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.
Removed
@@ -152,6 +155,50 @@ func (c *fireworksClient) Stream( | |||
return dec.Err() | |||
} | |||
|
|||
func (c *fireworksClient) updateModelStringIfFinetunedModelId(languageId string, model string) string { |
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.
Note that this code is in Sourcegraph backend (no cody-gateway
in the path), so it will not be executed for PLG requests (which bypass sourcegraph.com). If you want this to apply to PLG, you need to modify cmd/cody-gateway/internal/httpapi/completions/fireworks.go (ex. pickStarCoderModel
function)
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.
Hi @rafax do we need this from here ?
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.
Removed
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.
Move code to Cody Gateway
Move code for rewriting fine-tuned models
|
||
body.Model = pickStarCoderModel(body.Model, f.config) | ||
body.Model = pickFineTunedModel(body.Model, body.LanguageID) |
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.
Hi @rafax
did not notice this before merging the PR, but doing this body.LanguageID = ""
in line 142 for the condition body.LanguageID
will always make languageId
empty. I tested it locally and it was always returning the all language completions. Adding a minor fix for this, which I tested locally.
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.
Right, I updated the E2E tests to catch similar issues (verifying we resolve the right model)
Context
Test plan