Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

feat(langchain): Support BaseCallbackManager #4486

wants to merge 1 commit into from

Conversation

szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Jun 17, 2025 •
edited
Loading

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.

Fixes #4537

Thank you for contributing to sentry-python! Please add tests to validate your changes, and lint your code using tox -e linters.

Running the test suite on your PR might require maintainer approval.

@codecov Codecov
Copy link

codecov bot commented Jun 17, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 68.42105% with 6 lines in your changes missing coverage. Please review.

Project coverage is 80.66%. Comparing base (7804260) to head (d7efd46).
Report is 1 commits behind head on master.

❌ Unsupported file format

Upload processing failed due to unsupported file format. Please review the parser error message: 
Error parsing JUnit XML in /home/runner/work/sentry-python/sentry-python/.junitxml at 1:1649

Caused by:

RuntimeError: Error parsing XML


Caused by:
    0: ill-formed document: close tag `</testsuite>` does not match any open tag
    1: close tag `&lt;/testsuite&gt;` does not match any open tag</code></pre>

For more help, visit our troubleshooting guide.

Files with missing lines Patch % Lines
sentry_sdk/integrations/langchain.py 68.42% 4 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4486      +/-   ##
==========================================
- Coverage   80.69%   80.66%   -0.03%     
==========================================
  Files         156      156              
  Lines       16474    16487      +13     
  Branches     2799     2801       +2     
==========================================
+ Hits        13294    13300       +6     
- Misses       2298     2301       +3     
- Partials      882      886       +4
Files with missing lines Coverage Δ
sentry_sdk/integrations/langchain.py 70.90% <68.42%> (+0.77%) ⬆️

... and 3 files with indirect coverage changes

Base automatically changed from szokeasaurusrex/refactor-langchain-args to master June 24, 2025 12:37
@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review June 24, 2025 12:48
@szokeasaurusrex szokeasaurusrex requested a review from a team as a code owner June 24, 2025 12:48
@szokeasaurusrex szokeasaurusrex marked this pull request as draft June 24, 2025 12:48
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/langchain-support-manager branch 3 times, most recently from a2a6b6f to beec36c Compare June 25, 2025 09:44
@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review June 25, 2025 09:45
@szokeasaurusrex
feat(langchain): Support BaseCallbackManager
d7efd46 
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.
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/langchain-support-manager branch from beec36c to d7efd46 Compare June 25, 2025 09:53
@szokeasaurusrex szokeasaurusrex mentioned this pull request Jun 30, 2025
Open
sentrivana
sentrivana approved these changes Jun 30, 2025
View reviewed changes
Copy link
Contributor

@sentrivana sentrivana left a 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
integration.tiktoken_encoding_name,
),
]
local_callbacks = local_callbacks_with_sentry()
Copy link
Contributor

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.

@szokeasaurusrex szokeasaurusrex marked this pull request as draft July 1, 2025 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sentrivana sentrivana sentrivana approved these changes

@antonpirker antonpirker Awaiting requested review from antonpirker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Langchain Integration adds redundant chain callbacks
2 participants
@szokeasaurusrex @sentrivana