Observability user experiences fixes#1760
Observability user experiences fixes#1760rapids-bot[bot] merged 12 commits intoNVIDIA:release/1.5from
Conversation
Introduced the `validation_alias` using `AliasChoices` to support alternative field names for `nvext_max_sensitivity`. This enhances flexibility in handling input data, allowing the use of both "nvext_max_sensitivity" and "max_sensitivity" as valid keys. Signed-off-by: dnandakumar-nv <dnandakumar@nvidia.com>
This update introduces `AliasChoices` to support multiple aliases for the `nvext_prediction_trie_path` field. It improves flexibility by allowing alternative names like `prediction_trie_path` for easier configuration. Signed-off-by: dnandakumar-nv <dnandakumar@nvidia.com>
The configuration now activates nvext hints by setting `enable_nvext_hints` to true, improving integration with Dynamo nvext features. Additionally, the prediction trie path key was updated to `nvext_prediction_trie_path` for improved clarity and consistency. Signed-off-by: dnandakumar-nv <dnandakumar@nvidia.com>
… latency-example-fix
Handle race conditions during export task creation, closing coroutines and logging warnings instead of propagating RuntimeErrors. Enhance test coverage for handling shutdown scenarios. Additionally, update documentation to clarify LangChain dependencies, provide usage of `--override` flag for configuration, and correct environment variable naming for LangFuse. Signed-off-by: dnandakumar-nv <dnandakumar@nvidia.com>
WalkthroughRenames LangSmith references to LangChain in docs, adds CLI --override examples and evaluation prerequisites, corrects a Langfuse env var in an example README, and updates ProcessingExporter to close coroutines when not running and log (not raise) on RuntimeError from task creation; tests updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ok to test 5464fdd |
mnajafian-nv
left a comment
There was a problem hiding this comment.
LGTM! Conditional approval upon addressing the inline NITs
packages/nvidia_nat_core/tests/nat/observability/exporter/test_processing_exporter.py
Outdated
Show resolved
Hide resolved
examples/observability/simple_calculator_observability/README.md
Outdated
Show resolved
Hide resolved
…_processing_exporter.py Co-authored-by: mnajafian-nv <mnajafian@nvidia.com> Signed-off-by: Dhruv Nandakumar <168006707+dnandakumar-nv@users.noreply.github.com>
Co-authored-by: mnajafian-nv <mnajafian@nvidia.com> Signed-off-by: Dhruv Nandakumar <168006707+dnandakumar-nv@users.noreply.github.com>
|
/ok to test 29196a3 |
Cleaned up extraneous blank lines to improve code readability and maintain consistency. This change does not affect functionality or behavior but enhances clarity in the test file. Signed-off-by: dnandakumar-nv <dnandakumar@nvidia.com>
|
/ok to test a40a28e |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/nvidia_nat_core/tests/nat/observability/exporter/test_processing_exporter.py`:
- Around line 1113-1123: Replace the Mock() coroutine placeholders in the
failing tests with a real coroutine object (e.g. create a simple async def
_dummy(): await asyncio.sleep(0) and instantiate it with _dummy()) before
passing it to processing_exporter._create_export_task; keep the
asyncio.create_task patch that raises RuntimeError so the internal exception
path is exercised, then assert the coroutine was properly closed by checking
cr.cr_frame is None (instead of only verifying .close() was called). Apply this
change to both test cases that call _create_export_task (the shutdown case and
the "not running" case) so the test verifies actual coroutine cleanup rather
than just a Mock.close call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11bb6114-d879-43aa-b832-d55c774f4a50
📒 Files selected for processing (1)
packages/nvidia_nat_core/tests/nat/observability/exporter/test_processing_exporter.py
packages/nvidia_nat_core/tests/nat/observability/exporter/test_processing_exporter.py
Show resolved
Hide resolved
|
/merge |
Description
Improve export task handling and update LangSmith docs
Handle race conditions during export task creation, closing coroutines and logging warnings instead of propagating RuntimeErrors. Enhance test coverage for handling shutdown scenarios.
Additionally, update documentation to clarify LangChain dependencies, provide usage of
--overrideflag for configuration, and correct environment variable naming for LangFuse.By Submitting this PR I confirm:
Summary by CodeRabbit
Documentation
Bug Fixes
Tests