1/3 Support Model rewrite capability - Configuration format change#151
1/3 Support Model rewrite capability - Configuration format change#151J3rome wants to merge 8 commits into
Conversation
J3rome
changed the title
| .map(|s| glob::Pattern::new(&s).map_err(serde::de::Error::custom)) | ||
| .collect() | ||
| } | ||
|
|
There was a problem hiding this comment.
I feel like this belong to config.rs, it even use object from this module.
This module is an util module and aim to deal with "generic" serialization stuff.
There was a problem hiding this comment.
Yeah I agree. Will move
| - `{ "rewrite": "<target>" }` ⇒ the model is allowed and the upstream is called with `<target>` instead. In the example, requests for `us.anthropic.claude-opus-4-5*` are sent to Bedrock as the configured application-inference-profile ARN. | ||
|
|
||
| Notes: | ||
| - Ordering matters: the **first matching pattern wins** for rewrite resolution. Authorization is order-independent (any match allows). |
There was a problem hiding this comment.
If ordering matters, then it shouldn't be a dict. Otherwise people have to serialize a dict with ordered keys, which almost no library supports?
I suggest moving this to a list (sorry we didn't think of this during design!)
There was a problem hiding this comment.
Maybe now is a good time to ask the question: Do we really want to keep priority based on the ordering ?
Another way we could approach this would be to make the more specific pattern win
ex : claude-opus-* win over claude-* regardless of the ordering.
This way we don't have to duplicate the model key in the config to support rewrite & other capabilities and it's also less error prone then relying on ordering IMO
There was a problem hiding this comment.
I almost suggested to do the more specific pattern and I had given up because I was thinking of regexes, but its true that with fnmatch, just counting characters isn't that bad.
LGTM for your idea!
There was a problem hiding this comment.
@leameow Thoughts ?
If your good with that i'll implement this in a separate PR
There was a problem hiding this comment.
Yeah, good idea! I've seen this in other systems as well, 🚢 🇮🇹
There was a problem hiding this comment.
ok so remove this line from the docs and merge for now?
| } | ||
|
|
||
| #[test] | ||
| fn preserves_declaration_order_when_deserializing_model_rules() { |
| rewrite: Option<String>, | ||
| } | ||
|
|
||
| /// Serialize `Vec<ModelRule>` as an ordered object: `pattern => { "rewrite"?: target }`. |
3 participants
This is PR 1/3 to enable the model rewrite capability.
What is in this PR
Change the provider.capability config format.
Was
Is now
It is essentially a no-op other than the configuration changes.