2/3 Support Model rewrite capability - Enabling model rewrite for bedrock provider#152
2/3 Support Model rewrite capability - Enabling model rewrite for bedrock provider#152J3rome wants to merge 11 commits into
Conversation
| pub fn resolve_model_rewrite( | ||
| &self, | ||
| model: &str, | ||
| grant_model_rules: &[config::ModelRule], |
There was a problem hiding this comment.
It feels beyond the provider responsability.
I think we can deal with this in a cleaner way by extending an existing provider from a new capability config (something like:
let provider = provider.extend(grant_caps)
The grants could allow to override all the capability field in the provider config, and no code in the provider would need to be override aware.
There was a problem hiding this comment.
Just to be clear, my suggestion is to remove grant_model_rules and all related code.
There was a problem hiding this comment.
I agree that it's beyond the provider responsibility.
I'll move that logic to model_rewritte.rs where it belong.
That being said, I don't think we want to completely override the provider capabilities with the tailscale capabilities. We want that for the actual capabilities but not for the authorization.
The tailscale capabilities should not grant access to a model that is not already in the provider configuration. At least, that's not in line with what we are doing authorization wise right now (We first check the if the provider allow the model usage then the we check the capabilities).
There was a problem hiding this comment.
Moved everything to model_rewrite.rs. Let me know what you think
| request: axum::extract::Request, | ||
| rewrite: &ResolvedModelRewrite, | ||
| ) -> anyhow::Result<axum::extract::Request> { | ||
| crate::model_rewrite::rewrite_request_path(request, rewrite) |
There was a problem hiding this comment.
I'm not a fan of this blind search and replace rewrite approach. Bedrock as a clear API, we can simply swap a known part of the URL path? This way we shouldn't even need the ResolvedModelRewrite object. All we need is the desired model (a str).
There was a problem hiding this comment.
My goal was to make this generic so the provider basically only have to specify if we need to rewrite the model name in the url or in the body of the request.
But yeah, since we need to implement this method for each provider anyways, we could use the API knowledge to modify only the part we know we need to modify.
Just for completeness, I was also thinking of using a generic approach for the body of the request (out of scope for now). We wouldn't want to do a blind search and replace in this case because we don't want to mess with the user input that is part of the body and could include the model name. I was thinking of a generic model_rewrite::rewrite_request_body(request, key, rewrite).
|
|
||
| /// Apply `rewrite` to the request wherever this api type carries the model name (url or request body). | ||
| /// Defaults to leaving the request untouched (api types that don't extract a model never reach this path). | ||
| async fn rewrite_model_in_request( |
There was a problem hiding this comment.
It was made async because it will need to be when we rewrite the model name in the body request but you are right that right now it doesn't need to be.
We can make it sync for now and switch it back to async whenever we implement renaming in the body of the request
…n bedrock provider
2 participants
This is PR 2/3 to enable the model rewrite capability.
What is in this PR
rewrite_model_in_requesttrait which rewrite the model name in the/model/{model_name}/invokeurl.NOTE: We are not supporting override of the rewrite capability via Tailscale capabilities in this PR. This will come in the next PR