feat: add provider agnostic call decorator #740
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release/v1.14 #740 +/- ##
=================================================
Coverage 100.00% 100.00%
=================================================
Files 407 457 +50
Lines 14669 16733 +2064
=================================================
+ Hits 14669 16733 +2064
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mirascope/core/base/types.py
Outdated
|
|
||
|
|
||
| FinishReason: TypeAlias = Literal[ | ||
| "stop", "length", "tool_calls", "content_filter", "function_call" |
There was a problem hiding this comment.
nit: i think we can drop "function_call" since that is deprecated?
|
|
||
| @property | ||
| def common_usage(self) -> Usage | None: | ||
| if self.input_tokens is None and self.output_tokens is None: |
There was a problem hiding this comment.
why have a shared utility for the finish reason conversion but not he usage creation?
is there any reason not to just have shared utilities for all of these? Even if it's just a simple cast line so that any changes in the future happen only in one place?
|
|
||
| @property | ||
| def common_message_param(self) -> BaseMessageParam: | ||
| return _convert_parts_to_base_message_param(self.response.parts) |
There was a problem hiding this comment.
why have a utility here for gemini and not the other providers?
|
|
||
| @property | ||
| def common_message_param(self) -> BaseMessageParam: | ||
| raise NotImplementedError |
There was a problem hiding this comment.
are we planning on implementing this?
| return cast(list[FinishReason], self.finish_reasons) | ||
|
|
||
| @property | ||
| def common_message_param(self) -> BaseMessageParam: |
There was a problem hiding this comment.
realizing that we need to update BaseMessageParam to account for tool calls. This means we'll need to add support for role="tool" (including whatever other metadata tool messages might have and converting these into provider-specific tool messages, which we currently don't do because we assume the user is using provider-specific tool messages right now).
There was a problem hiding this comment.
we should try our best to match the utilities across all providers so that the structure of the codebase remains consistent.
For example, if we're going to have a utility for one provider, that means we should have the same utility for all providers. This will also help to ensure that we maintain consistent interfaces both externally and internally.
There was a problem hiding this comment.
I have created ToolCallPart and ToolResultPart and added processing for them.
However, currently there are no use cases for ToolCallPart.
There was a problem hiding this comment.
I realized that while we plan to convert ToolResultPart to provider-specific Messages, there's a constraint that tool_results can only be sent to the provider that requested the tool because a tool_call_id is required.
There was a problem hiding this comment.
in line with my previous comment, this is missing a provider-agnostic version of tool_message_params
mirascope/llm/llm_call.py
Outdated
|
|
||
| return inner | ||
|
|
||
| def _wrap_result(result: BaseCallResponse | Stream) -> CallResponse | Stream: |
There was a problem hiding this comment.
nit: define this above wrapper since inner uses this method (or pull it out into a private utility so we don't define it every time?)
mirascope/llm/stream.py
Outdated
There was a problem hiding this comment.
this is also missing the provider-agnostic tool_message_params method.
No description provided.