Track LangChain Runnable callbacks - #2100
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds chain callback handling for LangChain Runnable invocations, including run-name extraction, per-run input/name tracking, start/end event emission, and cleanup on end or error. Adds async tests for success and error paths. ChangesChain callback tracking
Estimated code review effort: 2 (Simple) | ~15 minutes Related issues: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
a551d13 to
c520adb
Compare
c520adb to
bf26bd5
Compare
|
Label Checker is failing because I cannot add labels from the fork. For this PR, I believe the appropriate labels are |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nvidia_nat_langchain/src/nat/plugins/langchain/callback_handler.py`:
- Around line 370-428: The chain event tracking maps are only populated in
on_chain_start and never cleaned up, causing per-run state to grow unbounded.
Update callback_handler.py in on_chain_end to remove the run_id_str entries from
_run_id_to_chain_input, _run_id_to_chain_name, and _run_id_to_start_time after
building the IntermediateStepPayload. If there is an error/abort path in the
LangChain callbacks (for example a chain error handler), apply the same cleanup
there using the same run_id_str keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dd5cb25f-e465-4114-8fa4-f8dfe961b09b
📒 Files selected for processing (2)
packages/nvidia_nat_langchain/src/nat/plugins/langchain/callback_handler.pypackages/nvidia_nat_langchain/tests/test_langchain_callback_handler.py
bf26bd5 to
b971b1b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/nvidia_nat_langchain/tests/test_langchain_callback_handler.py (2)
96-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring first line must end with a period.
The docstring's first line ends with a colon instead of a period, which will trip the Vale doc check.
As per coding guidelines, "The first line of docstrings must be a concise description ending with a period (Vale checks this)."
📝 Proposed fix
""" - Test that LangChain Runnable/chain callbacks produce paired function stats: + Test that LangChain Runnable/chain callbacks produce paired function stats. + - on_chain_start -> usage stat with event_type=FUNCTION_START - on_chain_end -> usage stat with event_type=FUNCTION_END """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nvidia_nat_langchain/tests/test_langchain_callback_handler.py` around lines 96 - 99, The docstring in the LangChain callback handler test currently ends the summary line with a colon instead of a period, which violates the doc style check. Update the first line of the docstring in test_langchain_callback_handler.py so the summary is a concise sentence ending with a period, keeping the rest of the test description unchanged.Source: Coding guidelines
105-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the default
tags/metadatapath.This test only exercises
on_chain_startwithtagsandmetadataexplicitly supplied. A follow-up test invoking with defaults (tags=None,metadata=None) would cover the branch that other tests, liketest_langchain_handler_clears_chain_state_on_error, only partially exercise foron_chain_start.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nvidia_nat_langchain/tests/test_langchain_callback_handler.py` around lines 105 - 140, Add a follow-up test for `LangChainCallbackHandler.on_chain_start` that omits both `tags` and `metadata` so the default path is covered. Use the existing `handler`/`all_stats` assertions pattern from this test, but call `on_chain_start` with `tags=None` and `metadata=None`, then verify the emitted start/end stats still populate correctly and that the handler state maps are cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/nvidia_nat_langchain/tests/test_langchain_callback_handler.py`:
- Around line 96-99: The docstring in the LangChain callback handler test
currently ends the summary line with a colon instead of a period, which violates
the doc style check. Update the first line of the docstring in
test_langchain_callback_handler.py so the summary is a concise sentence ending
with a period, keeping the rest of the test description unchanged.
- Around line 105-140: Add a follow-up test for
`LangChainCallbackHandler.on_chain_start` that omits both `tags` and `metadata`
so the default path is covered. Use the existing `handler`/`all_stats`
assertions pattern from this test, but call `on_chain_start` with `tags=None`
and `metadata=None`, then verify the emitted start/end stats still populate
correctly and that the handler state maps are cleared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cd942f18-7fae-43bd-9b71-183ab2ef286e
📒 Files selected for processing (2)
packages/nvidia_nat_langchain/src/nat/plugins/langchain/callback_handler.pypackages/nvidia_nat_langchain/tests/test_langchain_callback_handler.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nvidia_nat_langchain/src/nat/plugins/langchain/callback_handler.py
b971b1b to
7897eec
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/nvidia_nat_langchain/tests/test_langchain_callback_handler.py (1)
102-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract repeated test setup into a fixture.
The
all_stats = []/handler = LangchainProfilerHandler()/reactive_stream.subscribe(...)boilerplate is duplicated identically across all three tests.♻️ Suggested fixture extraction
+@pytest.fixture(name="chain_handler_fixture") +def chain_handler_fixture(reactive_stream: Subject): + all_stats = [] + handler = LangchainProfilerHandler() + _ = reactive_stream.subscribe(all_stats.append) + return handler, all_stats + + -async def test_langchain_handler_tracks_chain_runnable_events(reactive_stream: Subject): +async def test_langchain_handler_tracks_chain_runnable_events(chain_handler_fixture): ... - all_stats = [] - handler = LangchainProfilerHandler() - _ = reactive_stream.subscribe(all_stats.append) + handler, all_stats = chain_handler_fixtureAs per coding guidelines, "Extract frequently repeated test code into pytest fixtures with the
fixture_prefix or_fixturesuffix using snake_case, and define thenameargument in@pytest.fixture(name="...")".Also applies to: 147-149, 183-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nvidia_nat_langchain/tests/test_langchain_callback_handler.py` around lines 102 - 104, The test setup that creates all_stats, instantiates LangchainProfilerHandler, and subscribes to reactive_stream is repeated across multiple test cases in test_langchain_callback_handler.py. Extract that boilerplate into a pytest fixture using the required fixture naming convention (fixture_ prefix or _fixture suffix in snake_case) and register it with `@pytest.fixture`(name="..."), then update the tests to use the shared fixture instead of repeating the setup in each test.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/nvidia_nat_langchain/tests/test_langchain_callback_handler.py`:
- Around line 102-104: The test setup that creates all_stats, instantiates
LangchainProfilerHandler, and subscribes to reactive_stream is repeated across
multiple test cases in test_langchain_callback_handler.py. Extract that
boilerplate into a pytest fixture using the required fixture naming convention
(fixture_ prefix or _fixture suffix in snake_case) and register it with
`@pytest.fixture`(name="..."), then update the tests to use the shared fixture
instead of repeating the setup in each test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e61393fb-f6db-4d21-ac2e-e3ad3c62ef95
📒 Files selected for processing (2)
packages/nvidia_nat_langchain/src/nat/plugins/langchain/callback_handler.pypackages/nvidia_nat_langchain/tests/test_langchain_callback_handler.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nvidia_nat_langchain/src/nat/plugins/langchain/callback_handler.py
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
7897eec to
97a44de
Compare
willkill07
added
feature request
|
/ok to test 97a44de |
|
/merge |
2 participants
Summary
on_chain_start/on_chain_endhandling inLangchainProfilerHandlerFUNCTION_START/FUNCTION_ENDintermediate steps for base Runnable/chain invocationsCloses #1787
Validation
PYTHONUTF8=1 uv run --extra langchain --extra test --group dev pytest packages/nvidia_nat_langchain/tests/test_langchain_callback_handler.py -q(11 passed)PYTHONUTF8=1 uv run --extra langchain --extra test --group dev pytest packages/nvidia_nat_langchain/tests -q(684 passed, 13 skipped)PYTHONUTF8=1 uv run --extra langchain --extra test --group dev yapf --diff --style ./pyproject.toml packages/nvidia_nat_langchain/src/nat/plugins/langchain/callback_handler.py packages/nvidia_nat_langchain/tests/test_langchain_callback_handler.pyPYTHONUTF8=1 uv run --extra langchain --extra test --group dev ruff check packages/nvidia_nat_langchain/src/nat/plugins/langchain/callback_handler.py packages/nvidia_nat_langchain/tests/test_langchain_callback_handler.pyPYTHONUTF8=1 uv run --extra langchain --extra test --group dev pre-commit run --files packages/nvidia_nat_langchain/src/nat/plugins/langchain/callback_handler.py packages/nvidia_nat_langchain/tests/test_langchain_callback_handler.pygit diff --check origin/develop..HEADSummary by CodeRabbit
New Features
Bug Fixes
Tests