-
Notifications
You must be signed in to change notification settings - Fork 20.5k
fix(core): add tracing callbacks to RouterRunnable
#34554
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?
fix(core): add tracing callbacks to RouterRunnable
#34554
Conversation
CodSpeed Performance ReportMerging this PR will not alter performanceComparing
|
c1ab981 to
2c369c4
Compare
|
Flaky test failure - I'm going to re-run by pushing an empty commit. |
horus-bot
left a comment
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.
The self.name = "RouterRunnable" assignment in init is redundant since you're already passing name="RouterRunnable" to super().init().
RouterRunnable
aaron-seq
left a comment
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.
Code Review Summary
This PR successfully addresses the tracing callback issue in RouterRunnable. The implementation is well-structured and correctly integrates callback management for proper tracing visibility.
CI/CD Failure Analysis
Failures Identified:
1. CI Success Check (Required) - Exit code 1
The check shows "All Checks Passed" but exits with code 1, indicating a potential issue with the check aggregation logic itself rather than the code changes.
2. lint (libs/langchain, 3.14) / Python 3.14 - Exit code 2
This failure is due to infrastructure issues, not code quality:
- Failed to download Python 3.14 build from astral-sh/python-build-standalone
- HTTP 500 Internal Server Error from GitHub's release server
- Dependency installation failure after 3 retries
Conclusion: Both CI failures are infrastructure/environment-related, not caused by code issues in this PR.
Code Logic Review
Implementation Quality: Excellent
The changes correctly implement tracing callbacks for RouterRunnable:
Positive Aspects:
-
Proper Callback Manager Integration
- Correctly creates CallbackManager with inherited callbacks, tags, and metadata
- Properly configures run_manager for chain lifecycle tracking
- Uses
on_chain_start,on_chain_error, andon_chain_endappropriately
-
Correct Child Config Handling
- Creates proper child config with
run_manager.get_child() - Uses
set_config_contextto ensure proper context propagation - Maintains config isolation between parent and child runnables
- Creates proper child config with
-
Error Handling
- Comprehensive try-except-else pattern
- Proper error reporting via
on_chain_errorbefore re-raising - Handles both sync (invoke) and async (ainvoke) methods identically
-
Name Fix
- Setting
name="RouterRunnable"in__init__ensures correct tracing name - Previously defaulted to "RunnableSequence" causing confusion
- Setting
-
Test Updates
- Updated assertions reflect the new trace structure
- Now correctly expects
RouterRunnablename and proper child run nesting - Changed from 2 direct children to 1 child with 2 nested children (correct structure)
Code Quality Observations:
Line 87: Adding explicit name parameter
name="RouterRunnable",This is essential for proper identification in traces. Good fix.
Lines 114-149 (invoke): Callback integration
callback_manager = cb_manager.CallbackManager.configure(...)
run_manager = callback_manager.on_chain_start(...)Follows LangChain's callback pattern correctly.
Lines 137-149: Context and config handling
with set_config_context(child_config) as context:
output = context.run(runnable.invoke, actual_input, child_config)Proper use of context manager ensures config is available during execution.
Lines 180-181: Error propagation
await run_manager.on_chain_error(e)
raiseCorrect pattern: report error to callbacks before re-raising.
Test Changes (lines 2906-2908):
assert router_run.name == "RouterRunnable" # Fixed from "RunnableSequence"
assert len(router_run.child_runs) == 1 # Now correctly structured
assert len(router_run.child_runs[0].child_runs) == 2Test assertions now match the correct expected behavior.
Why CI/CD Failed
Root Causes:
-
Python 3.14 Download Failure (lint check)
- External dependency on GitHub's python-build-standalone releases
- Server returned HTTP 500 error
- Not related to code changes
- Recommendation: Re-run the workflow when GitHub's servers are stable
-
CI Success Aggregation Issue
- The aggregation check may have strict requirements
- Could be related to timing or specific check dependencies
- Exit code 1 suggests a scripting/logic issue in the CI definition itself
- Recommendation: Investigate
.github/workflows/configuration
Code Correctness: Verified
The code logic is correct and well-implemented:
- Follows LangChain's callback patterns precisely
- Proper lifecycle management (start, error, end)
- Correct async/await handling in ainvoke
- Appropriate use of context managers
- Test updates align with new behavior
- No race conditions or resource leaks
- Proper exception handling and propagation
Recommendations
-
CI Failures: These are infrastructure issues. Recommend re-running the failing workflows or waiting for GitHub's infrastructure to stabilize.
-
Merge Readiness: From a code perspective, this PR is ready to merge once CI infrastructure issues are resolved.
-
Consider: Adding a docstring example showing how callbacks are now properly traced.
Conclusion
The implementation correctly addresses the reported issue. The RouterRunnable now properly:
- Appears in trace hierarchies
- Reports its name as "RouterRunnable" instead of "RunnableSequence"
- Integrates callbacks for child runnables
- Maintains proper trace structure
The CI failures are unrelated to code quality and are caused by external infrastructure problems. Once these are resolved (via re-run or automatic recovery), this PR should merge successfully.
aaron-seq
left a comment
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.
Follow-up Review
Excellent work addressing the previous feedback! All CI/CD checks passing and implementation looks solid.
Updates Verified
✅ Removed redundant self.name assignment
✅ Added set_config_context for proper config propagation
✅ All 88 checks passing
✅ Proper callback integration
Final Assessment
The code is production-ready and successfully addresses the RouterRunnable tracing visibility issue. Implementation follows LangChain patterns correctly.
Ready for merge once maintainers approve.
Fix RouterRunnable being invisible in traces and incorrectly reporting its name.
Problem
RouterRunnable overrode invoke and ainvoke without tracing callbacks, causing:
Solution
AI Disclosure: This contribution was developed with assistance from an AI coding assistant.