Include input and output messages in weave observability traces #1050
Conversation
Signed-off-by: Patrick Chin <[email protected]>
…ing chat endpoint Signed-off-by: Patrick Chin <[email protected]>
…abled Signed-off-by: Patrick Chin <[email protected]>
… streaming endpoints Signed-off-by: Patrick Chin <[email protected]>
WalkthroughEnriches workflow START/END events with StreamEventData (input/output/preview), restores Weave exporter handling for websocket-style inputs and varied response formats, and adds a truncate_string utility for concise previews. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Runner
participant WeaveExporter as Weave Exporter
participant Weave as Weave Backend
rect rgb(248,249,255)
Note over Runner: Workflow start
Runner->>Runner: Emit WORKFLOW_START with data: StreamEventData(input=...)
Runner->>WeaveExporter: _create_weave_call(payload with StreamEventData)
end
rect rgb(255,250,240)
Note over WeaveExporter: Input extraction
WeaveExporter->>WeaveExporter: Detect websocket `messages` or fallback input
WeaveExporter->>Weave: Register call including `input_message`
end
rect rgb(248,255,245)
Note over Runner: Workflow end / streaming
Runner->>Runner: Collect streaming outputs (preview up to 50)
Runner->>Runner: Emit WORKFLOW_END with data: StreamEventData(output=..., preview=...)
Runner->>WeaveExporter: _finish_weave_call(payload with StreamEventData)
end
rect rgb(255,250,240)
Note over WeaveExporter: Output extraction
WeaveExporter->>WeaveExporter: _extract_output_message handles choices/delta/value
WeaveExporter->>Weave: Update call with `output`, `output_message`, `output_preview`
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
Signed-off-by: Patrick Chin <[email protected]>
thepatrickchin
force-pushed
the
weave-bugfix
branch
from
1ada9b9 to
fe5b6f2
Compare
Signed-off-by: Patrick Chin <[email protected]>
Signed-off-by: Patrick Chin <[email protected]>
thepatrickchin
force-pushed
the
weave-bugfix
branch
from
64403c5 to
01644fa
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: 1
🧹 Nitpick comments (5)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (3)
152-166: Harden input_message extraction for dict-shaped messages and avoid attribute-only access.Current logic assumes attribute access and may miss dict-based schemas. Add dict handling and safer extraction.
- # Extract message content if input has messages attribute - # When websocket mode is enabled, the message is located at messages[0].content[0].text, - messages = getattr(step.payload.data.input, 'messages', []) - if messages: - content = messages[0].content - if isinstance(content, list) and content: - inputs["input_message"] = getattr(content[0], 'text', content[0]) - else: - inputs["input_message"] = content + # Extract message content for websocket/NAT-UI schemas. + raw_input = step.payload.data.input + # Support both object-style and dict-style inputs + messages = getattr(raw_input, "messages", None) + if messages is None and isinstance(raw_input, dict): + messages = raw_input.get("messages", []) + if messages: + first = messages[0] + # content can be attribute, key, list, or plain string + content = getattr(first, "content", first.get("content") if isinstance(first, dict) else first) + if isinstance(content, list) and content: + head = content[0] + # prefer 'text' then 'content' then raw + if isinstance(head, dict): + inputs["input_message"] = head.get("text") or head.get("content") or str(head) + else: + inputs["input_message"] = getattr(head, "text", head) + else: + inputs["input_message"] = content
190-224: Make output extraction resilient: support dicts, strings, and empty/variant choices.Handle common OpenAI-like dicts, list-of-dicts, and plain string outputs; guard indices and fallbacks.
- def _extract_output_message(self, output_data: Any, outputs: dict[str, Any]) -> None: + def _extract_output_message(self, output_data: Any, outputs: dict[str, Any]) -> None: """ Extract message content from various response formats and add to outputs dictionary. Args: output_data: The raw output data from the response outputs: Dictionary to populate with extracted message content """ - # Handle direct "choices" attribute (non-streaming: output.choices[0].message.content) - choices = getattr(output_data, 'choices', None) - if choices: - outputs["output_message"] = choices[0].message.content - return + # Fast-path: plain string output + if isinstance(output_data, str): + outputs["output_message"] = output_data + return + + # Normalize accessor helpers + def _getattr_or_key(obj: Any, name: str, default: Any = None) -> Any: + if isinstance(obj, dict): + return obj.get(name, default) + return getattr(obj, name, default) + + # Handle direct "choices" (non-streaming) + choices = _getattr_or_key(output_data, "choices") + if choices: + first_choice = choices[0] if len(choices) > 0 else None + if first_choice: + message = _getattr_or_key(first_choice, "message") + if message: + outputs["output_message"] = _getattr_or_key(message, "content") + return + delta = _getattr_or_key(first_choice, "delta") + if delta: + outputs["output_preview"] = _getattr_or_key(delta, "content") + return - # Handle list-based output (streaming or websocket) – content may be in the following formats: + # Handle list-based output (streaming or websocket) - content may be in the following formats: # output[0].choices[0].message.content # output[0].choices[0].delta.content # output[0].value if not isinstance(output_data, list) or not output_data: return - choices = getattr(output_data[0], 'choices', None) + first = output_data[0] + choices = _getattr_or_key(first, "choices") if choices: - message = getattr(choices[0], 'message', None) - delta = getattr(choices[0], 'delta', None) + first_choice = choices[0] if len(choices) > 0 else None + message = _getattr_or_key(first_choice, "message") if first_choice else None + delta = _getattr_or_key(first_choice, "delta") if first_choice else None if message: - outputs["output_message"] = getattr(message, 'content', None) + outputs["output_message"] = _getattr_or_key(message, "content") elif delta: - outputs["output_preview"] = getattr(delta, 'content', None) + outputs["output_preview"] = _getattr_or_key(delta, "content") else: - value = getattr(output_data[0], 'value', None) + value = _getattr_or_key(first, "value") if value: outputs["output_preview"] = value + else: + # As a last resort, if the element itself is a string-like content + if isinstance(first, str): + outputs["output_message"] = first
152-166: Optional: add configurable redaction/truncation for PII and very large messages.To reduce risk and payload size, consider redacting or truncating input/output message fields via an opt-in env var (e.g.,
NAT_OBSERVABILITY_REDACT_MESSAGES=1) and/or a max length (e.g., 2–4k chars).I can draft a minimal redaction helper and wiring if you want.
Also applies to: 190-224
src/nat/runtime/runner.py (2)
254-261: Name a constant for preview size and annotate the list for clarity.Improves readability and easy tuning without code changes.
- # Collect preview of streaming results for the WORKFLOW_END event - output_preview = [] + # Collect preview of streaming results for the WORKFLOW_END event + output_preview: list[typing.Any] = [] @@ - if len(output_preview) < 50: + if len(output_preview) < OUTPUT_PREVIEW_LIMIT: output_preview.append(m)Add this constant near the top of the file (outside this hunk):
# How many streaming items to include in the workflow END preview OUTPUT_PREVIEW_LIMIT = 50
173-175: Code changes are correct; consider extracting preview limit to a named constant with type annotation.The START/END event payload changes are sound. Verification confirms all exporters (Weave, span exporter, property adapter) safely guard
payload.datafield access with null checks, so unknown fields pose no compatibility risk.However, the original review's suggestions remain unimplemented:
- Line 257: Extract hardcoded
50to a named constant (e.g.,MAX_STREAMING_OUTPUT_PREVIEW = 50)- Line 257: Add type annotation to
output_preview(e.g.,output_preview: list[Any])These improve maintainability and code clarity.
📜 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/weave_exporter.py(4 hunks)src/nat/runtime/runner.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/runtime/runner.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
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; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/runtime/runner.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/runtime/runner.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:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/runtime/runner.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
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ (or packages//src/)
Files:
src/nat/runtime/runner.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Changes in src/nat should prioritize backward compatibility
Files:
src/nat/runtime/runner.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/runtime/runner.py
🧬 Code graph analysis (2)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (1)
src/nat/data_models/intermediate_step.py (1)
data(293-294)
src/nat/runtime/runner.py (3)
src/nat/data_models/intermediate_step.py (2)
data(293-294)StreamEventData(67-77)src/nat/builder/function.py (3)
astream(207-208)astream(211-212)astream(215-260)tests/nat/runtime/test_runner_trace_ids.py (1)
astream(40-43)
🪛 Ruff (0.14.1)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
204-204: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
🔇 Additional comments (3)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (2)
19-19: LGTM: import use is appropriate.Importing
Anyis justified for mixed Weave payload shapes.
245-245: LGTM: populating derived output fields in Weave outputs.Calling
_extract_output_messagehere aligns the UI with root-trace visibility.Please run your streaming and non-streaming smoke tests to confirm
outputs["output_message"]oroutputs["output_preview"]always exist for the common schemas you support.src/nat/runtime/runner.py (1)
272-276: END event withStreamEventData(output=output_preview)aligns with exporter’s preview logic.Good; the exporter derives
output_message/output_previewfrom list-shaped outputs.
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Patrick Chin <[email protected]>
thepatrickchin
force-pushed
the
weave-bugfix
branch
from
d1914e5 to
5768269
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (1)
205-205: Fix ruff RUF003: replace EN DASH with hyphen.This issue was flagged in a previous review but remains unresolved.
- # Handle list-based output (streaming or websocket) – content may be in the following formats: + # Handle list-based output (streaming or websocket) - content may be in the following formats:
🧹 Nitpick comments (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (1)
158-166: Consider usinggetattrfor safer attribute access.While the surrounding try-except block (line 154) will catch any
AttributeError, explicitly usinggetattrwould make the code more defensive and clearer.# Extract message content if input has messages attribute # When websocket mode is enabled, the message is located at messages[0].content[0].text, messages = getattr(step.payload.data.input, 'messages', []) if messages: - content = messages[0].content + content = getattr(messages[0], 'content', None) - if isinstance(content, list) and content: + if content and isinstance(content, list): inputs["input_message"] = getattr(content[0], 'text', content[0]) - else: + elif content: inputs["input_message"] = content
📜 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/weave_exporter.py(4 hunks)src/nat/utils/string_utils.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/utils/string_utils.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
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; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/utils/string_utils.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/utils/string_utils.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:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.pysrc/nat/utils/string_utils.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
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ (or packages//src/)
Files:
src/nat/utils/string_utils.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Changes in src/nat should prioritize backward compatibility
Files:
src/nat/utils/string_utils.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/utils/string_utils.py
🧬 Code graph analysis (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (3)
src/nat/data_models/intermediate_step.py (2)
IntermediateStep(235-310)data(293-294)src/nat/utils/string_utils.py (1)
truncate_string(41-54)src/nat/builder/context.py (1)
output(59-60)
🪛 Ruff (0.14.1)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
205-205: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
🔇 Additional comments (4)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (4)
19-19: LGTM!The new imports are properly used in the
_extract_output_messagemethod.Also applies to: 26-26
246-246: LGTM!The call to
_extract_output_messageis correctly placed within the try-except block and properly populates the outputs dictionary with extracted message content.
212-220: Add length check before accessingchoices[0].Similar to the earlier case,
choices[0]is accessed without verifying the list is non-empty.choices = getattr(output_data[0], 'choices', None) - if choices: + if choices and len(choices) > 0: message = getattr(choices[0], 'message', None) delta = getattr(choices[0], 'delta', None) if message: outputs["output_message"] = truncate_string(getattr(message, 'content', None)) elif delta: outputs["output_preview"] = truncate_string(getattr(delta, 'content', None))Likely an incorrect or invalid review comment.
191-204: Add length check before accessingchoices[0].The code accesses
choices[0]without verifying the list is non-empty, which could raise anIndexError.def _extract_output_message(self, output_data: Any, outputs: dict[str, Any]) -> None: """ Extract message content from various response formats and add to outputs dictionary. Args: output_data: The raw output data from the response outputs: Dictionary to populate with extracted message content """ # Handle direct "choices" attribute (non-streaming: output.choices[0].message.content) choices = getattr(output_data, 'choices', None) - if choices: + if choices and len(choices) > 0: outputs["output_message"] = truncate_string(choices[0].message.content) returnLikely an incorrect or invalid review comment.
willkill07
added
improvement
willkill07
changed the title
|
/ok to test 5768269 |
willkill07
left a comment
willkill07
left a comment
There was a problem hiding this comment.
Approving with additional documentation suggestion.
Thank you for your contribution!
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
Outdated
Show resolved
Hide resolved
|
Something seems weird to me here:
From top-to-bottom:
Note: i'm just doing raw curl requests |
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
Outdated
Show resolved
Hide resolved
…tput_message Signed-off-by: Patrick Chin <[email protected]>
Signed-off-by: Patrick Chin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (1)
200-248: Comprehensive output extraction with good documentation.The method handles multiple output formats with clear documentation, addressing past review feedback about documenting supported formats. The early returns and safety checks are well-structured.
Optional: Consider consistent type handling for
truncate_string.Line 248 uses
str(value)before callingtruncate_string, but lines 218, 224, 236, and 242 pass values directly. While the current implementation works due to the surrounding try-except block, explicitly ensuring string types would improve consistency and type safety.For example:
outputs["output_message"] = truncate_string(str(choices[0].message.content))This is a minor refinement and can be deferred since exceptions are already caught at the call site.
📜 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/weave_exporter.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
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; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.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:
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
🧬 Code graph analysis (1)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (3)
src/nat/data_models/intermediate_step.py (2)
IntermediateStep(235-310)data(293-294)src/nat/observability/exporter/span_exporter.py (1)
SpanExporter(47-308)src/nat/utils/string_utils.py (1)
truncate_string(41-54)
🔇 Additional comments (3)
packages/nvidia_nat_weave/src/nat/plugins/weave/weave_exporter.py (3)
19-19: LGTM! Appropriate imports for the new functionality.The
Anytype andtruncate_stringutility are correctly imported and used in the new message extraction methods.Also applies to: 26-26
182-198: Well-structured input extraction.The method correctly handles both direct content and websocket mode with appropriate safety checks for empty lists. The implementation addresses the past review feedback to refactor input extraction into a dedicated method.
157-157: LGTM! Proper integration of message extraction.Both calls are appropriately placed within try-except blocks and enhance the trace data after capturing the raw input/output. This ensures the root traces display input and output messages as intended by the PR objectives.
Also applies to: 270-270
In some cases the input message already appears in the original dictionary without need for extraction so I left that as-is. I think users are unlikely to be switching between completion endpoints and websocket schema so normally the additional column wouldn't appear in the table. As for the outputs of the |
Signed-off-by: Patrick Chin <[email protected]>
thepatrickchin
force-pushed
the
weave-bugfix
branch
from
cb8d1c9 to
0e80471
Compare
|
/merge |
2 participants
Description
This PR ensures the input message and output message are displayed in the root observability trace in Weave when workflows are executed using either
nat runornat servewith NAT-UI.With
nat run:With
nat serve/chat/generate/chat/stream:For streaming endpoints, a preview of the first few tokens is collected for display in Weave
/generate/streamWebsocket Schemas
chatchat_streamgenerategenerate_streamCloses #1041
By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Improvements