Pydantic agent and some important infra#39
Conversation
alexb-palisaderesearch
force-pushed
the
pydantic-agent
branch
from
6501650 to
7ad62fc
Compare
alexb-palisaderesearch
force-pushed
the
pydantic-agent
branch
from
f3de68f to
a1c8c32
Compare
alexb-palisaderesearch
requested a review
from sergey-koldyba
as a code owner
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ArtemPt239
left a comment
ArtemPt239
left a comment
There was a problem hiding this comment.
LGTM, except for Settings seemingly not being persistent
| Data here will *not* be a part of a run history. | ||
| """ | ||
|
|
||
| settings: Settings |
There was a problem hiding this comment.
IMO Settings should be a part of the run history (i.e. logged somewhere for each run).
This way we can restore what happened later
There was a problem hiding this comment.
It also should help with properly restoring the agent from state if the settings can be copied from the parent run
There was a problem hiding this comment.
They are a part of it - they do get committed to git from where a "certified" run gets its commit hash.
There was a problem hiding this comment.
Ah. Perhaps you've meant the config file that was used in the run? That'd be actually a great candidate for the run metadata example.
| scrubbing=False, | ||
| ) | ||
| logfire.instrument_openai() | ||
| # TODO: instrument other stuff like model providers |
There was a problem hiding this comment.
I personally don't know what "to instrument" means in this context(
There was a problem hiding this comment.
Wrapping and/or injecting code to provide observability (that is, instrumentation, like the gauges on a plane).
There was a problem hiding this comment.
Maybe add the extended description to the comment?
There was a problem hiding this comment.
I'd say we need a proper README for it
|
Also, failing build looks super sus |
![semgrep-code-palisaderesearch[bot]](https://avatars.githubusercontent.com/u/137441464?s=60&v=4){kind=small}
There was a problem hiding this comment.
I would suggest to cleanup comments if they're not required or add short explanation about available options.
There was a problem hiding this comment.
I would suggest to cleanup comments if they're not required or add short explanation why this part exists in commented state.
|
@sergey-koldyba , if the comments are not blocking, can you approve merge? |
3 participants
Later: