-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: Add id and finish_reason to ModelResponse #2325
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -789,8 +789,40 @@ class ModelResponse: | |||||
For OpenAI models, this may include 'logprobs', 'finish_reason', etc. | ||||||
""" | ||||||
|
||||||
vendor_id: str | None = None | ||||||
"""Vendor ID as specified by the model provider. This can be used to track the specific request to the model.""" | ||||||
id: str | None = None | ||||||
"""Unique identifier for the model response, e.g. as returned by the model provider (OpenAI, etc).""" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
finish_reason: str | None = None | ||||||
fatelei marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As in the comment below, I think this should only support the values from https://opentelemetry.io/docs/specs/semconv/gen-ai/gen-ai-spans/#genai-attributes. If the model returns something else, the raw value can go into |
||||||
"""The reason the model finished generating this response, e.g. 'stop', 'length', etc.""" | ||||||
|
||||||
@property | ||||||
def vendor_id(self) -> str | None: | ||||||
"""Vendor ID as specified by the model provider. This can be used to track the specific request to the model. | ||||||
|
||||||
This is deprecated, use `id` instead. | ||||||
""" | ||||||
import warnings | ||||||
|
||||||
warnings.warn('vendor_id is deprecated, use id instead', DeprecationWarning, stacklevel=2) | ||||||
return self.id | ||||||
|
||||||
@vendor_id.setter | ||||||
def vendor_id(self, value: str | None) -> None: | ||||||
"""Set the vendor ID. | ||||||
|
||||||
This is deprecated, use `id` instead. | ||||||
""" | ||||||
import warnings | ||||||
|
||||||
warnings.warn('vendor_id is deprecated, use id instead', DeprecationWarning, stacklevel=2) | ||||||
self.id = value | ||||||
|
||||||
def __post_init__(self) -> None: | ||||||
"""Ensure vendor_details contains finish_reason for backward compatibility.""" | ||||||
if self.finish_reason and self.vendor_details is None: | ||||||
self.vendor_details = {} | ||||||
if self.finish_reason and self.vendor_details is not None: | ||||||
self.vendor_details['finish_reason'] = self.finish_reason | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic here can be simplifed a bit: the first |
||||||
|
||||||
def otel_events(self, settings: InstrumentationSettings) -> list[Event]: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue mentions:
Can you please handle that here as well so this PR can close that issue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that I also left some other related suggestions on that older PR that tried to add |
||||||
"""Return OpenTelemetry events for the response.""" | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,6 +137,8 @@ async def request( | |
if not response.usage.has_values(): # pragma: no branch | ||
response.usage = _estimate_usage(chain(messages, [response])) | ||
response.usage.requests = 1 | ||
response.id = getattr(response, 'id', None) | ||
response.finish_reason = getattr(response, 'finish_reason', None) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these lines necessary? |
||
return response | ||
|
||
@asynccontextmanager | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -273,15 +273,16 @@ def _process_response(self, response: _GeminiResponse) -> ModelResponse: | |
parts = response['candidates'][0]['content']['parts'] | ||
vendor_id = response.get('vendor_id', None) | ||
finish_reason = response['candidates'][0].get('finish_reason') | ||
vendor_details = {} | ||
if finish_reason: | ||
vendor_details = {'finish_reason': finish_reason} | ||
vendor_details['finish_reason'] = finish_reason | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As in the other PR that worked on this (#1882 (comment)), I think we should keep the raw value here and then add |
||
usage = _metadata_as_usage(response) | ||
usage.requests = 1 | ||
return _process_response_from_parts( | ||
parts, | ||
response.get('model_version', self._model_name), | ||
usage, | ||
vendor_id=vendor_id, | ||
id=vendor_id, | ||
vendor_details=vendor_details, | ||
) | ||
|
||
|
@@ -662,7 +663,7 @@ def _process_response_from_parts( | |
parts: Sequence[_GeminiPartUnion], | ||
model_name: GeminiModelName, | ||
usage: usage.Usage, | ||
vendor_id: str | None, | ||
id: str | None, | ||
vendor_details: dict[str, Any] | None = None, | ||
) -> ModelResponse: | ||
items: list[ModelResponsePart] = [] | ||
|
@@ -680,9 +681,7 @@ def _process_response_from_parts( | |
raise UnexpectedModelBehavior( | ||
f'Unsupported response from Gemini, expected all parts to be function calls or text, got: {part!r}' | ||
) | ||
return ModelResponse( | ||
parts=items, usage=usage, model_name=model_name, vendor_id=vendor_id, vendor_details=vendor_details | ||
) | ||
return ModelResponse(parts=items, usage=usage, model_name=model_name, id=id, vendor_details=vendor_details) | ||
|
||
|
||
class _GeminiFunctionCall(TypedDict): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -227,23 +227,44 @@ def _request( | |
output[part.tool_name] = part.content | ||
if output: | ||
return ModelResponse( | ||
parts=[TextPart(pydantic_core.to_json(output).decode())], model_name=self._model_name | ||
parts=[TextPart(pydantic_core.to_json(output).decode())], | ||
model_name=self._model_name, | ||
id=None, | ||
finish_reason=None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As these fields are optional, we shouldn't need to include them here (and below) |
||
) | ||
else: | ||
return ModelResponse(parts=[TextPart('success (no tool calls)')], model_name=self._model_name) | ||
return ModelResponse( | ||
parts=[TextPart('success (no tool calls)')], | ||
model_name=self._model_name, | ||
id=None, | ||
finish_reason=None, | ||
) | ||
else: | ||
return ModelResponse(parts=[TextPart(response_text)], model_name=self._model_name) | ||
return ModelResponse( | ||
parts=[TextPart(response_text)], | ||
model_name=self._model_name, | ||
id=None, | ||
finish_reason=None, | ||
) | ||
else: | ||
assert output_tools, 'No output tools provided' | ||
custom_output_args = output_wrapper.value | ||
output_tool = output_tools[self.seed % len(output_tools)] | ||
if custom_output_args is not None: | ||
return ModelResponse( | ||
parts=[ToolCallPart(output_tool.name, custom_output_args)], model_name=self._model_name | ||
parts=[ToolCallPart(output_tool.name, custom_output_args)], | ||
model_name=self._model_name, | ||
id=None, | ||
finish_reason=None, | ||
) | ||
else: | ||
response_args = self.gen_tool_args(output_tool) | ||
return ModelResponse(parts=[ToolCallPart(output_tool.name, response_args)], model_name=self._model_name) | ||
return ModelResponse( | ||
parts=[ToolCallPart(output_tool.name, response_args)], | ||
model_name=self._model_name, | ||
id=None, | ||
finish_reason=None, | ||
) | ||
|
||
|
||
@dataclass | ||
|
Uh oh!
There was an error while loading. Please reload this page.