-
Notifications
You must be signed in to change notification settings - Fork 555
feat(langchain): Support BaseCallbackManager
#4486
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
feat(langchain): Support BaseCallbackManager
#4486
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #4486 +/- ##
==========================================
- Coverage 80.76% 80.68% -0.08%
==========================================
Files 156 156
Lines 16483 16496 +13
Branches 2801 2806 +5
==========================================
- Hits 13312 13310 -2
- Misses 2290 2303 +13
- Partials 881 883 +2
|
a2a6b6f
to
beec36c
Compare
beec36c
to
d7efd46
Compare
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.
Thanks for adding! Looks ok but would prefer we simplify a bit, see comment
sentry_sdk/integrations/langchain.py
Outdated
integration.tiktoken_encoding_name, | ||
), | ||
] | ||
local_callbacks = local_callbacks_with_sentry() |
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.
This function-based approach seems a bit complicated. It took me longer to parse than necessary.
I'd prefer we got rid of the local_callbacks_with_sentry
functions altogether and move/inline the logic here to keep it simple.
While implementing #4479, I noticed that our Langchain integration lacks support for the `local_callbacks` having type `BaseCallbackManager`, which according to the type hint is possible. This change adds support for this case.
d7efd46
to
048c148
Compare
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.
Thanks for adding! Looks ok but would prefer we simplify a bit, see comment
Made this change, would appreciate if you can take a second look @sentrivana
While implementing #4479, I noticed that our Langchain integration lacks support for the
local_callbacks
having typeBaseCallbackManager
, which according to the type hint is possible.This change adds support for this case.
Fixes #4537
Thank you for contributing to
sentry-python
! Please add tests to validate your changes, and lint your code usingtox -e linters
.Running the test suite on your PR might require maintainer approval.