-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add OpenRouterModel as OpenAIChatModel subclass #3089
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
base: main
Are you sure you want to change the base?
Conversation
DouweM
left a comment
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.
@ajac-zero Muchas gracias Anibal!
|
Buen día @DouweM, can you take a look when you get the chance? |
DouweM
left a comment
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.
Gracias!
It'd be interesting to add support for the WebSearchTool built-in tool as well, shouldn't be too complicated I think: https://openrouter.ai/docs/features/web-search
|
@ajac-zero We can also remove this comment from openai.py: pydantic-ai/pydantic_ai_slim/pydantic_ai/models/openai.py Lines 558 to 560 in c5b1495
|
Co-authored-by: Douwe Maan <[email protected]>
Co-authored-by: Douwe Maan <[email protected]>
|
Hi, just found this useful pr and I think top_k and other missing model config should be added to align with the Request Schema documented here https://openrouter.ai/docs/api-reference/overview. |
| provider=OpenRouterProvider( | ||
| api_key='your-openrouter-api-key', | ||
| http_referer='https://your-app.com', | ||
| x_title='Your App', |
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.
It's a small thing, but since these map to headers and are documented in https://openrouter.ai/docs/app-attribution as being headers, would it make sense to expose the entire headers option, or document how to set headers via a custom client? That's the route we've taken in other providers -- I don't think we've exposed specific headers as arguments, except for auth etc.
I could see these making sense if we name them for the purpose, rather than the specific header they map to, e.g. app_url and app_title, matching how they're described in the docs. If they're not just headers but an "OpenRouter feature" to configure, dedicated args feel more natural. That'd also warrant an App Attribution section 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.
I prefer the second option, I would consider it a proper feature that should be supported.
| provider = infer_provider('gateway/openai' if provider == 'gateway' else provider) | ||
| self._provider = provider | ||
| self.client = provider.client | ||
|
|
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.
Nice, I like it, this is a bit similar to the recently introduced EventStream and I think it'd be a great direction to move these model classes into, if we can find something that's expressive enough for all of the crazy stuff that's happening in model message mapping...
| usage=_map_usage(response, self._provider.name, self._provider.base_url, self._model_name), | ||
| usage=self._map_usage(response), | ||
| model_name=response.model, | ||
| timestamp=timestamp, |
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.
Could we pass this into a method on the new context object?
| parts=items, | ||
| usage=_map_usage(response, self._provider.name, self._provider.base_url, self._model_name), | ||
| usage=self._map_usage(response), | ||
| model_name=response.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.
Typo: respose -> response
| data = _BaseReasoningDetail.model_validate_json(thinking_part.id) | ||
|
|
||
| if data.type == 'reasoning.text': | ||
| return _ReasoningText( |
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.
Now that we're in BaseModel land, we can use type as a union discriminator: https://docs.pydantic.dev/latest/concepts/unions/#discriminated-unions, and have TypeAdapter(A | B | C).validate_json() automatically deserialize as the correct subclass.
Looking at the code though, I'm feeling a little bad about abusing the id field like this. (I felt less bad about it when I suggested basically type|id|etc|etc, but I agree that's gonna be a mess with so many fields and potentially more in the future.)
Adding an arbitrary-provider-metadata field to parts has come up a couple of times recently (#3453, #3474), so it may be time to bite the bullet and do it. provider_details: dict[str, Any] | None = None (like the same field on ModelResponse) seems reasonable to me. Would you be up for adding it to the PR? (I'd also understand if you'd rather get this PR over with, in which case you can do it the id way and I/you/someone can do the other thing in a followup, as it can be backward compatible)
|
|
||
|
|
||
| class OpenRouterChatCompletion(chat.ChatCompletion): | ||
| class _OpenRouterCostDetails(BaseModel): |
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.
Can these be dataclasses? Pydantic supports those as well, and they're more light weight than BaseModels.
| provider_details: dict[str, Any] = {} | ||
|
|
||
| provider_details['downstream_provider'] = response.provider | ||
| provider_details['native_finish_reason'] = response.choices[0].native_finish_reason |
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.
Our provider_details['finish_reason'] is essentially the same thing as OpenRouter's native_finish_reason, as in the original value before being mapped to some abstraction layer. So it feels natural to me to call it finish_reason here. We'd "lose" OpenRouter's own intermediate value, but not really because that maps to the response.finish_reason values (I think).
I'm just thinking about code somewhere that may look for provider_details['finish_reason'] and expect it to the the ultimate original value
Hi! This pull request takes a shot at implementing a dedicated
OpenRouterModelmodel. Closes #2936.The differentiator for this PR is that this implementation minimizes code duplication as much as possible by delegating the main logic to
OpenAIChatModel, such that the new model class serves as a convenience layer for OpenRouter specific features.The main thinking behind this solution is that as long as the OpenRouter API is still fully accessible via the
openaipackage, it would be inefficient to reimplement the internal logic using this same package again. We can instead use hooks to achieve the requested features.I would like to get some thoughts on this implementation before starting to update the docs.
Addressed issues
Provider metadata can now be accessed via the 'downstream_provider' key in ModelMessage.provider_details:
The new
OpenRouterModelSettingsallows for the reasoning parameter by OpenRouter, the thinking can then be accessed as aThinkingPartin the model response:errorresponse from OpenRouter as exception instead of validation failure #2323. Closes OpenRouter uses non-compatible finish reason #2844These are dependent on some downstream logic from OpenRouter or their own downstream providers (that a response of type 'error' will have a >= 400 status code), but for most cases I would say it works as one would expect:
OpenRouterModel#1870 (comment)Add some additional type support to set the provider routing options from OpenRouter: