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
sentrivana
left a comment
There was a problem hiding this comment.
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.
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
szokeasaurusrex
left a comment
There was a problem hiding this comment.
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_callbackshaving 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.