This repository was archived by the owner on Sep 30, 2024. It is now read-only.
Relax modelconfig name restrictions#64161
Merged
Merged
Conversation
chrsmith
commented
Jul 30, 2024
| func SanitizeResourceName(name string) string { | ||
| // Start by just converting everything to lower-case, so that | ||
| // the result doesn't look too awkward. | ||
| name = strings.ToLower(name) |
Contributor
Author
There was a problem hiding this comment.
This wasn't exactly called out in the bug, but seemed like a good change to make.
If we were automatically converting things to lower case as part of the sanitization process, it would lead to confusion. e.g. "I put 'FOO' in the site config but the model ID is 'foo'".
emidoots
approved these changes
Jul 30, 2024
emidoots
left a comment
Member
There was a problem hiding this comment.
Looks good, test cases also look good - thank you!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR greatly relaxes the naming restrictions for resource IDs when using the new
modelconfigLLM model configuration system.The original intent was to set a high bar for how we would allow users to refer to LLM providers, API versions, or models. So that we could have use those names in URLs and/or display them if no human-friendly name was supplied. e.g.
opanai::v1::gpt-3.5-turbo.However, in practice, this was just a bit too optimistic. We already had carved out an exception for "API versions" to be less strict, since there were some cases where we saw those included slashes and things. And one of our trusted testers immediately ran into problems because the model name had an
@in it. (See the linked issue.)In order to avoid perpetually trying to work around LLM providers and whatever IDs they use for things, it seemed preferable to just relax our rules.
With this PR, we will constraint Provider, API Versions, and Model IDs to only be URL-safe(*). So previously characters like
[]@/+;!were prohibited, they are now allowed.) However, we still restrict names to only includea-zA-Zand no unicode characters, and curly braces are not allowed. (But[]and()are.Anyways, this should provide enough flexibility to avoid most (hopefully all) problems users would encounter. While still enforcing some type of standards with regard to how models are defined.
Fixes PRIME-451 : invalid ModelID (claude-3-5-sonnet@20240620) after upgrading to 5.5.2463.)
Test plan
Added tests.
Changelog
NA.