separate out observability logic #535
Conversation
There was a problem hiding this comment.
Seems like a good idea to abstract away some of the observability logic. It's a little bit unfortunate that we have to "pass around" this observer through function calls, but I don't have a better proposal at the moment.
I'd be in favor of moving the changes to RunContext into a different PR.
Would love to have a quick call (maybe Monday) to ask a few more questions here just to improve my understanding - motivation for this abstraction pattern, etc.
| prompt: str | ||
| """The original user prompt passed to the run.""" | ||
| model_selection_mode: ModelSelectionMode | ||
| """Dependencies for the agent.""" | ||
| retry: int = 0 | ||
| """Number of retries so far.""" | ||
| run_step: int = 0 | ||
| """The current step in the run.""" |
There was a problem hiding this comment.
Awesome to add these, but can we do it in a different PR that focuses just on this enhancement?
| ModelSelectionMode = Literal['from-agent', 'custom', 'override'] | ||
| """How the [model][pydantic_ai.models.Model] was selected for the run. | ||
| Meanings: | ||
| - `'from-agent'`: The model set on the agent was used | ||
| - `'custom'`: The model was set via the `model` kwarg, e.g. on [`run`][pydantic_ai.Agent.run] | ||
| - `'override'`: The model was set by the [`override`][pydantic_ai.Agent.override] function. | ||
| """ |
There was a problem hiding this comment.
Bikeshedding a bit here, could we be even more explicit?
Like agent, run-kwarg, and override? Feel free to take or leave these particular suggestions.
I'm hesitant to use "from" with agent, but not any of the others, and "custom" seems not specific enough...
| deps = self._get_deps(deps) | ||
| new_message_index = len(message_history) if message_history else 0 | ||
| run_context = RunContext(deps, [], None, model_used, user_prompt, model_selection_mode) | ||
| # noinspection PyTypeChecker |
There was a problem hiding this comment.
Why do we need this here?
There was a problem hiding this comment.
Maybe we could name this observability?
|
Overall I don't think this is a good idea, but there are a few good bits we should take out into a new PR. |
|
replaced by #570. |
should improve performance a tiny bit, and also will make it easier to replace
last_messages.