Add weave feedback integration for chat interactions #781
Conversation
WalkthroughPropagates a root Weave call ID into runtime Context as Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI
participant WeaveExporter
participant Context
participant Weave
Client->>FastAPI: POST /generate (start workflow)
FastAPI->>WeaveExporter: create_weave_call(...)
WeaveExporter->>Weave: create/fetch call
Weave-->>WeaveExporter: call (id)
WeaveExporter->>Context: set observability_trace_id if unset
FastAPI-->>Client: response (+ Observability-Trace-Id header / streamed SSE messages)
Note over Client: Later, user submits feedback
Client->>FastAPI: POST /feedback {observability_trace_id, reaction_type}
FastAPI->>Weave: fetch call by observability_trace_id
FastAPI->>Weave: record reaction
Weave-->>FastAPI: success/failure
FastAPI-->>Client: 200 / 4xx / 5xx
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thepatrickchin
force-pushed
the
weave-feedback
branch
2 times, most recently
from
a62e5ff to
2e64134
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/nat/front_ends/fastapi/intermediate_steps_subscriber.py (1)
47-58: Avoid swallowing unexpected context errors.
Context.weave_call_idalready returnsNonewhen unset, so the blanketexcept Exception: passonly hides real bugs. Please drop thetry/exceptor catch a specific, expected exception.- if adapter is None: - weave_call_id = None - try: - weave_call_id = context.weave_call_id - except Exception: - pass + if adapter is None: + weave_call_id = context.weave_call_idsrc/nat/front_ends/fastapi/step_adaptor.py (1)
42-48: Narrow the exception handling when fetching weave_call_id.Importing
Contextmight fail if the module isn’t available, but other failures (e.g., attribute errors) should surface. Catch onlyImportError(and optionallyAttributeError) instead of swallowing every exception.- def _get_weave_call_id(self) -> str | None: - """Retrieve weave_call_id from context if available.""" - try: - from nat.builder.context import Context - return Context.get().weave_call_id - except Exception: - return None + def _get_weave_call_id(self) -> str | None: + """Retrieve weave_call_id from context if available.""" + try: + from nat.builder.context import Context + except ImportError: + return None + return getattr(Context.get(), "weave_call_id", None)src/nat/front_ends/fastapi/message_validator.py (1)
247-259: Update docstrings to document the new parameter.The docstrings for all three methods should document the new
weave_call_idparameter.Add parameter documentation to each docstring:
For
create_system_response_token_message(after line 257)::param content: Message content. :param status: Status of the message (default: IN_PROGRESS). :param timestamp: Timestamp of the message (default: current UTC time). + :param weave_call_id: Weave call identifier for feedback integration (default: retrieved from context). :return: A WebSocketSystemResponseTokenMessage instance.Apply similar changes to
create_system_intermediate_step_messageandcreate_system_interaction_message.Also applies to: 295-307, 344-356
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/source/reference/api-server-endpoints.md(1 hunks)docs/source/workflows/observe/index.md(1 hunks)docs/source/workflows/observe/observe-workflow-with-weave.md(1 hunks)packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py(1 hunks)src/nat/builder/context.py(2 hunks)src/nat/data_models/api_server.py(14 hunks)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py(7 hunks)src/nat/front_ends/fastapi/intermediate_steps_subscriber.py(1 hunks)src/nat/front_ends/fastapi/message_handler.py(1 hunks)src/nat/front_ends/fastapi/message_validator.py(4 hunks)src/nat/front_ends/fastapi/step_adaptor.py(6 hunks)tests/nat/front_ends/fastapi/test_openai_compatibility.py(3 hunks)tests/nat/server/test_unified_api_server.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
{docs/source/**/*.md,**/README.@(md|ipynb)}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{docs/source/**/*.md,**/README.@(md|ipynb)}: Use the full name “NVIDIA NeMo Agent toolkit” on first use in documentation, then “NeMo Agent toolkit”; in headings use “NeMo Agent Toolkit” (capital T)
Do not use deprecated names (Agent Intelligence toolkit, aiqtoolkit, AgentIQ, AIQ/aiq) in documentation unless explicitly referring to deprecated names
Never use “NAT”/“nat” abbreviations in documentation
Documentation must be clear/comprehensive; avoid TODOs/FIXMEs/placeholders; avoid offensive/outdated terms; ensure spelling is correct
Files:
docs/source/workflows/observe/observe-workflow-with-weave.mddocs/source/reference/api-server-endpoints.mddocs/source/workflows/observe/index.md
docs/source/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.md: Documentation sources are Markdown files under docs/source
Surround code entities with backticks in docs to avoid Vale false positives
Do not use words listed in ci/vale/styles/config/vocabularies/nat/reject.txt; accepted words in accept.txt are allowed
Files:
docs/source/workflows/observe/observe-workflow-with-weave.mddocs/source/reference/api-server-endpoints.mddocs/source/workflows/observe/index.md
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}: Every file must start with the standard SPDX Apache-2.0 header
Confirm copyright years are up to date when a file is changed
All source files must include the SPDX Apache-2.0 header template (copy from an existing file)
Files:
docs/source/workflows/observe/observe-workflow-with-weave.mddocs/source/reference/api-server-endpoints.mdsrc/nat/builder/context.pytests/nat/front_ends/fastapi/test_openai_compatibility.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/step_adaptor.pysrc/nat/front_ends/fastapi/message_validator.pydocs/source/workflows/observe/index.mdsrc/nat/front_ends/fastapi/message_handler.pysrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pytests/nat/server/test_unified_api_server.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
docs/source/workflows/observe/observe-workflow-with-weave.mddocs/source/reference/api-server-endpoints.mdsrc/nat/builder/context.pytests/nat/front_ends/fastapi/test_openai_compatibility.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/step_adaptor.pysrc/nat/front_ends/fastapi/message_validator.pydocs/source/workflows/observe/index.mdsrc/nat/front_ends/fastapi/message_handler.pysrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pytests/nat/server/test_unified_api_server.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.py
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/workflows/observe/observe-workflow-with-weave.mddocs/source/reference/api-server-endpoints.mddocs/source/workflows/observe/index.md
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: In code comments use the abbreviations: nat (API namespace/CLI), nvidia-nat (package), NAT (env var prefixes); never use these abbreviations in documentation
Follow PEP 20 and PEP 8 for Python style
Run yapf with column_limit=120; yapf is used for formatting (run second)
Indent with 4 spaces (no tabs) and end each file with a single trailing newline
Use ruff (ruff check --fix) as a linter (not formatter) per pyproject.toml; fix warnings unless explicitly ignored
Respect Python naming schemes: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
When re-raising exceptions, use bare raise to preserve stack trace; log with logger.error(), not logger.exception()
When catching and logging without re-raising, use logger.exception() to capture full stack trace
Provide Google-style docstrings for every public module, class, function, and CLI command
Docstring first line must be a concise description ending with a period
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file I/O)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial and feasible
Files:
src/nat/builder/context.pytests/nat/front_ends/fastapi/test_openai_compatibility.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/step_adaptor.pysrc/nat/front_ends/fastapi/message_validator.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pytests/nat/server/test_unified_api_server.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All importable Python code must live under src/ or packages//src/
All public APIs must have Python 3.11+ type hints on parameters and return values
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or metadata when useful
Treat pyright warnings as errors during development
Files:
src/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/step_adaptor.pysrc/nat/front_ends/fastapi/message_validator.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/nat/**/* contains core functionality; changes should prioritize backward compatibility
Files:
src/nat/builder/context.pysrc/nat/front_ends/fastapi/step_adaptor.pysrc/nat/front_ends/fastapi/message_validator.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/builder/context.pysrc/nat/front_ends/fastapi/step_adaptor.pysrc/nat/front_ends/fastapi/message_validator.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.py
{tests/**/*.py,examples/*/tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Unit tests live in tests/ or examples/*/tests and use markers defined in pyproject.toml (e.g., integration)
Files:
tests/nat/front_ends/fastapi/test_openai_compatibility.pytests/nat/server/test_unified_api_server.py
{tests/**/*.py,examples/*/tests/**/*.py,packages/*/tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{tests/**/*.py,examples/*/tests/**/*.py,packages/*/tests/**/*.py}: Use pytest with pytest-asyncio for async code
Test functions should be named with test_ prefix using snake_case
Extract frequently repeated test code into pytest fixtures
Pytest fixtures should define the name argument in @pytest.fixture and the function should be prefixed fixture_ using snake_case
Mock external services with pytest_httpserver or unittest.mock; avoid live endpoints
Mark slow tests with @pytest.mark.slow to skip in default test suite
Mark integration tests requiring external services with @pytest.mark.integration and follow nat-tests guidelines
Files:
tests/nat/front_ends/fastapi/test_openai_compatibility.pytests/nat/server/test_unified_api_server.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/front_ends/fastapi/test_openai_compatibility.pytests/nat/server/test_unified_api_server.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
If a package contains Python code, it must have tests in a tests/ directory at the same level as pyproject.toml
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
🧠 Learnings (2)
📚 Learning: 2025-10-22T22:02:12.903Z
Learnt from: CR
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-10-22T22:02:12.903Z
Learning: Applies to {tests/**/*.py,examples/*/tests/**/*.py,packages/*/tests/**/*.py} : Mock external services with pytest_httpserver or unittest.mock; avoid live endpoints
Applied to files:
tests/nat/front_ends/fastapi/test_openai_compatibility.py
📚 Learning: 2025-10-22T22:02:12.903Z
Learnt from: CR
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-10-22T22:02:12.903Z
Learning: Applies to CHANGELOG.md : Add user-visible changes to CHANGELOG.md under the appropriate section
Applied to files:
docs/source/workflows/observe/index.md
🧬 Code graph analysis (9)
tests/nat/front_ends/fastapi/test_openai_compatibility.py (2)
src/nat/data_models/api_server.py (8)
ChatResponseChunk(400-505)create_streaming_chunk(462-505)UserMessageContentRoleType(41-47)from_string(170-181)from_string(362-397)from_string(425-459)Usage(309-312)ChatResponse(338-397)src/nat/builder/context.py (1)
weave_call_id(221-225)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (1)
src/nat/builder/context.py (1)
weave_call_id(221-225)
src/nat/front_ends/fastapi/step_adaptor.py (1)
src/nat/builder/context.py (2)
Context(122-341)weave_call_id(221-225)
src/nat/front_ends/fastapi/message_validator.py (2)
src/nat/data_models/api_server.py (8)
WebSocketMessageType(546-555)SystemResponseContent(676-679)Error(603-608)WebSocketMessageStatus(568-573)WebSocketSystemResponseTokenMessage(682-710)SystemIntermediateStepContent(649-652)WebSocketSystemIntermediateStepMessage(655-673)WebSocketSystemInteractionMessage(713-730)src/nat/builder/context.py (5)
conversation_id(190-197)weave_call_id(221-225)Context(122-341)get(118-119)get(331-341)
src/nat/front_ends/fastapi/message_handler.py (3)
src/nat/builder/context.py (2)
weave_call_id(221-225)conversation_id(190-197)src/nat/data_models/api_server.py (3)
WebSocketSystemResponseTokenMessage(682-710)WebSocketSystemIntermediateStepMessage(655-673)WebSocketSystemInteractionMessage(713-730)src/nat/front_ends/fastapi/message_validator.py (4)
create_system_response_token_message(235-281)create_system_intermediate_step_message(283-329)get_intermediate_step_parent_id(226-233)create_system_interaction_message(331-378)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (3)
src/nat/builder/context.py (4)
Context(122-341)weave_call_id(221-225)get(118-119)get(331-341)src/nat/data_models/api_server.py (1)
ChatResponse(338-397)src/nat/builder/workflow_builder.py (1)
WorkflowBuilder(154-1271)
tests/nat/server/test_unified_api_server.py (3)
src/nat/data_models/api_server.py (9)
UserMessageContentRoleType(41-47)WebSocketMessageStatus(568-573)ChatResponse(338-397)ChatResponseChunk(400-505)ResponseIntermediateStep(508-521)from_string(170-181)from_string(362-397)from_string(425-459)create_streaming_chunk(462-505)src/nat/builder/context.py (1)
weave_call_id(221-225)src/nat/front_ends/fastapi/message_validator.py (3)
MessageValidator(61-378)create_system_response_token_message(235-281)create_system_intermediate_step_message(283-329)
src/nat/data_models/api_server.py (1)
src/nat/builder/context.py (4)
weave_call_id(221-225)Context(122-341)get(118-119)get(331-341)
src/nat/front_ends/fastapi/intermediate_steps_subscriber.py (2)
src/nat/builder/context.py (1)
weave_call_id(221-225)src/nat/data_models/api_server.py (1)
ResponseIntermediateStep(508-521)
🪛 LanguageTool
docs/source/workflows/observe/observe-workflow-with-weave.md
[grammar] ~138-~138: Use a hyphen to join words.
Context: ...tomatically available, along with thumbs up and thumbs down buttons in the NeMo A...
(QB_NEW_EN_HYPHEN)
[grammar] ~138-~138: Use a hyphen to join words.
Context: ...ailable, along with thumbs up and thumbs down buttons in the NeMo Agent Toolkit U...
(QB_NEW_EN_HYPHEN)
🪛 Ruff (0.14.4)
src/nat/front_ends/fastapi/step_adaptor.py
47-47: Do not catch blind exception: Exception
(BLE001)
src/nat/front_ends/fastapi/message_validator.py
243-243: Do not perform function call SystemResponseContent in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
266-267: try-except-pass detected, consider logging the exception
(S110)
266-266: Do not catch blind exception: Exception
(BLE001)
280-280: Redundant exception object included in logging.exception call
(TRY401)
291-291: Do not perform function call SystemIntermediateStepContent in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
314-315: try-except-pass detected, consider logging the exception
(S110)
314-314: Do not catch blind exception: Exception
(BLE001)
328-328: Redundant exception object included in logging.exception call
(TRY401)
363-364: try-except-pass detected, consider logging the exception
(S110)
363-363: Do not catch blind exception: Exception
(BLE001)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
1095-1095: Loop control variable name not used within loop body
Rename unused name to _name
(B007)
1106-1106: Unused function argument: request
(ARG001)
1112-1112: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
1120-1120: Abstract raise to an inner function
(TRY301)
1150-1150: Do not catch blind exception: Exception
(BLE001)
src/nat/data_models/api_server.py
383-384: try-except-pass detected, consider logging the exception
(S110)
383-383: Do not catch blind exception: Exception
(BLE001)
445-446: try-except-pass detected, consider logging the exception
(S110)
445-445: Do not catch blind exception: Exception
(BLE001)
483-484: try-except-pass detected, consider logging the exception
(S110)
483-483: Do not catch blind exception: Exception
(BLE001)
src/nat/front_ends/fastapi/intermediate_steps_subscriber.py
50-51: try-except-pass detected, consider logging the exception
(S110)
50-50: Do not catch blind exception: Exception
(BLE001)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
Outdated
Show resolved
Hide resolved
willkill07
left a comment
willkill07
left a comment
There was a problem hiding this comment.
In general, we need to be mindful of scope staying within plugins/subpackages.
Terms can be generalized to avoid weave directly in an agnostic manner.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (1)
29-35: Tighten NAT_CONFIG_FILE test fixture behavior.For the autouse
setup_envfixture, consider storing and restoring the originalNAT_CONFIG_FILEand using a safer temp path instead of hard‑coding/tmp/dummy_nat_config.ymlto avoid cross‑test leakage and S108 warnings:-@pytest.fixture(autouse=True) -def setup_env(): - """Set up environment variables for tests.""" - # Set a dummy config file path for tests that don't use Dask - if "NAT_CONFIG_FILE" not in os.environ: - os.environ["NAT_CONFIG_FILE"] = "/tmp/dummy_nat_config.yml" - yield +@pytest.fixture(autouse=True) +def setup_env(): + """Set up environment variables for tests.""" + original = os.environ.get("NAT_CONFIG_FILE") + if original is None: + # Dummy value for tests that don't use Dask; no file access required + os.environ["NAT_CONFIG_FILE"] = "dummy_nat_config.yml" + try: + yield + finally: + if original is None: + os.environ.pop("NAT_CONFIG_FILE", None) + else: + os.environ["NAT_CONFIG_FILE"] = originalsrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (1)
548-553: Header-based observability trace propagation looks correct; consider light hardening.Injecting
Observability-Trace-Idviaadd_context_headers_to_responseon non‑streaming responses (including OpenAI‑compatible POST) cleanly exposes the trace ID to clients without touching existing payloads, which matches the intended behavior.If you expect any code paths where
Context.get()might not be initialized, you could optionally guard the helper to avoid a 500 from header injection:def add_context_headers_to_response(response: Response) -> None: try: observability_trace_id = Context.get().observability_trace_id except Exception: # fall back silently if context is unavailable observability_trace_id = None if observability_trace_id: response.headers["Observability-Trace-Id"] = observability_trace_idAlso applies to: 597-607, 652-655, 725-727
src/nat/front_ends/fastapi/step_adaptor.py (1)
42-46: Intermediate steps now carry observability_trace_id; optional robustness tweak.Using
_get_observability_trace_id()to populateobservability_trace_idon allResponseIntermediateStepinstances nicely ties LLM/tool/function/custom steps back to the root trace.To avoid losing intermediate steps if
Context.get()ever fails, you could wrap_get_observability_trace_idin a small try/except and returnNoneon error, letting the existingprocesslogic proceed without a trace ID instead of logging an exception and dropping the step.Also applies to: 121-126, 177-182, 213-218, 267-272, 294-299
src/nat/data_models/api_server.py (1)
379-384: Narrow exception handling and consider extracting to helperThe bare
Exceptioncatch could mask unexpected errors. The context-retrieval logic is also duplicated across three methods (lines 379-384, 441-446, 479-484).Consider these improvements:
- Use specific exceptions to avoid masking unrelated errors:
if observability_trace_id is None: try: from nat.builder.context import Context observability_trace_id = Context.get().observability_trace_id - except Exception: + except (ImportError, AttributeError, KeyError): pass
- Extract to a helper method (similar to
src/nat/front_ends/fastapi/message_validator.pylines 75-89) to reduce duplication and centralize the fallback logic:@staticmethod def _get_observability_trace_id_from_context() -> str | None: """Retrieve observability_trace_id from context if available.""" try: from nat.builder.context import Context return Context.get().observability_trace_id except (ImportError, AttributeError, KeyError): return NoneThen replace all three occurrences with:
if observability_trace_id is None: - try: - from nat.builder.context import Context - observability_trace_id = Context.get().observability_trace_id - except Exception: - pass + observability_trace_id = cls._get_observability_trace_id_from_context()This approach provides better error visibility and maintainability.
src/nat/front_ends/fastapi/message_validator.py (3)
259-259: Address mutable default and redundant exception loggingTwo minor issues flagged by static analysis:
Mutable default argument (line 259): While Pydantic models are generally safe, best practice is to use
Noneas the default and construct the object inside the function.Redundant exception in logging (line 291):
logger.exception()automatically includes exception details, makingstr(e)redundant.Apply these improvements:
async def create_system_response_token_message( self, message_type: Literal[WebSocketMessageType.RESPONSE_MESSAGE, WebSocketMessageType.ERROR_MESSAGE] = WebSocketMessageType.RESPONSE_MESSAGE, message_id: str | None = str(uuid.uuid4()), thread_id: str = "default", parent_id: str = "default", conversation_id: str | None = None, - content: SystemResponseContent | Error = SystemResponseContent(), + content: SystemResponseContent | Error | None = None, status: WebSocketMessageStatus = WebSocketMessageStatus.IN_PROGRESS, timestamp: str = str(datetime.datetime.now(datetime.UTC)), observability_trace_id: str | None = None) -> WebSocketSystemResponseTokenMessage | None: """ Creates a system response token message with default values. ... """ try: observability_trace_id = self._get_observability_trace_id_from_context(observability_trace_id) + if content is None: + content = SystemResponseContent() return WebSocketSystemResponseTokenMessage(type=message_type, id=message_id, thread_id=thread_id, parent_id=parent_id, conversation_id=conversation_id, content=content, status=status, timestamp=timestamp, observability_trace_id=observability_trace_id) except Exception as e: - logger.exception("Error creating system response token message: %s", str(e)) + logger.exception("Error creating system response token message") return NoneAlso applies to: 291-291
302-302: Apply same fixes for mutable default and loggingSimilar to
create_system_response_token_message, this method has the same two issues:
- Mutable default (line 302):
SystemIntermediateStepContent(name="default", payload="default")- Redundant exception logging (line 333)
Apply the same pattern:
async def create_system_intermediate_step_message( self, message_type: Literal[WebSocketMessageType.INTERMEDIATE_STEP_MESSAGE] = ( WebSocketMessageType.INTERMEDIATE_STEP_MESSAGE), message_id: str = str(uuid.uuid4()), thread_id: str = "default", parent_id: str = "default", conversation_id: str | None = None, - content: SystemIntermediateStepContent = SystemIntermediateStepContent(name="default", payload="default"), + content: SystemIntermediateStepContent | None = None, status: WebSocketMessageStatus = WebSocketMessageStatus.IN_PROGRESS, timestamp: str = str(datetime.datetime.now(datetime.UTC)), observability_trace_id: str | None = None) -> WebSocketSystemIntermediateStepMessage | None: """ Creates a system intermediate step message with default values. ... """ try: observability_trace_id = self._get_observability_trace_id_from_context(observability_trace_id) + if content is None: + content = SystemIntermediateStepContent(name="default", payload="default") return WebSocketSystemIntermediateStepMessage(type=message_type, id=message_id, thread_id=thread_id, parent_id=parent_id, conversation_id=conversation_id, content=content, status=status, timestamp=timestamp, observability_trace_id=observability_trace_id) except Exception as e: - logger.exception("Error creating system intermediate step message: %s", str(e)) + logger.exception("Error creating system intermediate step message") return NoneAlso applies to: 333-333
376-376: Remove redundant exception info from loggingSame logging issue:
logger.exception()already captures exception details.except Exception as e: - logger.exception("Error creating system interaction message: %s", str(e)) + logger.exception("Error creating system interaction message") return None
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/source/reference/api-server-endpoints.md(1 hunks)docs/source/workflows/observe/observe-workflow-with-weave.md(1 hunks)examples/observability/simple_calculator_observability/configs/config-weave.yml(1 hunks)packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py(1 hunks)packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py(1 hunks)packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py(1 hunks)src/nat/builder/context.py(2 hunks)src/nat/data_models/api_server.py(14 hunks)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py(5 hunks)src/nat/front_ends/fastapi/intermediate_steps_subscriber.py(1 hunks)src/nat/front_ends/fastapi/message_handler.py(1 hunks)src/nat/front_ends/fastapi/message_validator.py(5 hunks)src/nat/front_ends/fastapi/step_adaptor.py(6 hunks)tests/nat/front_ends/fastapi/test_openai_compatibility.py(3 hunks)tests/nat/server/test_unified_api_server.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/nat/front_ends/fastapi/intermediate_steps_subscriber.py
- src/nat/builder/context.py
- tests/nat/front_ends/fastapi/test_openai_compatibility.py
- packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
- src/nat/front_ends/fastapi/message_handler.py
🧰 Additional context used
📓 Path-based instructions (6)
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
examples/observability/simple_calculator_observability/configs/config-weave.ymldocs/source/workflows/observe/observe-workflow-with-weave.mddocs/source/reference/api-server-endpoints.mdtests/nat/server/test_unified_api_server.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pysrc/nat/front_ends/fastapi/step_adaptor.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pysrc/nat/front_ends/fastapi/message_validator.py
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should
contain a README.md or file README.ipynb.
- If an example contains Python code, it should be placed in a subdirectory named
src/and should
contain apyproject.tomlfile. Optionally, it might also contain scripts in ascripts/directory.- If an example contains YAML files, they should be placed in a subdirectory named
configs/. - If an example contains sample data files, they should be placed in a subdirectory nameddata/, and should
be checked into git-lfs.
Files:
examples/observability/simple_calculator_observability/configs/config-weave.yml
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/workflows/observe/observe-workflow-with-weave.mddocs/source/reference/api-server-endpoints.md
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/server/test_unified_api_server.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
src/nat/**/*
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pysrc/nat/front_ends/fastapi/step_adaptor.pysrc/nat/front_ends/fastapi/message_validator.py
🧬 Code graph analysis (7)
tests/nat/server/test_unified_api_server.py (3)
src/nat/data_models/api_server.py (16)
UserMessageContentRoleType(41-47)WebSocketMessageStatus(568-573)ChatResponse(338-397)ChatResponseChunk(400-505)ChatResponseChoice(295-297)ChoiceMessage(277-279)Usage(309-312)ChatResponseChunkChoice(300-302)ChoiceDelta(282-285)ResponseIntermediateStep(508-521)from_string(170-181)from_string(362-397)from_string(425-459)create_streaming_chunk(462-505)SystemResponseContent(676-679)SystemIntermediateStepContent(649-652)src/nat/builder/context.py (1)
observability_trace_id(221-225)src/nat/front_ends/fastapi/message_validator.py (3)
MessageValidator(61-377)create_system_response_token_message(251-292)create_system_intermediate_step_message(294-334)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (6)
src/nat/data_models/config.py (1)
GeneralConfig(191-243)src/nat/front_ends/fastapi/fastapi_front_end_config.py (1)
FastApiFrontEndConfig(136-271)packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (1)
WeaveFastAPIPluginWorker(29-120)packages/nvidia_nat_weave/src/nat/plugins/weave/register.py (1)
WeaveTelemetryExporter(28-46)packages/nvidia_nat_test/src/nat/test/functions.py (1)
EchoFunctionConfig(28-29)packages/nvidia_nat_test/src/nat/test/utils.py (1)
build_nat_client(93-123)
src/nat/data_models/api_server.py (1)
src/nat/builder/context.py (4)
observability_trace_id(221-225)Context(122-341)get(118-119)get(331-341)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (3)
src/nat/builder/context.py (4)
Context(122-341)observability_trace_id(221-225)get(118-119)get(331-341)src/nat/front_ends/fastapi/response_helpers.py (1)
generate_single_response(108-117)src/nat/data_models/api_server.py (1)
ChatResponse(338-397)
src/nat/front_ends/fastapi/step_adaptor.py (1)
src/nat/builder/context.py (4)
Context(122-341)get(118-119)get(331-341)observability_trace_id(221-225)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (3)
src/nat/builder/workflow_builder.py (1)
WorkflowBuilder(154-1271)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (3)
FastApiFrontEndPluginWorker(223-1275)add_routes(242-254)config(123-124)src/nat/utils/type_utils.py (1)
override(56-57)
src/nat/front_ends/fastapi/message_validator.py (2)
src/nat/builder/context.py (5)
observability_trace_id(221-225)Context(122-341)get(118-119)get(331-341)conversation_id(190-197)src/nat/data_models/api_server.py (8)
WebSocketMessageType(546-555)SystemResponseContent(676-679)Error(603-608)WebSocketMessageStatus(568-573)WebSocketSystemResponseTokenMessage(682-710)SystemIntermediateStepContent(649-652)WebSocketSystemIntermediateStepMessage(655-673)WebSocketSystemInteractionMessage(713-730)
🪛 Ruff (0.14.4)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
34-34: Probable insecure usage of temporary file or directory: "/tmp/dummy_nat_config.yml"
(S108)
src/nat/data_models/api_server.py
383-384: try-except-pass detected, consider logging the exception
(S110)
383-383: Do not catch blind exception: Exception
(BLE001)
445-446: try-except-pass detected, consider logging the exception
(S110)
445-445: Do not catch blind exception: Exception
(BLE001)
483-484: try-except-pass detected, consider logging the exception
(S110)
483-483: Do not catch blind exception: Exception
(BLE001)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
60-60: Loop control variable name not used within loop body
Rename unused name to _name
(B007)
72-72: Unused function argument: request
(ARG001)
78-78: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
86-86: Abstract raise to an inner function
(TRY301)
93-93: Consider moving this statement to an else block
(TRY300)
95-95: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
96-96: Use explicit conversion flag
Replace with conversion flag
(RUF010)
119-119: Do not catch blind exception: Exception
(BLE001)
src/nat/front_ends/fastapi/message_validator.py
259-259: Do not perform function call SystemResponseContent in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
291-291: Redundant exception object included in logging.exception call
(TRY401)
302-302: Do not perform function call SystemIntermediateStepContent in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
333-333: Redundant exception object included in logging.exception call
(TRY401)
🔇 Additional comments (8)
examples/observability/simple_calculator_observability/configs/config-weave.yml (1)
18-20: Example config correctly wires the Weave FastAPI plugin worker.The
front_endblock cleanly enables theWeaveFastAPIPluginWorkeralongside existing weave telemetry and matches how the worker is expected to be configured.docs/source/workflows/observe/observe-workflow-with-weave.md (1)
136-156: Feedback integration docs are clear and consistent.The new “User Feedback Integration” section accurately describes how to enable
/feedbackwithWeaveFastAPIPluginWorker, includes a correct config example, and cleanly cross‑references the API Server Endpoints docs.docs/source/reference/api-server-endpoints.md (1)
539-558: Feedback endpoint documentation aligns with implementation.The
/feedbacksection correctly documents route, payload fields (observability_trace_id,reaction_type), and the success response, and clearly notes the dependency on the Weave FastAPI plugin worker.packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (1)
38-114: Good coverage of Weave feedback route registration and behavior.The tests cleanly exercise the presence/absence of
/feedbackbased on weave telemetry, parameter validation (400s), and preservation of standard/generatebehavior when usingWeaveFastAPIPluginWorker.tests/nat/server/test_unified_api_server.py (1)
365-407: Observability trace ID propagation tests are comprehensive.The new tests cover:
- HTTP chat and chat_stream endpoints picking up
observability_trace_idfromContext.get().- Direct ChatResponse/ChatResponseChunk construction (explicit vs. context‑derived trace IDs).
- WebSocket message creation and serialization, including observability and the data‑model dump behavior.
This gives good confidence that end‑to‑end propagation behaves as intended across HTTP, streaming, and WebSocket paths.
Also applies to: 523-538, 671-716, 718-745, 770-783
src/nat/data_models/api_server.py (2)
354-354: LGTM - Clean field additionThe
observability_trace_idfield is properly added as an optional field, maintaining backward compatibility.
417-417: LGTM - Consistent field additions across modelsThe
observability_trace_idfield is consistently added to all response and WebSocket message models with proper typing and backward compatibility.Also applies to: 521-521, 673-673, 698-698, 730-730
src/nat/front_ends/fastapi/message_validator.py (1)
75-89: Excellent implementation addressing past feedbackThis helper method effectively centralizes the observability trace ID retrieval logic with specific exception handling, directly addressing the previous review comment's suggestion to extract and refine the duplicated context-retrieval code.
Based on learnings
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
Outdated
Show resolved
Hide resolved
willkill07
added
feature request
thepatrickchin
force-pushed
the
weave-feedback
branch
from
f10d620 to
78375c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (1)
55-120: Authentication bypass and blocking I/O in async endpoint.The
/feedbackendpoint has two significant issues:
Authentication bypass: Unlike other workflow routes, this endpoint does not use
SessionManager.sessionor the HTTP auth flow handler, bypassing any configured authentication/authorization.Blocking I/O: The synchronous Weave client calls (
weave.init(),client.get_call(),call.feedback.add_reaction()) perform network I/O and will block the FastAPI event loop.Apply this diff to address both issues:
async def _add_weave_feedback_route(self, app: FastAPI, builder: WorkflowBuilder): """Add the Weave feedback endpoint if Weave telemetry is configured.""" # Find Weave telemetry exporter configuration weave_config = None - for name, exporter_config in builder._telemetry_exporters.items(): + for exporter_config in builder._telemetry_exporters.values(): if hasattr(exporter_config.config, 'project') and \ exporter_config.config.__class__.__name__ == 'WeaveTelemetryExporter': weave_config = exporter_config.config break if not weave_config: logger.debug("Weave telemetry not configured, skipping feedback endpoint") return try: + import asyncio + from nat.front_ends.fastapi.session_manager import SessionManager - async def add_chat_feedback(request: Request, payload: dict): + async def add_chat_feedback(payload: dict): """Add reaction feedback for an assistant message via observability trace ID.""" - import weave - - # Get the weave project name from the configuration - entity = getattr(weave_config, 'entity', None) - project = getattr(weave_config, 'project') - weave_project = f"{entity}/{project}" if entity else project - - # Extract parameters from payload observability_trace_id = payload.get('observability_trace_id') reaction_type = payload.get('reaction_type') if not observability_trace_id or not reaction_type: raise HTTPException(status_code=400, detail="observability_trace_id and reaction_type are required") - # Add feedback directly + # Offload blocking Weave calls to thread pool + def _add_feedback(): + import weave + entity = weave_config.entity if hasattr(weave_config, 'entity') else None + project = weave_config.project + weave_project = f"{entity}/{project}" if entity else project + client = weave.init(weave_project) + call = client.get_call(observability_trace_id) + call.feedback.add_reaction(reaction_type) + try: - client = weave.init(weave_project) - call = client.get_call(observability_trace_id) - call.feedback.add_reaction(reaction_type) + await asyncio.to_thread(_add_feedback) return {"message": f"Added reaction '{reaction_type}' to call {observability_trace_id}"} except Exception as e: - logger.error("Failed to add feedback to Weave: %s", e) + logger.exception("Failed to add feedback to Weave") raise HTTPException(status_code=500, detail=f"Failed to add feedback: {str(e)}") from eNote: If authentication is required for this endpoint, wrap the handler body with
SessionManagerand the auth callback similar to other routes.
🧹 Nitpick comments (7)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (2)
29-36: Consider using a temp path helper for NAT_CONFIG_FILE in testsStatic analysis flags
/tmp/dummy_nat_config.ymlas a potentially insecure temp path. It’s harmless here since you never touch the file, but if you want to silence S108 and be future‑proof, consider deriving this viatempfile.gettempdir()or pytest’stmp_pathfixture instead of a hard‑coded/tmppath.
29-114: Align test fixture style with project guidelines (optional)To match the documented conventions, you could:
- Rename
setup_envtofixture_setup_envand supplyname="setup_env"in@pytest.fixture.- Add type hints (e.g.,
def fixture_setup_env() -> None:) and to the async tests’ parameters/returns.Not required for correctness, but it keeps new tests consistent with the rest of the suite and the stated guidelines.
tests/nat/server/test_unified_api_server.py (1)
376-401: Optional: add HTTP header assertions for observability_trace_idYou now verify the SSE
observability_trace:event, but not theObservability-Trace-IdHTTP header on non‑streaming chat/OpenAI endpoints. Adding small integration tests that assert this header matches a mockedobservability_trace_id(similar to the SSE test) would close the loop on header propagation.src/nat/front_ends/fastapi/intermediate_steps_subscriber.py (1)
35-36: Intermediate step subscriber correctly emits observability trace once (lint note)The new
trace_id_emittedguard andcontext.observability_trace_idcheck ensure a singleResponseObservabilityTraceis queued when a trace ID becomes available, without disturbing existing intermediate step handling.If you want to appease RUF006 and avoid import‑in‑callback style, you could optionally:
- Move the
ResponseObservabilityTraceimport to the module top, and- Either assign the
create_taskresult to_or track tasks in a local list/TaskGroup.Behavior is fine as is; these would just be lint/clarity improvements.
Also applies to: 47-56
src/nat/front_ends/fastapi/message_validator.py (1)
402-402: Remove redundant exception object from logging.exception call.The
logging.exception()method automatically includes exception details, so passingstr(e)is redundant.Apply this diff:
except Exception as e: - logger.exception("Error creating observability trace message: %s", str(e)) + logger.exception("Error creating observability trace message") return Nonepackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (2)
60-60: Unused loop variable.The loop variable
nameis not used within the loop body.Apply this diff:
- for name, exporter_config in builder._telemetry_exporters.items(): + for exporter_config in builder._telemetry_exporters.values():
77-78: Avoid getattr with constant attribute.Using
getattrwith a constant string'project'is not safer than direct attribute access and reduces readability.Apply this diff:
- entity = getattr(weave_config, 'entity', None) - project = getattr(weave_config, 'project') + entity = weave_config.entity if hasattr(weave_config, 'entity') else None + project = weave_config.project
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/source/reference/api-server-endpoints.md(1 hunks)docs/source/reference/websockets.md(2 hunks)docs/source/workflows/observe/index.md(1 hunks)docs/source/workflows/observe/observe-workflow-with-weave.md(1 hunks)examples/observability/simple_calculator_observability/configs/config-weave.yml(1 hunks)packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py(1 hunks)packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py(1 hunks)packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py(1 hunks)src/nat/builder/context.py(2 hunks)src/nat/data_models/api_server.py(3 hunks)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py(5 hunks)src/nat/front_ends/fastapi/intermediate_steps_subscriber.py(2 hunks)src/nat/front_ends/fastapi/message_handler.py(7 hunks)src/nat/front_ends/fastapi/message_validator.py(5 hunks)tests/nat/server/test_unified_api_server.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/source/workflows/observe/index.md
- packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
- docs/source/workflows/observe/observe-workflow-with-weave.md
- docs/source/reference/api-server-endpoints.md
🧰 Additional context used
📓 Path-based instructions (6)
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pydocs/source/reference/websockets.mdexamples/observability/simple_calculator_observability/configs/config-weave.ymltests/nat/server/test_unified_api_server.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/builder/context.pysrc/nat/front_ends/fastapi/message_validator.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
src/nat/**/*
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/builder/context.pysrc/nat/front_ends/fastapi/message_validator.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.py
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/reference/websockets.md
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should
contain a README.md or file README.ipynb.
- If an example contains Python code, it should be placed in a subdirectory named
src/and should
contain apyproject.tomlfile. Optionally, it might also contain scripts in ascripts/directory.- If an example contains YAML files, they should be placed in a subdirectory named
configs/. - If an example contains sample data files, they should be placed in a subdirectory nameddata/, and should
be checked into git-lfs.
Files:
examples/observability/simple_calculator_observability/configs/config-weave.yml
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/server/test_unified_api_server.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
🧬 Code graph analysis (8)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (2)
src/nat/builder/context.py (4)
Context(122-341)observability_trace_id(221-225)get(118-119)get(331-341)src/nat/front_ends/fastapi/response_helpers.py (1)
generate_single_response(108-117)
tests/nat/server/test_unified_api_server.py (3)
src/nat/data_models/api_server.py (8)
ObservabilityTraceContent(716-718)ResponseIntermediateStep(482-494)ResponsePayloadOutput(509-518)SystemIntermediateStepContent(635-638)SystemResponseContent(661-664)WebSocketMessageType(531-541)WebSocketObservabilityTraceMessage(721-735)ChatResponseChunk(391-479)src/nat/builder/context.py (3)
observability_trace_id(221-225)get(118-119)get(331-341)src/nat/front_ends/fastapi/message_validator.py (3)
MessageValidator(64-403)validate_message(91-120)create_observability_trace_message(375-403)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (3)
src/nat/builder/workflow_builder.py (1)
WorkflowBuilder(154-1271)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (2)
add_routes(302-315)config(128-129)src/nat/utils/type_utils.py (1)
override(56-57)
src/nat/front_ends/fastapi/message_handler.py (2)
src/nat/data_models/api_server.py (3)
ResponseObservabilityTrace(497-506)WebSocketObservabilityTraceMessage(721-735)WebSocketMessageType(531-541)src/nat/front_ends/fastapi/message_validator.py (1)
create_observability_trace_message(375-403)
src/nat/front_ends/fastapi/message_validator.py (2)
src/nat/data_models/api_server.py (4)
ObservabilityTraceContent(716-718)ResponseObservabilityTrace(497-506)WebSocketMessageType(531-541)WebSocketObservabilityTraceMessage(721-735)src/nat/builder/context.py (5)
Context(122-341)get(118-119)get(331-341)observability_trace_id(221-225)conversation_id(190-197)
src/nat/data_models/api_server.py (1)
src/nat/builder/context.py (2)
observability_trace_id(221-225)conversation_id(190-197)
src/nat/front_ends/fastapi/intermediate_steps_subscriber.py (2)
src/nat/builder/context.py (1)
observability_trace_id(221-225)src/nat/data_models/api_server.py (1)
ResponseObservabilityTrace(497-506)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (5)
src/nat/front_ends/fastapi/fastapi_front_end_config.py (1)
FastApiFrontEndConfig(151-293)packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (1)
WeaveFastAPIPluginWorker(29-120)packages/nvidia_nat_weave/src/nat/plugins/weave/register.py (1)
WeaveTelemetryExporter(28-46)packages/nvidia_nat_test/src/nat/test/functions.py (1)
EchoFunctionConfig(28-29)packages/nvidia_nat_test/src/nat/test/utils.py (1)
build_nat_client(93-123)
🪛 Ruff (0.14.5)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
60-60: Loop control variable name not used within loop body
Rename unused name to _name
(B007)
72-72: Unused function argument: request
(ARG001)
78-78: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
86-86: Abstract raise to an inner function
(TRY301)
93-93: Consider moving this statement to an else block
(TRY300)
95-95: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
96-96: Use explicit conversion flag
Replace with conversion flag
(RUF010)
119-119: Do not catch blind exception: Exception
(BLE001)
src/nat/front_ends/fastapi/message_validator.py
402-402: Redundant exception object included in logging.exception call
(TRY401)
src/nat/front_ends/fastapi/intermediate_steps_subscriber.py
54-54: Store a reference to the return value of loop.create_task
(RUF006)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
34-34: Probable insecure usage of temporary file or directory: "/tmp/dummy_nat_config.yml"
(S108)
🔇 Additional comments (6)
src/nat/builder/context.py (1)
69-76: Observability trace context plumbing looks correctThe new
observability_trace_idcontext var and correspondingContext.observability_trace_idaccessor are typed, consistent with existing workflow IDs, and provide a clean, backend‑agnostic hook for downstream propagation.Also applies to: 220-225
examples/observability/simple_calculator_observability/configs/config-weave.yml (1)
17-20: FastAPI front_end wiring for Weave looks consistentConfiguring the
fastapifront end withrunner_class: nat.plugins.weave.fastapi_plugin_worker.WeaveFastAPIPluginWorkercleanly ties this example into the Weave feedback integration and matches the Weave telemetry block below.docs/source/reference/websockets.md (1)
40-41: WebSocket observability_trace_message docs align with modelsThe added
observability_trace_messagetype and example JSON match theWebSocketMessageType.OBSERVABILITY_TRACE_MESSAGEandWebSocketObservabilityTraceMessage/ObservabilityTraceContentschema, and clearly document that the trace ID is sent once post‑completion.Also applies to: 391-405
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (1)
42-43: HTTP observability header propagation is implemented cleanlyUsing
add_context_headers_to_responseto attachObservability-Trace-IdfromContext.get().observability_trace_idin the non‑streaming GET/POST and OpenAI v1 code paths is a minimal, backward‑compatible way to expose the trace ID to clients while keeping the logic centralized.Also applies to: 672-677, 721-731, 769-779, 830-852
src/nat/front_ends/fastapi/message_handler.py (1)
74-75: WebSocket observability trace queuing and emission look correctThe
_pending_observability_tracemechanism correctly:
- Captures at most one
ResponseObservabilityTraceper workflow run,- Resets between requests,
- And emits a dedicated
observability_trace_messageonly after the finalsystem_response_message.This matches the data model and docs while avoiding interleaving trace metadata with the main token stream.
Also applies to: 181-182, 257-263, 347-367
tests/nat/server/test_unified_api_server.py (1)
42-43: Observability trace tests exercise SSE and WebSocket flows wellThe new tests:
- Validate raw
observability_trace_message→WebSocketObservabilityTraceMessageconversion,- Cover
create_observability_trace_message,- And confirm the chat_stream SSE includes an
observability_traceevent with the mocked trace ID while still yielding validChatResponseChunkdata.This gives solid coverage of the new observability trace surfaces.
Also applies to: 297-306, 376-401, 479-486, 659-673
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (1)
72-96: Critical: Address auth bypass and blocking I/O issues.This endpoint has critical issues that were flagged in a previous review but remain unresolved:
Authentication bypass: Unlike standard workflow routes,
add_chat_feedbackdoes not useSessionManager.sessionor the HTTP auth flow handler, effectively bypassing any configured authentication/authorization. In authenticated deployments, this creates a security risk.Blocking I/O in async handler:
weave.init(...),client.get_call(...), andcall.feedback.add_reaction(...)are synchronous operations that may perform network I/O. Running them directly in an async FastAPI handler blocks the event loop and degrades server performance under load.Exception logging: Line 95 should use
logger.exception()instead oflogger.error()when catching and logging exceptions without re-raising, per the coding guidelines.Unused parameter: The
requestparameter is declared but never used (unless needed for future auth integration).Apply these fixes:
async def _add_weave_feedback_route(self, app: FastAPI, builder: WorkflowBuilder): ... try: + import asyncio + + session_manager = SessionManager(await builder.build()) - async def add_chat_feedback(request: Request, payload: dict): + async def add_chat_feedback(request: Request, payload: dict): """Add reaction feedback for an assistant message via observability trace ID.""" - import weave - - # Get the weave project name from the configuration - entity = weave_config.entity if hasattr(weave_config, 'entity') else None - project = weave_config.project - weave_project = f"{entity}/{project}" if entity else project - - # Extract parameters from payload - observability_trace_id = payload.get('observability_trace_id') - reaction_type = payload.get('reaction_type') - - if not observability_trace_id or not reaction_type: - raise HTTPException(status_code=400, detail="observability_trace_id and reaction_type are required") - - # Add feedback directly - try: - client = weave.init(weave_project) - call = client.get_call(observability_trace_id) - call.feedback.add_reaction(reaction_type) - return {"message": f"Added reaction '{reaction_type}' to call {observability_trace_id}"} - except Exception as e: - logger.error("Failed to add feedback to Weave: %s", e) - raise HTTPException(status_code=500, detail=f"Failed to add feedback: {str(e)}") from e + async with session_manager.session(http_connection=request, + user_authentication_callback=self._http_flow_handler.authenticate): # type: ignore[attr-defined] + import weave + + # Get the weave project name from the configuration + entity = weave_config.entity + project = weave_config.project + weave_project = f"{entity}/{project}" if entity else project + + # Extract parameters from payload + observability_trace_id = payload.get('observability_trace_id') + reaction_type = payload.get('reaction_type') + + if not observability_trace_id or not reaction_type: + raise HTTPException(status_code=400, detail="observability_trace_id and reaction_type are required") + + # Add feedback in a thread to avoid blocking the event loop + def _do_feedback(): + client = weave.init(weave_project) + call = client.get_call(observability_trace_id) + call.feedback.add_reaction(reaction_type) + + try: + await asyncio.to_thread(_do_feedback) + return {"message": f"Added reaction '{reaction_type}' to call {observability_trace_id}"} + except Exception as e: + logger.exception("Failed to add feedback to Weave") + raise HTTPException(status_code=500, detail=f"Failed to add feedback: {str(e)}") from eAs per coding guidelines.
🧹 Nitpick comments (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (1)
58-64: Useisinstancefor more robust type checking.The current approach using
__class__.__name__ == 'WeaveTelemetryExporter'is fragile and can break with subclasses or dynamic imports.Apply this diff:
+from nat.plugins.weave.register import WeaveTelemetryExporter + async def _add_weave_feedback_route(self, app: FastAPI, builder: WorkflowBuilder): """Add the Weave feedback endpoint if Weave telemetry is configured.""" # Find Weave telemetry exporter configuration weave_config = None for exporter_config in builder._telemetry_exporters.values(): - if hasattr(exporter_config.config, 'project') and \ - exporter_config.config.__class__.__name__ == 'WeaveTelemetryExporter': + if isinstance(exporter_config.config, WeaveTelemetryExporter): weave_config = exporter_config.config break
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py(1 hunks)packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
🧬 Code graph analysis (2)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (3)
src/nat/builder/workflow_builder.py (1)
WorkflowBuilder(154-1271)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (2)
add_routes(302-315)config(128-129)src/nat/utils/type_utils.py (1)
override(56-57)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (5)
src/nat/front_ends/fastapi/fastapi_front_end_config.py (1)
FastApiFrontEndConfig(151-293)packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (1)
WeaveFastAPIPluginWorker(29-120)packages/nvidia_nat_weave/src/nat/plugins/weave/register.py (1)
WeaveTelemetryExporter(28-46)packages/nvidia_nat_test/src/nat/test/functions.py (1)
EchoFunctionConfig(28-29)packages/nvidia_nat_test/src/nat/test/utils.py (1)
build_nat_client(93-123)
🪛 Ruff (0.14.5)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
72-72: Unused function argument: request
(ARG001)
86-86: Abstract raise to an inner function
(TRY301)
93-93: Consider moving this statement to an else block
(TRY300)
95-95: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
96-96: Use explicit conversion flag
Replace with conversion flag
(RUF010)
119-119: Do not catch blind exception: Exception
(BLE001)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
47-47: Unused lambda argument: args
(ARG005)
47-47: Unused lambda argument: kwargs
(ARG005)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Pipeline / Check
🔇 Additional comments (8)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (5)
1-28: LGTM!License header and imports are correct and appropriate for the test module.
31-55: LGTM!Both fixtures provide good test isolation. The
mock_weavefixture properly mocks Weave client interactions to avoid authentication issues during unit tests. The static analysis warning about unused lambda arguments is a false positive—the arguments are intentionally ignored in the mock.
58-79: LGTM!The test correctly verifies that the
/feedbackendpoint is registered when Weave telemetry is configured. Allowing both 200 and 500 status codes is appropriate given the mocked environment.
82-119: LGTM!Excellent validation coverage:
- Confirms
/feedbackendpoint is not registered when Weave is not configured (404).- Verifies required parameter validation returns 400 for missing fields.
122-134: LGTM!Good coverage to ensure
WeaveFastAPIPluginWorkerpreserves standard route functionality when extending the base worker class.packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (3)
1-44: LGTM!License header, imports, and class documentation are correct and follow coding guidelines.
46-53: LGTM!The override correctly calls the parent's
add_routesbefore adding Weave-specific routes, maintaining proper inheritance hierarchy.
98-120: LGTM!Route registration with proper OpenAPI metadata and error handling for registration failures is well-implemented.
thepatrickchin
force-pushed
the
weave-feedback
branch
from
56ff366 to
d37e3ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
docs/source/reference/api-server-endpoints.md (1)
539-557: Consider moving the Feedback Endpoint section before Migration Guide.The Feedback Endpoint documentation is placed after the Migration Guide section, which may be confusing since it's a new feature rather than migration-related content. Consider moving it to appear with the other endpoint documentation sections (after Chat Streaming or before the OpenAI Chat Completions section).
The documentation content itself is clear and comprehensive with proper request/response examples.
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (2)
31-38: Environment variable fixture may leak across tests.The fixture sets
NAT_CONFIG_FILEbut doesn't restore the original value on teardown. If another test depends on the original state, this could cause issues.Apply this diff to properly restore the environment:
@pytest.fixture(name="setup_env", autouse=True) def fixture_setup_env() -> None: """Set up environment variables for tests.""" # Set a dummy config file path for tests that don't use Dask + original_value = os.environ.get("NAT_CONFIG_FILE") if "NAT_CONFIG_FILE" not in os.environ: temp_dir = tempfile.gettempdir() os.environ["NAT_CONFIG_FILE"] = os.path.join(temp_dir, "dummy_nat_config.yml") yield + # Restore original state + if original_value is None: + os.environ.pop("NAT_CONFIG_FILE", None) + else: + os.environ["NAT_CONFIG_FILE"] = original_value
58-79: Consider adding a test for successful feedback submission.This test validates the endpoint exists but doesn't verify successful operation. While the mock makes full integration difficult, consider enhancing the mock to verify the happy path returns 200.
Example enhancement:
async def test_weave_feedback_endpoint_success() -> None: """Test successful feedback submission with properly mocked Weave client.""" mock_call = MagicMock() mock_call.feedback.add_reaction = MagicMock() config = Config( general=GeneralConfig( front_end=FastApiFrontEndConfig(), telemetry={"tracing": {"weave": WeaveTelemetryExporter(project="test-project")}} ), workflow=EchoFunctionConfig(), ) async with build_nat_client(config, worker_class=WeaveFastAPIPluginWorker) as client: # Would need to mock client.get_call to return mock_call response = await client.post("/feedback", json={ "observability_trace_id": "test-trace-id", "reaction_type": "👍" }) # Verify successful responsesrc/nat/front_ends/fastapi/intermediate_steps_subscriber.py (1)
47-55: Store task reference to prevent potential garbage collection issues.The static analysis correctly identifies that
loop.create_task()returns a task reference that should be stored. While the_q.put()operation is typically fast, not storing the reference could theoretically allow the task to be garbage collected before completion in edge cases.Apply this diff to store the task reference (consistent with line 67):
+ # Store reference to prevent GC before completion + _ = loop.create_task(_q.put(ResponseObservabilityTrace(observability_trace_id=observability_trace_id))) - loop.create_task(_q.put(ResponseObservabilityTrace(observability_trace_id=observability_trace_id)))Note: Line 67 already follows the pattern
loop.create_task(_q.put(adapted))without storing the reference. For consistency, either update both or acknowledge the existing pattern is acceptable for this use case.packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (2)
89-96: Synchronous Weave calls may block the async event loop.
weave.init(),client.get_call(), andcall.feedback.add_reaction()are synchronous and perform network I/O. Running them directly in an async endpoint can block the event loop under load.Consider offloading to a thread:
+import asyncio + async def add_chat_feedback(request: Request, payload: dict): ... - try: - client = weave.init(weave_project) - call = client.get_call(observability_trace_id) - call.feedback.add_reaction(reaction_type) - return {"message": f"Added reaction '{reaction_type}' to call {observability_trace_id}"} - except Exception as e: - logger.error("Failed to add feedback to Weave: %s", e) - raise HTTPException(status_code=500, detail=f"Failed to add feedback: {e!s}") from e + def _sync_add_feedback(): + client = weave.init(weave_project) + call = client.get_call(observability_trace_id) + call.feedback.add_reaction(reaction_type) + + try: + await asyncio.to_thread(_sync_add_feedback) + return {"message": f"Added reaction '{reaction_type}' to call {observability_trace_id}"} + except Exception as e: + logger.error("Failed to add feedback to Weave: %s", e) + raise HTTPException(status_code=500, detail=f"Failed to add feedback: {e!s}") from eThis also addresses RUF010 by using
{e!s}instead of{str(e)}.
72-72: Unusedrequestparameter.The
requestparameter isn't used in the handler. If authentication is needed in the future (per earlier discussions), it can be added then. For now, you could prefix with underscore to indicate intentional non-use, or keep as-is if planning near-term auth integration.-async def add_chat_feedback(request: Request, payload: dict): +async def add_chat_feedback(_request: Request, payload: dict):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/source/reference/api-server-endpoints.md(1 hunks)docs/source/reference/websockets.md(2 hunks)docs/source/workflows/observe/index.md(1 hunks)docs/source/workflows/observe/observe-workflow-with-weave.md(1 hunks)examples/observability/simple_calculator_observability/configs/config-weave.yml(1 hunks)packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py(1 hunks)packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py(1 hunks)packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py(1 hunks)src/nat/builder/context.py(2 hunks)src/nat/data_models/api_server.py(3 hunks)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py(5 hunks)src/nat/front_ends/fastapi/intermediate_steps_subscriber.py(2 hunks)src/nat/front_ends/fastapi/message_handler.py(7 hunks)src/nat/front_ends/fastapi/message_validator.py(5 hunks)tests/nat/server/test_unified_api_server.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/nat/builder/context.py
- docs/source/workflows/observe/observe-workflow-with-weave.md
- docs/source/reference/websockets.md
- examples/observability/simple_calculator_observability/configs/config-weave.yml
- packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
- docs/source/workflows/observe/index.md
- tests/nat/server/test_unified_api_server.py
🧰 Additional context used
📓 Path-based instructions (16)
**/*.{md,rst,py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use 'NVIDIA NeMo Agent toolkit' on first use, then 'NeMo Agent toolkit' for subsequent references
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pydocs/source/reference/api-server-endpoints.mdsrc/nat/front_ends/fastapi/message_handler.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,toml,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' for environment variable prefixes and informal comments
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 20 and PEP 8 for Python style guidelines
Run yapf second (PEP 8 base, column_limit = 120) for Python formatting
Use ruff check --fix (via pre-commit) for linting using configuration embedded in pyproject.toml, fix warnings unless explicitly ignored
Use snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
All public APIs require Python 3.11+ type hints on parameters and return values
Prefer collections.abc / typing abstractions (Sequence over list) for type hints
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Preserve stack traces and prevent duplicate logging when handling exceptions; use bare raise statements and logger.error() when re-raising
When catching and logging exceptions without re-raising, always use logger.exception() to capture the full stack trace information
Provide Google-style docstrings for every public module, class, function and CLI command
The first line of docstrings must be a concise description ending with a period (Vale checks this)
Surround code entities with backticks in docstrings to avoid Vale false-positives
Validate and sanitise all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorised operations whenever beneficial and feasible
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,js,ts,yaml,yml,json,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces, never tabs, and ensure every file ends with a single newline
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pydocs/source/reference/api-server-endpoints.mdsrc/nat/front_ends/fastapi/message_handler.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,env,toml,yaml,yml,json}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never commit API keys, credentials or personal data; use environment variables or .env files excluded from Git
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Every file must start with the standard SPDX Apache-2.0 header
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived automatically by setuptools-scm; never hard-code them in code or docs
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pydocs/source/reference/api-server-endpoints.mdsrc/nat/front_ends/fastapi/message_handler.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php,sh}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All source files must include the SPDX Apache-2.0 header template
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values (except for return values of
None,
in that situation no return type hint is needed).
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pydocs/source/reference/api-server-endpoints.mdsrc/nat/front_ends/fastapi/message_handler.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
src/nat/**/*
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_validator.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use 'NeMo Agent Toolkit' (capitalize 'T') when the name appears in headings
Files:
docs/source/reference/api-server-endpoints.md
docs/**/*.{md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use deprecated names: Agent Intelligence toolkit, aiqtoolkit, AgentIQ, AIQ/aiq in documentation (unless intentionally referring to deprecated versions)
Files:
docs/source/reference/api-server-endpoints.md
docs/source/**/*.{md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.{md,rst}: Documentation must be clear and comprehensive, without TODOs, FIXMEs, or placeholder text
Ensure documentation is free of offensive or outdated terms
Ensure documentation is free of spelling mistakes and do not use words listed in ci/vale/styles/config/vocabularies/nat/reject.txt
Files:
docs/source/reference/api-server-endpoints.md
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/reference/api-server-endpoints.md
**/test_*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/test_*.py: Use pytest with pytest-asyncio for asynchronous code testing
Test functions should be named using the test_ prefix, using snake_case
Extract frequently repeated code into pytest fixtures, which should be named using the fixture_ prefix and define the name argument in the decorator
Mock external services with pytest_httpserver or unittest.mock instead of hitting live endpoints
Mark slow tests with @pytest.mark.slow so they can be skipped in the default test suite
Mark integration tests requiring external services with @pytest.mark.integration so they can be skipped in the default test suite
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
🧠 Learnings (2)
📚 Learning: 2025-11-20T10:27:43.583Z
Learnt from: thepatrickchin
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 781
File: src/nat/data_models/api_server.py:721-735
Timestamp: 2025-11-20T10:27:43.583Z
Learning: In src/nat/data_models/api_server.py, WebSocket message classes follow an established pattern for timestamp defaults using `timestamp: str = str(datetime.datetime.now(datetime.UTC))`. When adding new WebSocket message classes, maintain consistency with this pattern rather than introducing different approaches for individual classes.
Applied to files:
src/nat/front_ends/fastapi/message_handler.pysrc/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.py
📚 Learning: 2025-11-20T10:29:16.010Z
Learnt from: thepatrickchin
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 781
File: src/nat/front_ends/fastapi/message_validator.py:378-378
Timestamp: 2025-11-20T10:29:16.010Z
Learning: In src/nat/front_ends/fastapi/message_validator.py, all message creation functions (create_system_response_token_message, create_system_intermediate_step_message, create_system_interaction_message, create_observability_trace_message) use the pattern `message_id: str | None = str(uuid.uuid4())` for default message ID generation. When adding new message creation functions, maintain consistency with this established pattern rather than introducing different approaches for individual functions.
Applied to files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.py
🧬 Code graph analysis (4)
src/nat/front_ends/fastapi/message_handler.py (3)
src/nat/data_models/api_server.py (3)
ResponseObservabilityTrace(497-506)WebSocketObservabilityTraceMessage(721-735)WebSocketMessageType(531-541)src/nat/front_ends/fastapi/message_validator.py (1)
create_observability_trace_message(375-403)src/nat/builder/context.py (1)
conversation_id(190-197)
src/nat/data_models/api_server.py (2)
src/nat/builder/context.py (2)
observability_trace_id(221-225)conversation_id(190-197)packages/nvidia_nat_data_flywheel/tests/observability/processor/test_dfw_record_processor.py (1)
model_dump_json(336-338)
src/nat/front_ends/fastapi/intermediate_steps_subscriber.py (2)
src/nat/builder/context.py (1)
observability_trace_id(221-225)src/nat/data_models/api_server.py (1)
ResponseObservabilityTrace(497-506)
src/nat/front_ends/fastapi/message_validator.py (1)
src/nat/data_models/api_server.py (4)
ObservabilityTraceContent(716-718)ResponseObservabilityTrace(497-506)WebSocketMessageType(531-541)WebSocketObservabilityTraceMessage(721-735)
🪛 Ruff (0.14.8)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
72-72: Unused function argument: request
(ARG001)
86-86: Abstract raise to an inner function
(TRY301)
93-93: Consider moving this statement to an else block
(TRY300)
95-95: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
96-96: Use explicit conversion flag
Replace with conversion flag
(RUF010)
119-119: Do not catch blind exception: Exception
(BLE001)
src/nat/front_ends/fastapi/intermediate_steps_subscriber.py
54-54: Store a reference to the return value of loop.create_task
(RUF006)
src/nat/front_ends/fastapi/message_validator.py
402-402: Redundant exception object included in logging.exception call
(TRY401)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
47-47: Unused lambda argument: args
(ARG005)
47-47: Unused lambda argument: kwargs
(ARG005)
🔇 Additional comments (23)
src/nat/front_ends/fastapi/message_handler.py (6)
33-42: LGTM: New imports for observability trace support.The imports for
ResponseObservabilityTraceandWebSocketObservabilityTraceMessageare correctly added to support the new observability trace handling in WebSocket flow.
74-74: LGTM: Instance variable for pending observability trace.Properly initialized to
Noneto track the pending trace that will be sent after the completion message.
181-181: LGTM: Reset pending trace at workflow start.Correctly resets
_pending_observability_tracetoNoneat the beginning of each workflow request to ensure clean state between requests.
257-262: LGTM: WebSocket observability trace message creation.The new branch correctly delegates to
create_observability_trace_messagefollowing the established pattern for other message types.
362-367: LGTM: Observability trace sent after completion message.The implementation correctly:
- Places the trace sending in the
finallyblock to ensure it's sent even on errors- Sends the trace after the completion message as intended by the design
- Clears the pending trace after sending to avoid memory retention
347-351: Add documentation clarifying single-trace expectation.The code stores only the first
ResponseObservabilityTraceand silently ignores subsequent ones. If this is intentional (and the design guarantees a single trace per workflow run), add a comment explaining this assumption. If multiple traces are possible, consider handling or logging them explicitly rather than silently discarding them.packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (2)
41-55: Mock fixture looks correct but static analysis flags unused args.The lambda
lambda *args, **kwargs: mock_weave_clientintentionally ignores arguments to match theweave.initsignature. The static analysis warning (ARG005) is a false positive in this context.
122-134: LGTM: Standard routes test.Good coverage ensuring that extending
FastApiFrontEndPluginWorkerdoesn't break existing functionality.src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (5)
42-42: LGTM: Context import added.Required for accessing
observability_trace_idin the header propagation logic.
728-730: LGTM: Header propagation in GET single endpoint.The observability trace header is correctly added after generating the response.
776-778: LGTM: Header propagation in POST single endpoint.Consistent with the GET endpoint implementation.
849-851: LGTM: Header propagation in OpenAI-compatible endpoint.Correctly applied only to the non-streaming path. Streaming responses handle trace propagation differently (via the response body).
672-676: Consider exposingObservability-Trace-Idin CORS headers.The new header may not be accessible to cross-origin frontend clients unless it's included in
expose_headers. If the frontend needs to read this header, the CORS configuration should expose it. Verify if the current CORS configuration already includes this header and update if necessary.src/nat/front_ends/fastapi/intermediate_steps_subscriber.py (2)
35-35: LGTM: Guard flag for one-time emission.The
trace_id_emittedflag correctly ensures the observability trace is emitted only once per subscription.
53-53: Deferred import is acceptable here.The import inside the callback avoids potential circular import issues and is only executed once due to the
trace_id_emittedguard.src/nat/front_ends/fastapi/message_validator.py (3)
79-89: Clean helper extraction with appropriate exception handling.The helper correctly catches specific exceptions (
ImportError,AttributeError,KeyError) rather than bareException, and the docstring follows the project's style. This addresses the earlier refactoring feedback well.
166-168: LGTM — conversion follows established pattern.The handling of
ResponseObservabilityTracecorrectly extracts theobservability_trace_idfrom the data model and creates the appropriate content type, consistent with how other response types are handled in this method.
375-403: Implementation follows established patterns; minor consistency note.The method correctly follows the file's established patterns for
message_idandtimestampdefaults. Based on learnings, this consistency is intentional.The
logger.exception("...: %s", str(e))on line 402 includes a redundantstr(e)sincelogger.exceptionautomatically captures exception info. However, this matches the pattern used increate_system_response_token_message(line 293),create_system_intermediate_step_message(line 332), andcreate_system_interaction_message(line 372), so maintaining consistency is appropriate.src/nat/data_models/api_server.py (2)
497-506: Well-designed streaming serialization for observability traces.The
get_stream_data()method correctly uses a distinctobservability_trace:prefix, enabling the frontend SSE parser to differentiate this event type fromdata:andintermediate_data:events. The implementation is clean and follows theResponseSerializablecontract.
716-735: Models follow established conventions.
ObservabilityTraceContentcorrectly usesextra="forbid"consistent with other content models, andWebSocketObservabilityTraceMessageusesextra="allow"matching other WebSocket message classes. The omission ofthread_idandstatusfields is appropriate since this is a one-time message sent after workflow completion rather than a conversational message requiring status tracking. Per learnings, the timestamp default pattern is intentional.packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (3)
29-44: Good class documentation with usage example.The docstring clearly explains the purpose and provides a practical YAML configuration example, which will help users integrate this worker correctly.
55-68: Robust config detection with appropriate fallback.The detection logic correctly iterates through telemetry exporters and checks both
hasattrand class name to identifyWeaveTelemetryExporter. The debug log and early return provide clean behavior when Weave isn't configured.Minor: The loop variable could be named
_or_exporter_configif you don't use the iteration name directly, but this is negligible.
117-119: Appropriate error handling for route registration.Using
logger.warningfor registration failure is appropriate since the server can continue operating without this endpoint. The broadExceptioncatch is acceptable here as registration failures shouldn't crash the server.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (2)
79-105: Auth alignment and async handling of Weave calls look good; a few small logging/ruff cleanups are possible.Functionally this handler is in good shape:
- It now runs under
SessionManager.session(..., user_authentication_callback=self._http_flow_handler.authenticate), so/feedbackrespects the same auth/session flow as other routes.- Potentially blocking Weave client work is correctly offloaded via
asyncio.to_thread, keeping the FastAPI event loop responsive.- Validation of
observability_trace_idandreaction_typebefore doing any Weave I/O is appropriate.The remaining points are minor style/observability items driven by Ruff and the logging guidelines:
- Ruff TRY300 / TRY301 / TRY400 / RUF010: If you’re aiming for a clean
ruffrun, you can:
Narrow the
tryto justawait asyncio.to_thread(add_weave_feedback)and use anelse:for the successreturn, e.g.:try:await asyncio.to_thread(add_weave_feedback)return {"message": f"Added reaction '{reaction_type}' to call {observability_trace_id}"}except Exception as e:logger.error("Failed to add feedback to Weave: %s", e)raise HTTPException(status_code=500, detail=f"Failed to add feedback: {str(e)}") from e
try:await asyncio.to_thread(add_weave_feedback)except Exception as e: # noqa: TRY301logger.exception("Failed to add feedback to Weave")raise HTTPException(status_code=500,detail=f"Failed to add feedback: {e!s}", # addresses RUF010) from eelse:return {"message": f"Added reaction '{reaction_type}' to call {observability_trace_id}",}- The `# noqa: TRY301` is optional but may be useful if you prefer the straightforward `raise HTTPException(...) from e` pattern over introducing an extra helper.None of this is a blocker; the current implementation is functionally correct and secure.
You may want to run
ruff check --fixlocally to confirm these exact codes match your project’s configuration and adjust thenoqachoice accordingly.
127-128: Capture stack traces when route registration fails, and consider justifying the broadExceptioncatch.Catching a broad
Exceptionduring feedback-route registration is a reasonable way to avoid breaking the whole API server when Weave misconfiguration occurs, but:
- Per coding guidelines, when you log and do not re-raise, use
logger.exception()to capture the full stack trace.- Ruff flags
BLE001for the blindexcept Exception; if this broad catch is intentional as a hardening measure, it’s worth making that explicit.A small tweak that improves observability and documents the intent:
- except Exception as e: - logger.warning("Failed to register Weave feedback endpoint: %s", e) + except Exception: # noqa: BLE001 + # Swallow registration errors so nat can still start without Weave feedback, + # but log full stack for debugging. + logger.exception("Failed to register Weave feedback endpoint")This keeps the robustness behavior while aligning better with both Ruff and the logging guidance.
🧹 Nitpick comments (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (1)
60-77: Telemetry exporter detection is correct; string-based type check is a minor brittleness.The loop over
builder._telemetry_exporters.values()and direct use ofweave_config.entity/weave_config.projectcorrectly gates/feedbackregistration on Weave being configured. The only minor concern is matching on__class__.__name__ == 'WeaveTelemetryExporter', which is slightly brittle to refactors/renames.If you don’t mind depending directly on the exporter type, consider using an
isinstancecheck instead; otherwise this is fine as-is for a tightly coupled plugin.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{md,rst,py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use 'NVIDIA NeMo Agent toolkit' on first use, then 'NeMo Agent toolkit' for subsequent references
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,toml,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' for environment variable prefixes and informal comments
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 20 and PEP 8 for Python style guidelines
Run yapf second (PEP 8 base, column_limit = 120) for Python formatting
Use ruff check --fix (via pre-commit) for linting using configuration embedded in pyproject.toml, fix warnings unless explicitly ignored
Use snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
All public APIs require Python 3.11+ type hints on parameters and return values
Prefer collections.abc / typing abstractions (Sequence over list) for type hints
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Preserve stack traces and prevent duplicate logging when handling exceptions; use bare raise statements and logger.error() when re-raising
When catching and logging exceptions without re-raising, always use logger.exception() to capture the full stack trace information
Provide Google-style docstrings for every public module, class, function and CLI command
The first line of docstrings must be a concise description ending with a period (Vale checks this)
Surround code entities with backticks in docstrings to avoid Vale false-positives
Validate and sanitise all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorised operations whenever beneficial and feasible
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,js,ts,yaml,yml,json,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces, never tabs, and ensure every file ends with a single newline
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,env,toml,yaml,yml,json}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never commit API keys, credentials or personal data; use environment variables or .env files excluded from Git
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Every file must start with the standard SPDX Apache-2.0 header
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived automatically by setuptools-scm; never hard-code them in code or docs
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php,sh}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All source files must include the SPDX Apache-2.0 header template
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values (except for return values of
None,
in that situation no return type hint is needed).
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
🪛 Ruff (0.14.8)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
89-90: Abstract raise to an inner function
(TRY301)
101-101: Consider moving this statement to an else block
(TRY300)
103-103: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
104-104: Use explicit conversion flag
Replace with conversion flag
(RUF010)
127-127: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (1)
31-56: WeaveFastAPIPluginWorkeradd_routesoverride looks correct and minimal.Using
super().add_routes(app, builder)followed by_add_weave_feedback_routecleanly composes the Weave-specific behavior on top of the base FastAPI worker without altering existing routes. No functional or security issues spotted here.
thepatrickchin
force-pushed
the
weave-feedback
branch
from
3ba5a91 to
deb0ecf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (2)
62-65: Consider usingisinstance()instead of string-based class name comparison.String comparison on
__class__.__name__is fragile and won't catch subclasses. If the class is renamed or refactored, this will silently fail to detect Weave configuration.+ from nat.plugins.weave.weave_telemetry_exporter import WeaveTelemetryExporter + async def _add_weave_feedback_route(self, app: FastAPI, builder: WorkflowBuilder): """Add the Weave feedback endpoint if Weave telemetry is configured.""" # Find Weave telemetry exporter configuration weave_config = None for exporter_config in builder._telemetry_exporters.values(): - if exporter_config.config.__class__.__name__ == 'WeaveTelemetryExporter': + if isinstance(exporter_config.config, WeaveTelemetryExporter): weave_config = exporter_config.config break
127-127: Uselogger.exception()when catching without re-raising.Per coding guidelines, when catching exceptions without re-raising, use
logger.exception()to capture the full stack trace. This helps diagnose registration failures.except Exception as e: - logger.warning("Failed to register Weave feedback endpoint: %s", e) + logger.exception("Failed to register Weave feedback endpoint: %s", e)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/source/reference/rest-api/api-server-endpoints.md(1 hunks)docs/source/reference/rest-api/websockets.md(2 hunks)docs/source/run-workflows/observe/observe-workflow-with-weave.md(1 hunks)examples/observability/simple_calculator_observability/configs/config-weave.yml(1 hunks)packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py(1 hunks)packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py(1 hunks)packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py(1 hunks)src/nat/builder/context.py(2 hunks)src/nat/data_models/api_server.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (17)
**/*.{md,rst,py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use 'NVIDIA NeMo Agent toolkit' on first use, then 'NeMo Agent toolkit' for subsequent references
Files:
src/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pydocs/source/reference/rest-api/api-server-endpoints.mdsrc/nat/data_models/api_server.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pydocs/source/reference/rest-api/websockets.mddocs/source/run-workflows/observe/observe-workflow-with-weave.mdpackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,toml,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' for environment variable prefixes and informal comments
Files:
src/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pysrc/nat/data_models/api_server.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pyexamples/observability/simple_calculator_observability/configs/config-weave.ymlpackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 20 and PEP 8 for Python style guidelines
Run yapf second (PEP 8 base, column_limit = 120) for Python formatting
Use ruff check --fix (via pre-commit) for linting using configuration embedded in pyproject.toml, fix warnings unless explicitly ignored
Use snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
All public APIs require Python 3.11+ type hints on parameters and return values
Prefer collections.abc / typing abstractions (Sequence over list) for type hints
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Preserve stack traces and prevent duplicate logging when handling exceptions; use bare raise statements and logger.error() when re-raising
When catching and logging exceptions without re-raising, always use logger.exception() to capture the full stack trace information
Provide Google-style docstrings for every public module, class, function and CLI command
The first line of docstrings must be a concise description ending with a period (Vale checks this)
Surround code entities with backticks in docstrings to avoid Vale false-positives
Validate and sanitise all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorised operations whenever beneficial and feasible
Files:
src/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pysrc/nat/data_models/api_server.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,js,ts,yaml,yml,json,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces, never tabs, and ensure every file ends with a single newline
Files:
src/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pydocs/source/reference/rest-api/api-server-endpoints.mdsrc/nat/data_models/api_server.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pyexamples/observability/simple_calculator_observability/configs/config-weave.ymldocs/source/reference/rest-api/websockets.mddocs/source/run-workflows/observe/observe-workflow-with-weave.mdpackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,env,toml,yaml,yml,json}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never commit API keys, credentials or personal data; use environment variables or .env files excluded from Git
Files:
src/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pysrc/nat/data_models/api_server.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pyexamples/observability/simple_calculator_observability/configs/config-weave.ymlpackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Every file must start with the standard SPDX Apache-2.0 header
Files:
src/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pysrc/nat/data_models/api_server.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived automatically by setuptools-scm; never hard-code them in code or docs
Files:
src/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pydocs/source/reference/rest-api/api-server-endpoints.mdsrc/nat/data_models/api_server.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pydocs/source/reference/rest-api/websockets.mddocs/source/run-workflows/observe/observe-workflow-with-weave.mdpackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php,sh}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All source files must include the SPDX Apache-2.0 header template
Files:
src/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pysrc/nat/data_models/api_server.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values (except for return values of
None,
in that situation no return type hint is needed).
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pydocs/source/reference/rest-api/api-server-endpoints.mdsrc/nat/data_models/api_server.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pyexamples/observability/simple_calculator_observability/configs/config-weave.ymldocs/source/reference/rest-api/websockets.mddocs/source/run-workflows/observe/observe-workflow-with-weave.mdpackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
src/nat/**/*
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/builder/context.pysrc/nat/data_models/api_server.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.- When adding a new package, that new package name (as defined in the
pyproject.tomlfile) should
be added as a dependency to the nvidia-nat-all package inpackages/nvidia_nat_all/pyproject.toml
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use 'NeMo Agent Toolkit' (capitalize 'T') when the name appears in headings
Files:
docs/source/reference/rest-api/api-server-endpoints.mddocs/source/reference/rest-api/websockets.mddocs/source/run-workflows/observe/observe-workflow-with-weave.md
docs/**/*.{md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use deprecated names: Agent Intelligence toolkit, aiqtoolkit, AgentIQ, AIQ/aiq in documentation (unless intentionally referring to deprecated versions)
Files:
docs/source/reference/rest-api/api-server-endpoints.mddocs/source/reference/rest-api/websockets.mddocs/source/run-workflows/observe/observe-workflow-with-weave.md
docs/source/**/*.{md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.{md,rst}: Documentation must be clear and comprehensive, without TODOs, FIXMEs, or placeholder text
Ensure documentation is free of offensive or outdated terms
Ensure documentation is free of spelling mistakes and do not use words listed in ci/vale/styles/config/vocabularies/nat/reject.txt
Files:
docs/source/reference/rest-api/api-server-endpoints.mddocs/source/reference/rest-api/websockets.mddocs/source/run-workflows/observe/observe-workflow-with-weave.md
docs/source/**/*
⚙️ CodeRabbit configuration file
docs/source/**/*: This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in thedocs/source/_staticdirectory.Documentation Categories
Ensure documentation is placed in the correct category:
get-started/: Introductory documentation for new users
get-started/tutorials/: Step-by-step learning guidesbuild-workflows/: Workflow creation, configuration, adding remote MCP tools or A2A agents -run-workflows/: Execution, observability, serving workflows via MCP and A2A protocols -improve-workflows/: Evaluation and optimization guides -components/: Specific component implementations (agents, tools, connectors) -extend/: Custom component development and testing (not core library contributions) -reference/: Python and REST API documentation only -resources/: Project information (licensing, FAQs)
resources/contributing/: Development environment and contribution guidesPlacement rules:
- Component implementations always belong in
components/, notbuild-workflows/2. API documentation belongs only inreference/3. Using remote MCP tools or A2A agents should be placed inbuild-workflows/4. Serving workflows via MCP/A2A should be placed inrun-workflows/
Files:
docs/source/reference/rest-api/api-server-endpoints.mddocs/source/reference/rest-api/websockets.mddocs/source/run-workflows/observe/observe-workflow-with-weave.md
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should
contain a README.md or file README.ipynb.
- If an example contains Python code, it should be placed in a subdirectory named
src/and should
contain apyproject.tomlfile. Optionally, it might also contain scripts in ascripts/directory.- If an example contains YAML files, they should be placed in a subdirectory named
configs/. - If an example contains sample data files, they should be placed in a subdirectory nameddata/, and should
be checked into git-lfs.
Files:
examples/observability/simple_calculator_observability/configs/config-weave.yml
**/test_*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/test_*.py: Use pytest with pytest-asyncio for asynchronous code testing
Test functions should be named using the test_ prefix, using snake_case
Extract frequently repeated code into pytest fixtures, which should be named using the fixture_ prefix and define the name argument in the decorator
Mock external services with pytest_httpserver or unittest.mock instead of hitting live endpoints
Mark slow tests with @pytest.mark.slow so they can be skipped in the default test suite
Mark integration tests requiring external services with @pytest.mark.integration so they can be skipped in the default test suite
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
🧠 Learnings (3)
📚 Learning: 2025-11-20T10:27:43.583Z
Learnt from: thepatrickchin
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 781
File: src/nat/data_models/api_server.py:721-735
Timestamp: 2025-11-20T10:27:43.583Z
Learning: In src/nat/data_models/api_server.py, WebSocket message classes follow an established pattern for timestamp defaults using `timestamp: str = str(datetime.datetime.now(datetime.UTC))`. When adding new WebSocket message classes, maintain consistency with this pattern rather than introducing different approaches for individual classes.
Applied to files:
src/nat/data_models/api_server.py
📚 Learning: 2025-11-20T10:29:16.010Z
Learnt from: thepatrickchin
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 781
File: src/nat/front_ends/fastapi/message_validator.py:378-378
Timestamp: 2025-11-20T10:29:16.010Z
Learning: In src/nat/front_ends/fastapi/message_validator.py, all message creation functions (create_system_response_token_message, create_system_intermediate_step_message, create_system_interaction_message, create_observability_trace_message) use the pattern `message_id: str | None = str(uuid.uuid4())` for default message ID generation. When adding new message creation functions, maintain consistency with this established pattern rather than introducing different approaches for individual functions.
Applied to files:
src/nat/data_models/api_server.py
📚 Learning: 2025-12-12T20:49:44.305Z
Learnt from: zterek
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 1243
File: examples/risk_and_security/retail_agent/src/nat_retail_agent/configs/red-teaming.yml:1-98
Timestamp: 2025-12-12T20:49:44.305Z
Learning: In the NVIDIA/NeMo-Agent-Toolkit repository, YAML files generally use 2-space indentation. When reviewing YAML, prefer 2-space indentation to match the existing style over a 4-space guideline until a repo-wide standardization is performed. This applies to YAML configuration files (e.g., red-teaming.yml) and, more broadly, all *.yml files in the project.
Applied to files:
examples/observability/simple_calculator_observability/configs/config-weave.yml
🧬 Code graph analysis (3)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (3)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (2)
FastApiFrontEndPluginWorker(228-1431)config(128-129)src/nat/runtime/session.py (2)
SessionManager(126-607)shared_builder(227-228)src/nat/utils/type_utils.py (1)
override(56-57)
src/nat/data_models/api_server.py (2)
src/nat/builder/context.py (2)
observability_trace_id(229-233)conversation_id(191-198)packages/nvidia_nat_data_flywheel/tests/observability/processor/test_dfw_record_processor.py (1)
model_dump_json(336-338)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (1)
src/nat/builder/context.py (3)
observability_trace_id(229-233)get(119-120)get(339-349)
🪛 markdownlint-cli2 (0.18.1)
docs/source/run-workflows/observe/observe-workflow-with-weave.md
136-136: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
138-138: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 0
(MD005, list-indent)
147-147: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 0
(MD005, list-indent)
🪛 Ruff (0.14.8)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
89-90: Abstract raise to an inner function
(TRY301)
101-101: Consider moving this statement to an else block
(TRY300)
103-103: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
104-104: Use explicit conversion flag
Replace with conversion flag
(RUF010)
127-127: Do not catch blind exception: Exception
(BLE001)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
47-47: Unused lambda argument: args
(ARG005)
47-47: Unused lambda argument: kwargs
(ARG005)
🔇 Additional comments (21)
examples/observability/simple_calculator_observability/configs/config-weave.yml (1)
18-20: LGTM! Configuration correctly enables Weave FastAPI plugin worker.The front_end configuration properly references the
WeaveFastAPIPluginWorker, enabling the/feedbackendpoint for user reaction feedback when Weave telemetry is configured.docs/source/reference/rest-api/websockets.md (2)
40-40: LGTM! Message type correctly documented.The
observability_trace_messagetype is properly added to the list of possible WebSocket message types.
390-405: LGTM! Comprehensive documentation for observability trace messages.The new section clearly documents the System Observability Trace Message with a well-structured example that matches the
WebSocketObservabilityTraceMessagedata model added insrc/nat/data_models/api_server.py.docs/source/reference/rest-api/api-server-endpoints.md (1)
539-557: LGTM! Clear and comprehensive feedback endpoint documentation.The documentation properly describes the
/feedbackendpoint with practical examples and appropriate cross-references to the Weave observability guide. The note about availability withWeaveFastAPIPluginWorkerhelps users understand when this endpoint is accessible.docs/source/run-workflows/observe/observe-workflow-with-weave.md (1)
136-156: LGTM! Clear documentation for user feedback integration.The new section effectively documents how to enable the feedback endpoint when using Weave telemetry. The YAML configuration example is correct and the cross-reference to the API Server Endpoints documentation is helpful.
Note: The static analysis hints about indentation appear to be false positives from markdownlint misinterpreting the document structure. The markdown formatting is correct.
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (1)
176-181: LGTM! Correctly captures root observability trace ID.The logic properly addresses the previous review feedback by computing the root call from either the existing framework call (e.g., LangChain) or the newly created call, and setting
observability_trace_idonly when not already present. This ensures the root trace ID is captured regardless of nesting context.src/nat/builder/context.py (2)
75-75: LGTM! Context variable correctly added.The
observability_trace_idContextVar follows the established pattern for context state management and is appropriately typed asContextVar[str | None].
228-233: LGTM! Property correctly exposes observability trace ID.The property follows the established pattern for context accessors, with appropriate type hints and a clear docstring.
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (5)
31-55: LGTM! Well-structured test fixtures.The autouse fixtures properly set up the test environment and mock Weave dependencies to avoid authentication issues during unit testing.
58-79: LGTM! Comprehensive test for feedback endpoint with Weave configured.The test correctly verifies that the
/feedbackendpoint is registered when Weave telemetry is configured and handles both success (200) and initialization failure (500) scenarios appropriately.
82-98: LGTM! Correct test for endpoint absence without Weave.The test properly verifies that the
/feedbackendpoint returns 404 when Weave telemetry is not configured, ensuring conditional registration works as expected.
101-119: LGTM! Thorough parameter validation testing.The test correctly verifies that the
/feedbackendpoint validates required parameters (observability_trace_idandreaction_type) and returns 400 for missing fields.
122-134: LGTM! Verification of standard route preservation.The test ensures that
WeaveFastAPIPluginWorkerdoesn't break existing functionality by verifying that standard workflow endpoints like/generatecontinue to work correctly.src/nat/data_models/api_server.py (4)
497-507: LGTM! Clean implementation of observability trace response.The
ResponseObservabilityTraceclass correctly implements bothBaseModelandResponseSerializable, with proper field typing and a well-formattedget_stream_data()method for SSE streaming.
540-540: LGTM! Enum value correctly added.The
OBSERVABILITY_TRACE_MESSAGEtype is properly added to theWebSocketMessageTypeenum to support observability trace messaging.
716-718: LGTM! Content model correctly defined.
ObservabilityTraceContentusesextra="forbid"which is appropriate for content payload models, ensuring strict schema validation.
721-735: LGTM! WebSocket message follows established patterns.
WebSocketObservabilityTraceMessagecorrectly follows the established patterns for WebSocket message classes in this file, including the timestamp default pattern that is consistent across all 6 WebSocket message types. Based on learnings, this consistency is intentionally maintained.packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (4)
1-28: LGTM!License header and imports are correctly organized with proper SPDX Apache-2.0 header and current copyright year.
31-46: LGTM!Class docstring is well-structured with a clear description and practical YAML configuration example.
48-55: LGTM!The method properly delegates to the parent class before adding Weave-specific routes, maintaining correct initialization order.
79-104: Good implementation addressing past review concerns.The endpoint now correctly uses
SessionManager.sessionwith authentication and offloads blocking Weave calls viaasyncio.to_thread. The use oflogger.error()on line 103 is correct per coding guidelines when re-raising exceptions.Minor style improvement for the f-string (RUF010):
- raise HTTPException(status_code=500, detail=f"Failed to add feedback: {str(e)}") from e + raise HTTPException(status_code=500, detail=f"Failed to add feedback: {e!s}") from e
willkill07
left a comment
willkill07
left a comment
There was a problem hiding this comment.
Overall this looks good!
The only thing I'm struggling with is the long-term where someone also has a custom FastAPI worker class and also wants Weave feedback :\
But that's not a problem for you to solve :)
|
/ok to test 927ebfc |
Signed-off-by: Maryam Najafian <[email protected]>
Signed-off-by: Patrick Chin <[email protected]>
Signed-off-by: Patrick Chin <[email protected]>
- Auto-registers /feedback endpoint when Weave telemetry detected - Eliminates need for chat_feedback tool in workflow configs Signed-off-by: Patrick Chin <[email protected]>
…steps for streaming endpoints Signed-off-by: Patrick Chin <[email protected]>
Signed-off-by: Patrick Chin <[email protected]>
Signed-off-by: Patrick Chin <[email protected]>
Signed-off-by: Patrick Chin <[email protected]>
Signed-off-by: Patrick Chin <[email protected]>
- Create WeaveFastAPIPluginWorker in nvidia_nat_weave package - Remove hard-coded add_weave_feedback_route from core FastAPI worker - Make feedback endpoint opt-in via runner_class configuration Signed-off-by: Patrick Chin <[email protected]>
…f embedding in data chunks - Remove observability_trace_id from ChatResponse, ChatResponseChunk, ResponseIntermediateStep, and WebSocket message types - Detect and emit trace ID once when it becomes available - Keep API server data models agnostic to observability concerns Signed-off-by: Patrick Chin <[email protected]>
- Introduced `observability_trace_message` type to handle observability trace IDs separately. - Updated WebSocket message models to include a dedicated structure for observability trace messages. - Implemented logic to emit the observability trace ID once per workflow run, ensuring it is sent as a distinct message. - Adjusted message validation and handling to accommodate the new observability trace message format. Signed-off-by: Patrick Chin <[email protected]>
Signed-off-by: Patrick Chin <[email protected]>
- Wrap handler in SessionManager.session with http auth callback - Run blocking weave.init/get_call/add_reaction via asyncio.to_thread - Simplify telemetry exporter iteration and config access Signed-off-by: Patrick Chin <[email protected]> Signed-off-by: Patrick Chin <[email protected]>
…rker Signed-off-by: Patrick Chin <[email protected]>
Signed-off-by: Patrick Chin <[email protected]>
thepatrickchin
force-pushed
the
weave-feedback
branch
from
e71559a to
970df5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/nat/front_ends/fastapi/intermediate_steps_subscriber.py (1)
47-56: Consider storing the task reference for observability trace emission.The logic correctly emits the observability trace ID once when it becomes available. However, the task created at line 54 is not stored, which could theoretically lead to premature garbage collection (though unlikely given the quick
putoperation).Apply this diff to store the task reference:
- loop.create_task(_q.put(ResponseObservabilityTrace(observability_trace_id=observability_trace_id))) + task = loop.create_task(_q.put(ResponseObservabilityTrace(observability_trace_id=observability_trace_id)))This follows Python best practices for asyncio task creation, even though the risk is minimal in this specific case.
src/nat/front_ends/fastapi/message_validator.py (1)
375-403: Simplify exception logging in create_observability_trace_message.The method correctly follows established patterns for message creation. However, line 402 passes
str(e)tologger.exception(), which is redundant sinceexception()automatically captures the exception from context.Apply this diff:
except Exception as e: - logger.exception("Error creating observability trace message: %s", str(e)) + logger.exception("Error creating observability trace message") return NoneThis simplifies the logging call while still capturing the full stack trace.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/source/reference/rest-api/api-server-endpoints.md(1 hunks)docs/source/reference/rest-api/websockets.md(2 hunks)docs/source/run-workflows/observe/observe-workflow-with-weave.md(1 hunks)examples/observability/simple_calculator_observability/configs/config-weave.yml(1 hunks)packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py(1 hunks)packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py(1 hunks)packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py(1 hunks)src/nat/builder/context.py(2 hunks)src/nat/data_models/api_server.py(3 hunks)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py(5 hunks)src/nat/front_ends/fastapi/intermediate_steps_subscriber.py(2 hunks)src/nat/front_ends/fastapi/message_handler.py(7 hunks)src/nat/front_ends/fastapi/message_validator.py(5 hunks)tests/nat/server/test_unified_api_server.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/observability/simple_calculator_observability/configs/config-weave.yml
- docs/source/reference/rest-api/websockets.md
- docs/source/run-workflows/observe/observe-workflow-with-weave.md
- docs/source/reference/rest-api/api-server-endpoints.md
- src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
- tests/nat/server/test_unified_api_server.py
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{md,rst,py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use 'NVIDIA NeMo Agent toolkit' on first use, then 'NeMo Agent toolkit' for subsequent references
Files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,toml,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' for environment variable prefixes and informal comments
Files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 20 and PEP 8 for Python style guidelines
Run yapf second (PEP 8 base, column_limit = 120) for Python formatting
Use ruff check --fix (via pre-commit) for linting using configuration embedded in pyproject.toml, fix warnings unless explicitly ignored
Use snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
All public APIs require Python 3.11+ type hints on parameters and return values
Prefer collections.abc / typing abstractions (Sequence over list) for type hints
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Preserve stack traces and prevent duplicate logging when handling exceptions; use bare raise statements and logger.error() when re-raising
When catching and logging exceptions without re-raising, always use logger.exception() to capture the full stack trace information
Provide Google-style docstrings for every public module, class, function and CLI command
The first line of docstrings must be a concise description ending with a period (Vale checks this)
Surround code entities with backticks in docstrings to avoid Vale false-positives
Validate and sanitise all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorised operations whenever beneficial and feasible
Files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,js,ts,yaml,yml,json,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces, never tabs, and ensure every file ends with a single newline
Files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,env,toml,yaml,yml,json}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never commit API keys, credentials or personal data; use environment variables or .env files excluded from Git
Files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Every file must start with the standard SPDX Apache-2.0 header
Files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived automatically by setuptools-scm; never hard-code them in code or docs
Files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php,sh}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All source files must include the SPDX Apache-2.0 header template
Files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values (except for return values of
None,
in that situation no return type hint is needed).
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.pypackages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/builder/context.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
src/nat/**/*
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.pysrc/nat/front_ends/fastapi/intermediate_steps_subscriber.pysrc/nat/front_ends/fastapi/message_handler.pysrc/nat/builder/context.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.- When adding a new package, that new package name (as defined in the
pyproject.tomlfile) should
be added as a dependency to the nvidia-nat-all package inpackages/nvidia_nat_all/pyproject.toml
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pypackages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.pypackages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/test_*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/test_*.py: Use pytest with pytest-asyncio for asynchronous code testing
Test functions should be named using the test_ prefix, using snake_case
Extract frequently repeated code into pytest fixtures, which should be named using the fixture_ prefix and define the name argument in the decorator
Mock external services with pytest_httpserver or unittest.mock instead of hitting live endpoints
Mark slow tests with @pytest.mark.slow so they can be skipped in the default test suite
Mark integration tests requiring external services with @pytest.mark.integration so they can be skipped in the default test suite
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
🧠 Learnings (4)
📚 Learning: 2025-11-20T10:27:43.583Z
Learnt from: thepatrickchin
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 781
File: src/nat/data_models/api_server.py:721-735
Timestamp: 2025-11-20T10:27:43.583Z
Learning: In src/nat/data_models/api_server.py, WebSocket message classes follow an established pattern for timestamp defaults using `timestamp: str = str(datetime.datetime.now(datetime.UTC))`. When adding new WebSocket message classes, maintain consistency with this pattern rather than introducing different approaches for individual classes.
Applied to files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.pysrc/nat/front_ends/fastapi/message_handler.py
📚 Learning: 2025-11-20T10:29:16.010Z
Learnt from: thepatrickchin
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 781
File: src/nat/front_ends/fastapi/message_validator.py:378-378
Timestamp: 2025-11-20T10:29:16.010Z
Learning: In src/nat/front_ends/fastapi/message_validator.py, all message creation functions (create_system_response_token_message, create_system_intermediate_step_message, create_system_interaction_message, create_observability_trace_message) use the pattern `message_id: str | None = str(uuid.uuid4())` for default message ID generation. When adding new message creation functions, maintain consistency with this established pattern rather than introducing different approaches for individual functions.
Applied to files:
src/nat/data_models/api_server.pysrc/nat/front_ends/fastapi/message_validator.py
📚 Learning: 2025-11-24T18:56:53.109Z
Learnt from: CR
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-24T18:56:53.109Z
Learning: Applies to **/*.py : Provide Google-style docstrings for every public module, class, function and CLI command
Applied to files:
src/nat/front_ends/fastapi/message_validator.py
📚 Learning: 2025-11-24T18:56:53.109Z
Learnt from: CR
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-24T18:56:53.109Z
Learning: Applies to docs/source/**/*.{md,rst} : Documentation must be clear and comprehensive, without TODOs, FIXMEs, or placeholder text
Applied to files:
src/nat/front_ends/fastapi/message_validator.py
🧬 Code graph analysis (5)
src/nat/data_models/api_server.py (2)
src/nat/builder/context.py (2)
observability_trace_id(229-233)conversation_id(191-198)packages/nvidia_nat_data_flywheel/tests/observability/processor/test_dfw_record_processor.py (1)
model_dump_json(336-338)
src/nat/front_ends/fastapi/message_validator.py (2)
src/nat/data_models/api_server.py (4)
ObservabilityTraceContent(716-718)ResponseObservabilityTrace(497-506)WebSocketMessageType(531-541)WebSocketObservabilityTraceMessage(721-735)src/nat/builder/context.py (4)
get(119-120)get(339-349)observability_trace_id(229-233)conversation_id(191-198)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (1)
src/nat/builder/context.py (3)
observability_trace_id(229-233)get(119-120)get(339-349)
src/nat/front_ends/fastapi/intermediate_steps_subscriber.py (2)
src/nat/builder/context.py (1)
observability_trace_id(229-233)src/nat/data_models/api_server.py (1)
ResponseObservabilityTrace(497-506)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (6)
src/nat/data_models/api_server.py (1)
Request(50-68)src/nat/builder/workflow_builder.py (1)
WorkflowBuilder(307-1549)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (3)
FastApiFrontEndPluginWorker(228-1431)add_routes(329-343)config(128-129)src/nat/runtime/session.py (2)
SessionManager(126-607)shared_builder(227-228)src/nat/utils/type_utils.py (1)
override(56-57)src/nat/builder/context.py (1)
observability_trace_id(229-233)
🪛 Ruff (0.14.8)
src/nat/front_ends/fastapi/message_validator.py
402-402: Redundant exception object included in logging.exception call
(TRY401)
src/nat/front_ends/fastapi/intermediate_steps_subscriber.py
54-54: Store a reference to the return value of loop.create_task
(RUF006)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
89-90: Abstract raise to an inner function
(TRY301)
101-101: Consider moving this statement to an else block
(TRY300)
103-103: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
104-104: Use explicit conversion flag
Replace with conversion flag
(RUF010)
127-127: Do not catch blind exception: Exception
(BLE001)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
47-47: Unused lambda argument: args
(ARG005)
47-47: Unused lambda argument: kwargs
(ARG005)
🔇 Additional comments (19)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (1)
176-182: LGTM! Root call ID propagation now handles all cases.The fix properly addresses the past review concern by computing the root call from either the existing call (when attaching to another framework's trace) or the newly created call, and setting the observability_trace_id only once. This ensures the /feedback workflow receives the call ID regardless of the trace nesting scenario.
src/nat/builder/context.py (2)
75-75: LGTM! New context variable follows established patterns.The
observability_trace_idContextVar is correctly initialized and follows the naming and structure of other context variables in the class.
228-233: LGTM! Property implementation is clean and well-documented.The
observability_trace_idproperty correctly exposes the context variable with appropriate type hints and a concise Google-style docstring.packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (1)
31-134: LGTM! Comprehensive test coverage for Weave feedback integration.The test suite appropriately covers:
- Endpoint registration when Weave is configured
- Absence of endpoint when Weave is not configured
- Parameter validation (400 responses for missing fields)
- Preservation of standard routes
The mocking strategy correctly bypasses Weave authentication for unit tests.
src/nat/front_ends/fastapi/message_handler.py (5)
74-74: LGTM! Pending observability trace state properly initialized.The
_pending_observability_tracefield is correctly initialized to support deferred emission of the trace ID after the workflow completion message.
181-181: LGTM! State reset ensures clean handling of new workflows.Resetting
_pending_observability_tracewhen processing a new workflow request prevents stale trace IDs from being sent with subsequent responses.
257-262: LGTM! Observability trace message creation follows established patterns.The creation of
WebSocketObservabilityTraceMessagecorrectly delegates to the validator'screate_observability_trace_messagemethod, maintaining consistency with other message types.
348-351: LGTM! Observability trace correctly captured during streaming.The logic appropriately stores the first observability trace encountered and skips immediate emission using
continue, ensuring it will be sent after the completion message.
363-367: LGTM! Deferred observability trace emission is correctly implemented.After sending the completion message, any pending observability trace is properly emitted and the state is cleared, ensuring clients receive the trace ID for correlation.
src/nat/front_ends/fastapi/message_validator.py (3)
79-89: LGTM! Helper method reduces code duplication.The
_get_observability_trace_id_from_contextmethod appropriately centralizes the logic for retrieving the observability trace ID from context, with proper exception handling for import and attribute errors.
166-168: LGTM! Observability trace conversion is correctly implemented.The conversion from
ResponseObservabilityTracetoObservabilityTraceContentfollows the established pattern for other data model conversions in this method.
234-236: LGTM! Message type resolution correctly handles observability traces.The mapping of
ResponseObservabilityTracetoWebSocketMessageType.OBSERVABILITY_TRACE_MESSAGEis properly integrated into the resolution logic.src/nat/data_models/api_server.py (4)
497-507: LGTM! ResponseObservabilityTrace correctly implements streaming serialization.The class properly implements
ResponseSerializablewith aget_stream_datamethod that formats the observability trace for server-sent events, using theobservability_trace:prefix to distinguish it from standard data events.
540-540: LGTM! WebSocket message type enum extended correctly.The addition of
OBSERVABILITY_TRACE_MESSAGE = "observability_trace_message"follows the established naming convention for WebSocket message types.
716-719: LGTM! ObservabilityTraceContent data model is well-defined.The content model correctly uses
ConfigDict(extra="forbid")to ensure strict validation and contains the requiredobservability_trace_idfield.
721-736: LGTM! WebSocketObservabilityTraceMessage follows established patterns.The message class correctly follows the established WebSocket message structure with:
- Literal type discrimination
- Default values for id, parent_id, and conversation_id
- Timestamp default using the established pattern
- Appropriate docstring
- ConfigDict(extra="allow") for derived model support
Based on learnings, the timestamp default pattern is intentional and consistent across all WebSocket message classes.
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (3)
48-55: LGTM! Route registration correctly extends base worker.The override properly calls
super().add_routes()to preserve standard endpoints before adding Weave-specific routes.
60-69: LGTM! Weave configuration detection is straightforward.The loop correctly searches for
WeaveTelemetryExporterin the telemetry exporters, and gracefully returns early if Weave is not configured.
106-128: LGTM! Route registration includes appropriate metadata.The
/feedbackendpoint is correctly registered with:
- POST method
- Descriptive documentation
- Error response schema (500)
- Informational logging on success
- Warning logging on failure (appropriate use of broad exception handling for non-fatal setup)
Signed-off-by: Patrick Chin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (1)
57-69: Add return type hint and consider more robust type checking.Two improvements:
- The private method is missing a return type hint (
-> None).- Using
__class__.__name__string comparison at line 63 is fragile to refactoring and doesn't provide type safety. If importingWeaveTelemetryExporteris feasible, useisinstance(exporter_config.config, WeaveTelemetryExporter)instead.Apply this diff for the return type hint:
- async def _add_weave_feedback_route(self, app: FastAPI, builder: WorkflowBuilder): + async def _add_weave_feedback_route(self, app: FastAPI, builder: WorkflowBuilder) -> None: """Add the Weave feedback endpoint if Weave telemetry is configured."""If the
WeaveTelemetryExporterclass can be imported without circular dependencies, also apply:+from nat.plugins.weave.weave_telemetry_exporter import WeaveTelemetryExporter + ... for exporter_config in builder._telemetry_exporters.values(): - if exporter_config.config.__class__.__name__ == 'WeaveTelemetryExporter': + if isinstance(exporter_config.config, WeaveTelemetryExporter): weave_config = exporter_config.config break
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{md,rst,py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use 'NVIDIA NeMo Agent toolkit' on first use, then 'NeMo Agent toolkit' for subsequent references
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,toml,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' for environment variable prefixes and informal comments
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 20 and PEP 8 for Python style guidelines
Run yapf second (PEP 8 base, column_limit = 120) for Python formatting
Use ruff check --fix (via pre-commit) for linting using configuration embedded in pyproject.toml, fix warnings unless explicitly ignored
Use snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
All public APIs require Python 3.11+ type hints on parameters and return values
Prefer collections.abc / typing abstractions (Sequence over list) for type hints
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Preserve stack traces and prevent duplicate logging when handling exceptions; use bare raise statements and logger.error() when re-raising
When catching and logging exceptions without re-raising, always use logger.exception() to capture the full stack trace information
Provide Google-style docstrings for every public module, class, function and CLI command
The first line of docstrings must be a concise description ending with a period (Vale checks this)
Surround code entities with backticks in docstrings to avoid Vale false-positives
Validate and sanitise all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorised operations whenever beneficial and feasible
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,js,ts,yaml,yml,json,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces, never tabs, and ensure every file ends with a single newline
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,env,toml,yaml,yml,json}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never commit API keys, credentials or personal data; use environment variables or .env files excluded from Git
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Every file must start with the standard SPDX Apache-2.0 header
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived automatically by setuptools-scm; never hard-code them in code or docs
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php,sh}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All source files must include the SPDX Apache-2.0 header template
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values (except for return values of
None,
in that situation no return type hint is needed).
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.- When adding a new package, that new package name (as defined in the
pyproject.tomlfile) should
be added as a dependency to the nvidia-nat-all package inpackages/nvidia_nat_all/pyproject.toml
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
🧠 Learnings (1)
📚 Learning: 2025-11-24T18:56:53.109Z
Learnt from: CR
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-24T18:56:53.109Z
Learning: Applies to **/*.py : When catching and logging exceptions without re-raising, always use logger.exception() to capture the full stack trace information
Applied to files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
🧬 Code graph analysis (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (3)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (2)
add_routes(329-343)config(128-129)src/nat/runtime/session.py (2)
SessionManager(126-607)shared_builder(227-228)src/nat/utils/type_utils.py (1)
override(56-57)
🪛 Ruff (0.14.8)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
89-90: Abstract raise to an inner function
(TRY301)
101-101: Consider moving this statement to an else block
(TRY300)
103-103: Redundant exception object included in logging.exception call
(TRY401)
104-104: Use explicit conversion flag
Replace with conversion flag
(RUF010)
127-127: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (6)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (6)
1-14: LGTM: License header is correct.The SPDX Apache-2.0 header is present with the correct 2025 copyright year.
16-28: LGTM: Imports and logger setup follow conventions.The import ordering (standard library, third-party, local) and logger initialization are correct.
31-46: LGTM: Class definition and docstring are well-structured.The docstring follows Google style with a clear first line ending in a period, and includes a helpful usage example.
71-77: LGTM: Session manager and project configuration setup is correct.The session manager creation and Weave project name construction from entity/project configuration are implemented correctly.
106-125: LGTM: Route registration with proper metadata.The
/feedbackendpoint is correctly registered with appropriate path, methods, description, and error response documentation. The success log message is helpful for debugging.
127-128: LGTM: Appropriate exception handling for optional feature registration.The broad exception catch here is acceptable since this is registering an optional feedback endpoint during initialization. Logging a warning and continuing ensures that a failure here won't prevent the main application routes from functioning.
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
Show resolved
Hide resolved
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Patrick Chin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (2)
114-114: Use explicit conversion flag for cleaner f-string.Per static analysis (RUF010), prefer
{e!s}overstr(e)in f-strings for explicit string conversion.- raise HTTPException(status_code=500, detail=f"Failed to add feedback: {str(e)}") from e + raise HTTPException(status_code=500, detail=f"Failed to add feedback: {e!s}") from e
137-137: Graceful degradation is appropriate, but consider logging the exception traceback.Catching a broad
Exceptionhere (flagged by static analysis as BLE001) is acceptable for optional functionality that shouldn't prevent app startup. However, usinglogger.warningloses the traceback. Consider usinglogger.exceptionat debug level or includingexc_info=Trueto aid troubleshooting configuration issues.except Exception as e: - logger.warning("Failed to register Weave feedback endpoint: %s", e) + logger.warning("Failed to register Weave feedback endpoint: %s", e, exc_info=True)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{md,rst,py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use 'NVIDIA NeMo Agent toolkit' on first use, then 'NeMo Agent toolkit' for subsequent references
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,toml,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' for environment variable prefixes and informal comments
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 20 and PEP 8 for Python style guidelines
Run yapf second (PEP 8 base, column_limit = 120) for Python formatting
Use ruff check --fix (via pre-commit) for linting using configuration embedded in pyproject.toml, fix warnings unless explicitly ignored
Use snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
All public APIs require Python 3.11+ type hints on parameters and return values
Prefer collections.abc / typing abstractions (Sequence over list) for type hints
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Preserve stack traces and prevent duplicate logging when handling exceptions; use bare raise statements and logger.error() when re-raising
When catching and logging exceptions without re-raising, always use logger.exception() to capture the full stack trace information
Provide Google-style docstrings for every public module, class, function and CLI command
The first line of docstrings must be a concise description ending with a period (Vale checks this)
Surround code entities with backticks in docstrings to avoid Vale false-positives
Validate and sanitise all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorised operations whenever beneficial and feasible
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,js,ts,yaml,yml,json,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces, never tabs, and ensure every file ends with a single newline
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,env,toml,yaml,yml,json}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never commit API keys, credentials or personal data; use environment variables or .env files excluded from Git
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Every file must start with the standard SPDX Apache-2.0 header
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived automatically by setuptools-scm; never hard-code them in code or docs
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php,sh}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All source files must include the SPDX Apache-2.0 header template
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values (except for return values of
None,
in that situation no return type hint is needed).
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.- When adding a new package, that new package name (as defined in the
pyproject.tomlfile) should
be added as a dependency to the nvidia-nat-all package inpackages/nvidia_nat_all/pyproject.toml
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
🧬 Code graph analysis (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (2)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (3)
FastApiFrontEndPluginWorker(228-1431)add_routes(329-343)config(128-129)src/nat/runtime/session.py (2)
SessionManager(126-607)shared_builder(227-228)
🪛 Ruff (0.14.8)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py
114-114: Use explicit conversion flag
Replace with conversion flag
(RUF010)
137-137: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (3)
32-43: LGTM!The Pydantic models are well-structured with proper docstrings and type hints. Using typed models for request/response provides automatic validation and clear API documentation.
45-69: LGTM!The class structure is clean with a comprehensive docstring including usage example. The
add_routesmethod correctly calls the parent implementation first before adding Weave-specific routes, and the return type hint is properly specified.
101-106: Moveweave.init()outside the nested function to application startup.Logging is initialized globally, so
weave.init()is designed to be called once at application startup, not on every request. Calling it insideadd_weave_feedback()violates this pattern. Move the initialization outside the request handler (e.g., during route registration or application startup) and store the returned client or rely on the global initialization.
|
/ok to test 970df5f |
@willkill07, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 15a99ce |
Signed-off-by: Patrick Chin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{md,rst,py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use 'NVIDIA NeMo Agent toolkit' on first use, then 'NeMo Agent toolkit' for subsequent references
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,toml,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' for environment variable prefixes and informal comments
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 20 and PEP 8 for Python style guidelines
Run yapf second (PEP 8 base, column_limit = 120) for Python formatting
Use ruff check --fix (via pre-commit) for linting using configuration embedded in pyproject.toml, fix warnings unless explicitly ignored
Use snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
All public APIs require Python 3.11+ type hints on parameters and return values
Prefer collections.abc / typing abstractions (Sequence over list) for type hints
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Preserve stack traces and prevent duplicate logging when handling exceptions; use bare raise statements and logger.error() when re-raising
When catching and logging exceptions without re-raising, always use logger.exception() to capture the full stack trace information
Provide Google-style docstrings for every public module, class, function and CLI command
The first line of docstrings must be a concise description ending with a period (Vale checks this)
Surround code entities with backticks in docstrings to avoid Vale false-positives
Validate and sanitise all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorised operations whenever beneficial and feasible
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,js,ts,yaml,yml,json,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces, never tabs, and ensure every file ends with a single newline
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/test_*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/test_*.py: Use pytest with pytest-asyncio for asynchronous code testing
Test functions should be named using the test_ prefix, using snake_case
Extract frequently repeated code into pytest fixtures, which should be named using the fixture_ prefix and define the name argument in the decorator
Mock external services with pytest_httpserver or unittest.mock instead of hitting live endpoints
Mark slow tests with @pytest.mark.slow so they can be skipped in the default test suite
Mark integration tests requiring external services with @pytest.mark.integration so they can be skipped in the default test suite
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,env,toml,yaml,yml,json}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never commit API keys, credentials or personal data; use environment variables or .env files excluded from Git
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Every file must start with the standard SPDX Apache-2.0 header
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,md,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived automatically by setuptools-scm; never hard-code them in code or docs
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*.{py,js,ts,java,cpp,c,go,rb,php,sh}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All source files must include the SPDX Apache-2.0 header template
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values (except for return values of
None,
in that situation no return type hint is needed).
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.- When adding a new package, that new package name (as defined in the
pyproject.tomlfile) should
be added as a dependency to the nvidia-nat-all package inpackages/nvidia_nat_all/pyproject.toml
Files:
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
🧬 Code graph analysis (1)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (4)
src/nat/data_models/config.py (1)
GeneralConfig(202-269)src/nat/front_ends/fastapi/fastapi_front_end_config.py (1)
FastApiFrontEndConfig(151-293)packages/nvidia_nat_weave/src/nat/plugins/weave/fastapi_plugin_worker.py (1)
WeaveFastAPIPluginWorker(45-138)packages/nvidia_nat_weave/src/nat/plugins/weave/register.py (1)
WeaveTelemetryExporter(28-46)
🪛 Ruff (0.14.8)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py
47-47: Unused lambda argument: args
(ARG005)
47-47: Unused lambda argument: kwargs
(ARG005)
🔇 Additional comments (4)
packages/nvidia_nat_weave/tests/test_fastapi_plugin_worker.py (4)
1-28: LGTM!The SPDX header is correct, copyright year is current, and imports are appropriately organized.
31-38: LGTM!The fixture correctly sets up the environment variable with proper naming conventions, type hint, and docstring.
58-79: LGTM!The test correctly verifies that the
/feedbackendpoint is registered when Weave telemetry is configured. The acceptance of both 200 and 500 status codes is appropriate for a unit test focused on route registration rather than full integration behavior.
82-134: LGTM!The remaining test functions provide excellent coverage:
- Verification that
/feedbackendpoint is not registered without Weave configuration- Parameter validation testing for required fields
- Confirmation that standard routes continue to function correctly with the WeaveFastAPIPluginWorker
All tests follow pytest conventions with proper naming, docstrings, and async/await usage.
|
/ok to test 45857c1 |
|
/merge |
4 participants
Description
This PR enables user reaction feedback for AI assistant responses through Weave observability. Users just need to enable Weave observability – there is no additional configuration needed.
/feedbackPOST endpoint/feedbackwithweave_call_idandreaction_typeSee corresponding NAT-UI PR for frontend implementation NVIDIA/NeMo-Agent-Toolkit-UI#42
Closes #759
By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.