-
Notifications
You must be signed in to change notification settings - Fork 563
feat(integrations): pydantic-ai integration #4906
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: master
Are you sure you want to change the base?
Conversation
Hey, added a commit to run the new test suite in ci c7e7ec2 -- the script updated the test matrix for other test suites too and some of them are now failing; I'll fix that on master and then we can rebase. |
### Description Updating tox + reorg the AI group alphabetically. New openai release doesn't work on 3.8, explicitly testing on 3.9+ from there Doing this now to unblock #4906 (comment) #### Issues <!-- * resolves: #1234 * resolves: LIN-1234 --> #### Reminders - Please add tests to validate your changes, and lint your code using `tox -e linters`. - Add GH Issue ID _&_ Linear ID (if applicable) - PR title should use [conventional commit](https://develop.sentry.dev/engineering-practices/commit-messages/#type) style (`feat:`, `fix:`, `ref:`, `meta:`) - For external contributors: [CONTRIBUTING.md](https://github.com/getsentry/sentry-python/blob/master/CONTRIBUTING.md), [Sentry SDK development docs](https://develop.sentry.dev/sdk/), [Discord community](https://discord.gg/Ww9hbqr)
Unrelated ci failures fixed. Do we have test apps/scripts for this? If so would be awesome if you could add them to https://github.com/getsentry/testing-sentry-python like Vjeran did for google-genai in getsentry/testing-sentry-python#1 |
@sentrivana I've pushed all tests I have here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments since I am not sure I'll get to reviewing everything in detail. There should be reviews from other SDK members coming.
tool_call_id = None | ||
|
||
# Handle ToolCallPart (assistant requesting tool use) | ||
if "ToolCall" in part.__class__.__name__: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matching on a string contained inside part.__class__.__name__
seems brittle.
Why do we not check isinstance(part, BaseToolCallPart)
to handle all relevant classes?
A similar comment applies to the other branches of the if-elif here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do that in other integrations as well (e.g in openai_agents), I'm fine with either solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a first look and left some comments. Some general feedback:
-
There's a lot of imports in functions, I've pointed out some but not all. Please go through all non-top-level imports and if it doesn't break anything, move them to the other imports at the top of the file.
-
The code is not easy to follow, mostly because related parts are disconnected.
- Big part of this is that span management is scattered, e.g. a span is created in a specific function/file, then
__enter__
ed in a different file, and the corresponding__exit__
is someplace else still. Sometimes, we don't even know if a span is active, like insentry_sdk/integrations/pydantic_ai/patches/agent_run.py
, which shouldn't be the case:
# It could be that there is an "invoke agent" span still open current_span = sentry_sdk.get_current_span() if current_span is not None and current_span.timestamp is None: current_span.__exit__(None, None, None)
It might be that most of this scattered management is simply a byproduct of how Pydantic AI/agentic flows work in general. But if we can do something to improve the flow we should.
- In general you need to do a lot of jumping around in the code to follow a specific code path. I think a big part of this is the number of new files introduced by this PR. Can we consolidate these a bit? I.e., if functions from a single file are only used in one other file, can we move them there?
- Big part of this is that span management is scattered, e.g. a span is created in a specific function/file, then
This would make the code more reviewable for me, since right now I'm struggling to grasp the bigger picture. 🙂
Co-authored-by: Ivana Kellyer <[email protected]>
Description
Integration for Pydantic AI. Support for sync and async (streaming and non-streaming)
Issues
Closes https://linear.app/getsentry/issue/TET-1242/python-sdk-pydantic-ai-integration
Reminders
tox -e linters
.feat:
,fix:
,ref:
,meta:
)