Add ModelRequestParameters and ModelSettings to the ModelRequest #3405
Conversation
mwildehahn
force-pushed
the
mh/tool-introspection
branch
2 times, most recently
from
dea4c44 to
acced35
Compare
| if TYPE_CHECKING: | ||
| from .models.instrumented import InstrumentationSettings | ||
|
|
||
| ModelRequestParametersField = ModelRequestParameters | None |
There was a problem hiding this comment.
Even though we don't serialize these, if we don't do this the TypeAdapter will try to inspect all the fields leading to issues with some httpx types included in the models.
There was a problem hiding this comment.
If this is the only way we can solve that, this should be a comment.
But I wonder if @Viicos has another idea. He's out today but will be back on Monday.
There was a problem hiding this comment.
Yes this is a reasonable pattern, but do note that this means that no validation will be performed (because it is Any in the else branch).
mwildehahn
force-pushed
the
mh/tool-introspection
branch
from
28e3d44 to
618e9a1
Compare
| model_settings = ctx.deps.model_settings | ||
| # Record metadata on the ModelRequest (the last request in the original history) | ||
| self.request.model_request_parameters = model_request_parameters | ||
| self.request.model_settings = model_settings |
There was a problem hiding this comment.
I think we should do this in Model.prepare_request instead, as model classes sometimes still perform their own modifications and we want to make sure we see catch the final values that were actually sent to the LLM.
| if TYPE_CHECKING: | ||
| from .tools import ToolDefinition | ||
| else: # pragma: no cover | ||
| ToolDefinition = Any |
| ) | ||
| """Full request parameters captured for this request. | ||
| Available for introspection during a run. This field is excluded from serialization. |
There was a problem hiding this comment.
"during a run" implies it'll be removed after the run, but that's not the case
| if TYPE_CHECKING: | ||
| from .models.instrumented import InstrumentationSettings | ||
|
|
||
| ModelRequestParametersField = ModelRequestParameters | None |
There was a problem hiding this comment.
If this is the only way we can solve that, this should be a comment.
But I wonder if @Viicos has another idea. He's out today but will be back on Monday.
| model_request_parameters: Annotated[ModelRequestParametersField, pydantic.Field(exclude=True, repr=False)] = field( | ||
| default=None, repr=False, compare=False | ||
| ) |
There was a problem hiding this comment.
I think the duplication of repr=False is not necessary? Same below.
3 participants
This allows us to inspect the resolved tools / settings that were sent with the request. We don't serialize these in the ModelRequest for now since it would bloat existing serialization unnecessarily.