Cody Gateway: New Claude 3.5 Sonnet model#63395
Conversation
There was a problem hiding this comment.
This changes looks good to me, but before you submit could you please run go test ./internal/modelconfig and confirm that those unit tests pass?
The intention was that it would confirm that all provider and model IDs adhere to strict naming rules. So either (1) the tests aren't being run or (2) the logic/implementation is wrong, and allowing this to pass.
This isn't related to your specific change, so if you are rushing to get this out just merge this as-is and I can follow up with that later. (Since even if the models.json file is corrupted, it won't impact anything right now.)
i.e. this test, right here:
https://github.com/sourcegraph/sourcegraph/blob/b05bd5bb16ca89928d060b1574f51e5d765657a0/internal/modelconfig/validation_test.go#L42-L45
| // Pro Anthropic models. | ||
| newModel( | ||
| modelIdentity{ | ||
| MRef: mRef(anthropic_06_2023, "claude-3.5-sonnet"), |
There was a problem hiding this comment.
This should fail the unit tests. So if CI/CD isn't unhappy with this change, something is amiss.
Did you run go test ./internal/modelconfig/, and what did it say? (I assumed it the relevant tests would be ran automatically, but perhaps not?)
Anyways, the ModelID here "claude-3.5-sonnet" would fail the regular expression defined over in:
https://github.com/sourcegraph/sourcegraph/blob/main/internal/modelconfig/validation.go#L19
However, this seems like a reasonable enough change (allowing '.') that we should just update the regular expression for resource IDs.
⚠️ This would be a major no-no after we ship the server-side-model-config stuff, as it would be a change that would break downstream Sourcegraph instances. But since it hasn't actually shipped yet, updating the validation logic is A-OK.
There was a problem hiding this comment.
It's passing 👀
sourcegraph on bee/sonnet-3.5-model via 🐹 v1.22.4 via v20.8.1 on ☁️ (us-west-1) on ☁️ beatrix@sourcegraph.com
❯ go test ./internal/modelconfig/
ok github.com/sourcegraph/sourcegraph/internal/modelconfig 0.238s
Should we tickle this in a follow-up issue instead?
There was a problem hiding this comment.
@chrsmith If I understand it correctly, the linked regex should not only fail for claude-3.5-sonnet but also claude-3-sonnet, since it doesn't capture the dashes...?
There was a problem hiding this comment.
Sorry I wasn't clearer. The dashes aren't the problem, it's the period in "3.5". If the regex is [a-z][a-z-]*[a-z] HOW IS THAT ALLOWED?!?
There was a problem hiding this comment.
OHHHH this is why based on Cody:
In Go, a hyphen in a character class is interpreted as a range operator, not a literal hyphen. So, [a-z-] is equivalent to [a-z-] or [a-z\w]. This means that the regex is actually matching any word character (letters, numbers, or underscores) in addition to hyphens.
| const Claude3Haiku = "claude-3-haiku-20240307" | ||
| const Claude3Sonnet = "claude-3-sonnet-20240229" | ||
| const Claude3Opus = "claude-3-opus-20240229" | ||
| const Claude35Sonnet = "claude-3.5-sonnet-20240620" |
There was a problem hiding this comment.
Nit: There isn't a right or wrong way to name this. But FWIW, I've seen code bases that used an underscore where a period would be for readability. e.g. Claude3_5Sonnet. Or perhaps Claude3point5Sonnet or similar.
🤷 This looks weird ("Claude 35"), but I have no suggestion for how to improve it.
There was a problem hiding this comment.
this is the format we are using for Gemini too but i agree it looks weird. Will look into it in a follow-up PR and unify them!
CONTEXT: https://sourcegraph.slack.com/archives/C05AGQYD528/p1718898110684289?thread_ts=1718896254.676939&cid=C05AGQYD528
CLOSE https://linear.app/sourcegraph/issue/CODY-2177
Adding new Claude 3.5 Sonnet (
claude-3.5-sonnet-20240620) to the Cody Gateway allow list.Model ID based on Anthropic Console:
Claude 3.5 Sonnet is Live on s0.dev to confirm this is the correct model ID
Test plan
Verify you can use the new model through Cody Gateway
Changelog
feature(plg): new Claude 3.5 Sonnet model support for Cody Pro users