Ensure error reporting and propagating in a consistent pattern #722
Conversation
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
yczhang-nv
added
improvement
WalkthroughStandardizes exception logging and re-raise patterns across the repo (use bare Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant ReWOO_Agent as ReWOO Agent
participant Planner
participant Executor
participant Tool
participant Solver
User->>ReWOO_Agent: prompt
ReWOO_Agent->>Planner: plan(prompt)
Planner-->>ReWOO_Agent: plan steps
loop For each step
ReWOO_Agent->>Executor: execute(step)
Executor->>Tool: call(tool, inputs)
Tool-->>Executor: tool_response (raw)
Note right of Executor #DFF0D8: New: store raw tool_response\nin intermediate_results (no ToolMessage wrapper)
Executor-->>ReWOO_Agent: intermediate_results updated
end
ReWOO_Agent->>Solver: solve(prompt, intermediate_results)
Solver-->>ReWOO_Agent: final answer
ReWOO_Agent-->>User: answer
sequenceDiagram
autonumber
participant AgentBase as Agent Base
participant Tool
AgentBase->>Tool: invoke tool
alt Tool returns dict
Tool-->>AgentBase: dict
Note right of AgentBase #FFF7D6: AgentBase wraps dict into [dict]\nToolMessage.content becomes [dict]
else Tool returns str/list
Tool-->>AgentBase: str or list
end
alt Tool error
AgentBase->>AgentBase: logger.exception(...)\nreturn error ToolMessage (content string)
end
sequenceDiagram
autonumber
participant Caller
participant Retriever
Caller->>Retriever: retrieve(query)
Retriever-->>Caller: results or error
alt RetrieverError and raise_errors=True
Retriever->>Retriever: logger.error("Retriever threw an error: %s.", e)
Retriever-->>Caller: re-raise (original traceback)
else RetrieverError and raise_errors=False
Retriever->>Retriever: logger.exception("Retriever threw an error: %s. Returning an empty response.", e)
Retriever-->>Caller: RetrieverOutput(results=[])
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (22)
examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.py (2)
205-211: Fix potential ZeroDivisionError and bogus secondary axis when tokens/sec unavailable.If all token counts or TPS are zero,
scaling_factorbecomes 0, andax.get_ylim()[1] / scaling_factorwill crash. Guard and omit the secondary axis when not meaningful.Use:
- max_token_count = max(*(prompt_tokens or [0]), *(completion_tokens or [0]), *(total_tokens or [0])) - max_tps = max(tokens_per_second or [0]) - scaling_factor = max_token_count / max(max_tps, 1) * 0.8 # Scale tokens/sec to be visible but not dominate - - scaled_tokens_per_second = [tps * scaling_factor for tps in tokens_per_second] + max_token_count = max(*(prompt_tokens or [0]), *(completion_tokens or [0]), *(total_tokens or [0])) + max_tps = max(tokens_per_second or [0]) + bars4 = None + if max_token_count > 0 and max_tps > 0: + scaling_factor = (max_token_count / max_tps) * 0.8 + scaled_tokens_per_second = [tps * scaling_factor for tps in tokens_per_second] + else: + scaling_factor = None + scaled_tokens_per_second = [0] * len(tokens_per_second) @@ - bars4 = ax.bar(x + width * 1.5, scaled_tokens_per_second, width, label="Tokens/Sec", color="red", alpha=0.7) + if any(v > 0 for v in scaled_tokens_per_second): + bars4 = ax.bar(x + width * 1.5, scaled_tokens_per_second, width, label="Tokens/Sec", color="red", alpha=0.7) @@ - add_labels(bars4, tokens_per_second, "{:.2f}") + if bars4 is not None: + add_labels(bars4, tokens_per_second, "{:.2f}") @@ - ax2 = ax.twinx() - ax2.set_ylabel("Tokens per Second") - # Set y-limits based on the scaling - ax2.set_ylim(0, ax.get_ylim()[1] / scaling_factor) + if bars4 is not None and scaling_factor: + ax2 = ax.twinx() + ax2.set_ylabel("Tokens per Second") + ax2.set_ylim(0, ax.get_ylim()[1] / scaling_factor)Also applies to: 241-242, 250-255
89-96: Validate required columns before use to avoid KeyError.
df.groupby("context.trace_id")anddf["span_kind"]assume columns exist. Add a check and fail fast with a clear error.@@ - # Group by trace_id + # Validate required columns + required_cols = {"context.trace_id", "span_kind", "start_time", "end_time"} + missing = sorted(required_cols - set(df.columns)) + if missing: + raise ValueError(f"Missing required columns in dataframe: {', '.join(missing)}") + # Group by trace_idAlso applies to: 126-134
src/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.py (1)
81-87: Key mismatch: FunctionRef used as dict key but later looked up by string name.
function_map[fn_ref](FunctionRef key) vsfunction_map[item.name](str key) will never match; lookups will fail.Apply:
- function_map = {} - for fn_ref in config.augmented_fns: - # Retrieve the actual function from the builder - fn_obj = builder.get_function(fn_ref) - function_map[fn_ref] = fn_obj + function_map: dict[str, object] = {} + for fn_ref in config.augmented_fns: + # Retrieve the actual function from the builder + fn_obj = builder.get_function(fn_ref) + function_map[str(fn_ref)] = fn_obj @@ - fn_description = ("\n".join(f"- **{fn_ref}**: {function_map[fn_ref].description or 'No description provided.'}" - for fn_ref in config.augmented_fns)) + fn_description = ("\n".join(f"- **{fn_ref}**: {function_map[str(fn_ref)].description or 'No description provided.'}" + for fn_ref in config.augmented_fns)) @@ - if item.name not in function_map: + if item.name not in function_map: ... - else: - fn = function_map[item.name] + else: + fn = function_map[item.name]Also applies to: 155-162, 119-121
src/nat/eval/swe_bench_evaluator/evaluate.py (1)
90-91: Return type mismatch: function returns (None, None) but annotation forbids None.This will trip pyright (treated as error per repo policy). Allow Optional paths.
- def process_eval_input(self, eval_input: EvalInput) -> tuple[Path, Path]: + def process_eval_input(self, eval_input: EvalInput) -> tuple[Path | None, Path | None]:src/nat/tool/code_execution/local_sandbox/local_sandbox_server.py (1)
1-14: Add standard SPDX header per repo policy.This file uses a custom header; repository requires SPDX lines at top.
-# Copyright (c) 2024-2025, NVIDIA CORPORATION. All rights reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. +# SPDX-FileCopyrightText: Copyright (c) 2024-2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0src/nat/utils/io/yaml_tools.py (1)
79-81: Replace asserts with explicit validation errors.Asserts can be stripped with -O and will hide validation. Prefer explicit exceptions.
- assert isinstance(interpolated_config_str, str), "Config must be a string" + if not isinstance(interpolated_config_str, str): + raise TypeError("Config must be a string") @@ - assert isinstance(config_data, dict) + if not isinstance(config_data, dict): + raise ValueError("Top-level YAML must be a mapping (dictionary).")Also applies to: 91-93
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (2)
730-740: Fix log label (“evaluation job”) and drop redundante.This path is for generation jobs.
- except Exception as e: - logger.exception("Error in evaluation job %s: %s", job_id, e) + except Exception: + logger.exception("Error in generation job %s", job_id) job_store.update_status(job_id, "failure", error=str(e))
169-188: Log filter leak: removeFilter called with a new instance, not the one added.A different
LogFilterinstance is removed, leaving the original installed and suppressing logs indefinitely.- logging.getLogger("uvicorn.access").addFilter(LogFilter(logs_to_suppress)) + access_logger = logging.getLogger("uvicorn.access") + lf = LogFilter(logs_to_suppress) + access_logger.addFilter(lf) try: response = await call_next(request) finally: - logging.getLogger("uvicorn.access").removeFilter(LogFilter(logs_to_suppress)) + access_logger.removeFilter(lf)src/nat/registry_handlers/rest/rest_handler.py (1)
149-160: Standardize exception logging and avoid duplicates.In except blocks where you don’t re-raise, use a single
logger.exception(msg); drop separatelogger.error(...)andexc_info=True.@@ - except Exception as e: + except Exception as e: msg = f"Error occured when installing packages: {e}" - logger.error(msg) + logger.exception(msg) if (os.path.exists(tmp_dir)): shutil.rmtree(tmp_dir) @@ - validated_pull_response = PullResponse(status={ - "status": StatusEnum.ERROR, "message": msg, "action": ActionEnum.PULL - }) - logger.exception(validated_pull_response.status.message) + validated_pull_response = PullResponse(status={ + "status": StatusEnum.ERROR, "message": msg, "action": ActionEnum.PULL + }) yield validated_pull_response @@ - logger.exception(validated_publish_response.status.message, exc_info=True) + logger.exception(validated_publish_response.status.message) @@ - logger.exception(validated_search_response.status.message, exc_info=True) + logger.exception(validated_search_response.status.message) @@ - logger.exception(validated_remove_response.status.message, exc_info=True) + logger.exception(validated_remove_response.status.message)Also applies to: 86-93, 188-198, 226-233
examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/flow_chart.py (1)
133-198: Close Matplotlib figure and save from the fig object to prevent memory leaks.Repeated calls will leak figures; prefer fig.savefig and close the figure.
- _, ax = plt.subplots(figsize=(14, 10)) + fig, ax = plt.subplots(figsize=(14, 10)) @@ - plt.tight_layout() + fig.tight_layout() @@ - plt.savefig(buf, format="png") + fig.savefig(buf, format="png") @@ - with open(image_path, "wb") as f: - f.write(buf.getvalue()) + with open(image_path, "wb") as f: + f.write(buf.getvalue()) + buf.close() + plt.close(fig)examples/advanced_agents/alert_triage_agent/src/nat_alert_triage_agent/hardware_check_tool.py (1)
55-61: Avoid passing passwords on the command line (leaks via process list).Use an env var or password file supported by the tool; avoid including secrets in argv.
- command = ["ipmimonitoring", "-h", ip_address, "-u", username, "-p", password, "--privilege-level=USER"] + command = ["ipmimonitoring", "-h", ip_address, "-u", username, "--privilege-level=USER"] @@ - result = subprocess.run(command, capture_output=True, text=True, check=True) + child_env = {**os.environ, "IPMI_PASSWORD": password} + result = subprocess.run(command, capture_output=True, text=True, check=True, env=child_env)Add missing import near the top of the file:
import ossrc/nat/observability/exporter/base_exporter.py (1)
269-276: Align on_error with PR policy: use logger.exception when not re-raising.Ensures traceback is captured once at this boundary.
- logger.error("Error in event subscription: %s", exc, exc_info=True) + logger.exception("Error in event subscription: %s", exc)src/nat/agent/react_agent/agent.py (1)
124-133: Don’t log traceback when re-raising; use logger.error instead of logger.exception.- except Exception as ex: - logger.exception("%s Unable to find tool with the name %s\n%s", - AGENT_LOG_PREFIX, - tool_name, - ex, - exc_info=True) - raise + except Exception as ex: + logger.error("%s Unable to find tool with the name %s: %s", + AGENT_LOG_PREFIX, + tool_name, + ex) + raisesrc/nat/front_ends/fastapi/message_validator.py (3)
233-271: Fix default arguments that capture time/UUID and a mutable model at import time.str(uuid.uuid4()), datetime.now(...), and SystemResponseContent() as defaults freeze values across calls and the model is mutable. Generate them inside.
Apply:
- async def create_system_response_token_message( + 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()), + message_id: str | None = None, 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.timezone.utc)) + timestamp: str | None = None ) -> WebSocketSystemResponseTokenMessage | None: @@ - try: + try: + if message_id is None: + message_id = str(uuid.uuid4()) + if timestamp is None: + timestamp = str(datetime.datetime.now(datetime.timezone.utc)) + if content is None: + content = SystemResponseContent() return WebSocketSystemResponseTokenMessage(type=message_type,
273-311: Same default-arguments issue in create_system_intermediate_step_message.Apply:
- async def create_system_intermediate_step_message( + 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()), + message_id: str | None = None, 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.timezone.utc)) + timestamp: str | None = None ) -> WebSocketSystemIntermediateStepMessage | None: @@ - try: + try: + if message_id is None: + message_id = str(uuid.uuid4()) + if timestamp is None: + timestamp = str(datetime.datetime.now(datetime.timezone.utc)) + if content is None: + content = SystemIntermediateStepContent(name="default", payload="default") return WebSocketSystemIntermediateStepMessage(type=message_type,
312-351: Same default-arguments issue in create_system_interaction_message (message_id, timestamp).Apply:
- async def create_system_interaction_message( + async def create_system_interaction_message( self, *, message_type: Literal[WebSocketMessageType.SYSTEM_INTERACTION_MESSAGE] = ( WebSocketMessageType.SYSTEM_INTERACTION_MESSAGE), - message_id: str | None = str(uuid.uuid4()), + message_id: str | None = None, thread_id: str = "default", parent_id: str = "default", conversation_id: str | None = None, content: HumanPrompt, status: WebSocketMessageStatus = WebSocketMessageStatus.IN_PROGRESS, - timestamp: str = str(datetime.datetime.now(datetime.timezone.utc)) + timestamp: str | None = None ) -> WebSocketSystemInteractionMessage | None: @@ - try: + try: + if message_id is None: + message_id = str(uuid.uuid4()) + if timestamp is None: + timestamp = str(datetime.datetime.now(datetime.timezone.utc)) return WebSocketSystemInteractionMessage(type=message_type,examples/advanced_agents/alert_triage_agent/src/nat_alert_triage_agent/telemetry_metrics_host_performance_check_tool.py (1)
145-153: Avoid blocking I/O in async function: replace requests with httpx.AsyncClient.requests.get blocks the event loop inside async _arun; switch to async HTTP client.
Apply:
-import requests +import httpx- response = requests.get(url, params=params, timeout=30) - response.raise_for_status() - data = response.json() + async with httpx.AsyncClient(timeout=30.0) as client: + resp = await client.get(url, params=params) + resp.raise_for_status() + data = resp.json()If httpx isn’t available in examples’ deps, I can open a follow-up PR to update it.
Also applies to: 22-24
src/nat/agent/tool_calling_agent/agent.py (1)
158-165: Same: avoid duplicate tracebacks in build_graph error path.Use
logger.errorthen bareraise.Apply:
- except Exception as ex: - logger.exception( - "%s Failed to build Tool Calling Agent Graph: %s", - AGENT_LOG_PREFIX, - ex, - exc_info=ex, - ) - raise + except Exception as ex: + logger.error("%s Failed to build Tool Calling Agent Graph: %s", AGENT_LOG_PREFIX, ex) + raisesrc/nat/agent/rewoo_agent/agent.py (4)
95-104: Avoid duplicate tracebacks; simplify dead try/except in _get_tool.
dict.get()won’t raise; theexceptpluslogger.exception(...); raiseviolates the PR guideline (log error, then re-raise elsewhere) and is dead code here. Uselogger.errorand return the resolved tool (possibly None).- def _get_tool(self, tool_name: str): - try: - return self.tools_dict.get(tool_name) - except Exception as ex: - logger.exception("%s Unable to find tool with the name %s\n%s", - AGENT_LOG_PREFIX, - tool_name, - ex, - exc_info=True) - raise + def _get_tool(self, tool_name: str): + tool = self.tools_dict.get(tool_name) + if tool is None: + logger.error("%s Unable to find tool with the name %s", AGENT_LOG_PREFIX, tool_name) + return tool
237-246: Breaking change: intermediate_results now holds raw tool outputs, but code still dereferences .content.Storing
tool_responsedirectly (Line 275) means the loop must no longer assumeToolMessage. Current.contentderef will raise at runtime. Normalize values to support both legacyToolMessageand raw outputs.- # Replace the placeholder in the tool input with the previous tool output - for _placeholder, _tool_output in intermediate_results.items(): - _tool_output = _tool_output.content - # If the content is a list, get the first element which should be a dict - if isinstance(_tool_output, list): - _tool_output = _tool_output[0] - assert isinstance(_tool_output, dict) - - tool_input = self._replace_placeholder(_placeholder, tool_input, _tool_output) + # Replace the placeholder in the tool input with the previous tool output + for _placeholder, _tool_output in intermediate_results.items(): + # Normalize: ToolMessage -> content; otherwise use value as-is + if isinstance(_tool_output, ToolMessage): + value = _tool_output.content + else: + value = _tool_output + # If value is a list and first element is a dict, prefer that shape + if isinstance(value, list) and value and isinstance(value[0], dict): + value = value[0] + tool_input = self._replace_placeholder(_placeholder, tool_input, value)Outside-this-hunk follow-ups for consistency (optional but recommended):
- Update ReWOOGraphState type to reflect raw outputs:
from typing import Any ... intermediate_results: dict[str, Any] = Field(default_factory=dict)- Store a plain string/dict for the “tool not found” case instead of
ToolMessage(...)to keep shapes uniform:intermediate_results[placeholder] = TOOL_NOT_FOUND_ERROR_MESSAGE.format( tool_name=tool, tools=configured_tool_names )Also applies to: 275-276
293-305: Same shape mismatch in solver_node: remove.contentderef and normalize values.Without this, any raw tool output will break here too.
- for step in state.steps.content: + for step in state.steps.content: step_info = step["evidence"] placeholder = step_info.get("placeholder", "") tool_input = step_info.get("tool_input", "") intermediate_results = state.intermediate_results - for _placeholder, _tool_output in intermediate_results.items(): - _tool_output = _tool_output.content - # If the content is a list, get the first element which should be a dict - if isinstance(_tool_output, list): - _tool_output = _tool_output[0] - assert isinstance(_tool_output, dict) - - tool_input = self._replace_placeholder(_placeholder, tool_input, _tool_output) - - placeholder = placeholder.replace(_placeholder, str(_tool_output)) + for _placeholder, _tool_output in intermediate_results.items(): + if isinstance(_tool_output, ToolMessage): + value = _tool_output.content + else: + value = _tool_output + if isinstance(value, list) and value and isinstance(value[0], dict): + value = value[0] + tool_input = self._replace_placeholder(_placeholder, tool_input, value) + placeholder = placeholder.replace(_placeholder, str(value)) _plan = step.get("plan") tool = step_info.get("tool") plan += f"Plan: {_plan}\n{placeholder} = {tool}[{tool_input}]"
408-411: Remove unreachable duplicate returns in validate_solver_prompt.Multiple
return Truestatements are dead code; keep a single return.- return True - return True - return True - return True + return True
🧹 Nitpick comments (61)
packages/nvidia_nat_redis/src/nat/plugins/redis/schema.py (7)
56-56: Consider lowering verbosity for schema construction logs.“Created embedding field …” at INFO is potentially noisy for normal runs; DEBUG is more appropriate.
- logger.info("Created embedding field with dimension %d", embedding_dim) + logger.debug("Created embedding field with dimension %d", embedding_dim)
66-69: Use DEBUG for per-field schema dumps.Field-by-field dumps at INFO can clutter logs; keep the header at INFO (or DEBUG) and details at DEBUG.
- logger.info("Schema fields:") + logger.debug("Schema fields:") @@ - logger.info(" - %s: %s", field.name, type(field).__name__) + logger.debug(" - %s: %s", field.name, type(field).__name__)
32-41: Add return type hints for public API.Per guidelines, annotate return type. The schema shape is fixed: TextField, TagField, TextField, VectorField.
-def create_schema(embedding_dim: int = DEFAULT_DIM): +def create_schema(embedding_dim: int = DEFAULT_DIM) -> tuple[TextField, TagField, TextField, VectorField]:
92-97: Normalize error matching and keep traceback policy consistent.
- Mixed-case check for “Index needs recreation” can miss variants; normalize to lower-case like the other clause.
- You’re re-raising (good); keep using logger.error here to avoid duplicate stack traces elsewhere.
- error_msg = str(ex) - if "no such index" not in error_msg.lower() and "Index needs recreation" not in error_msg: + error_msg = str(ex) + lowered = error_msg.lower() + if "no such index" not in lowered and "index needs recreation" not in lowered: logger.error("Unexpected Redis error: %s", error_msg) raise
103-109: Include traceback when swallowing non-fatal drop failures.When not re-raising, prefer emitting the traceback. Either use logger.exception (ERROR level) or warning with exc_info=True. Given PR objective, use logger.exception.
- except redis_exceptions.ResponseError as e: - if "no such index" not in str(e).lower(): - logger.warning("Error while dropping index: %s", str(e)) + except redis_exceptions.ResponseError as e: + if "no such index" not in str(e).lower(): + logger.exception("Error while dropping index: %s", e)
112-113: Deduplicate “created schema” log.This repeats the earlier message; drop it or reduce to DEBUG to avoid noise.
- logger.info("Created schema with embedding dimension: %d", embedding_dim or DEFAULT_DIM) + logger.debug("Created schema with embedding dimension: %d", embedding_dim or DEFAULT_DIM)
130-136: Minor: avoid str(e) in logger.*; logging formats exception cleanly.Also keep consistent parameterized form.
- except redis_exceptions.ResponseError as e: - logger.error("Failed to create index: %s", str(e)) + except redis_exceptions.ResponseError as e: + logger.error("Failed to create index: %s", e) raise - except redis_exceptions.ConnectionError as e: - logger.error("Redis connection error while creating index: %s", str(e)) + except redis_exceptions.ConnectionError as e: + logger.error("Redis connection error while creating index: %s", e) raisepackages/nvidia_nat_redis/src/nat/plugins/redis/redis_editor.py (6)
33-36: Docstring brand usage: prefer “nat” (lowercase) per style guide.Use “nat interfaces” instead of “NAT interfaces” in documentation/docstrings.
- Wrapper class that implements NAT interfaces for Redis memory storage. + Wrapper class that implements nat interfaces for Redis memory storage.
116-121: Remove TODO in comment or link a tracked issue.Guidelines prohibit TODOs in code comments. Either reword or reference an issue ID.
- user_id = kwargs.get("user_id", "redis") # TODO: remove this fallback username + # Fallback username used when caller does not supply user_id. + user_id = kwargs.get("user_id", "redis")I can open a follow-up to make user_id an explicit, typed parameter on search without breaking callers.
150-154: Redundant DEBUG log lines can be consolidated.Consider merging the two query debug lines to reduce noise.
- logger.debug("Search query parameters: vec length=%d", len(query_vector_bytes)) - # Log the actual query being executed - logger.debug("Full search query: %s", search_query.query_string()) + logger.debug("Full search query: %s | vec length=%d", + search_query.query_string(), len(query_vector_bytes))
172-187: Use float formatting for similarity scores.Scores are typically floats; %d truncates. Also avoid dumping full memory_data dict unless needed.
- logger.debug("Similarity score: %d", getattr(doc, 'score', 0)) - logger.debug("Extracted data for result %d: %s", i + 1, memory_data) + logger.debug("Similarity score: %.4f", float(getattr(doc, 'score', 0.0))) + logger.debug("Extracted keys for result %d: %s", i + 1, list(memory_data.keys()))
52-55: API typing: prefer Sequence over list for inputs.Broaden accepted inputs without sacrificing type safety.
- async def add_items(self, items: list[MemoryItem]) -> None: + async def add_items(self, items: 'Sequence[MemoryItem]') -> None:Add at top-level imports:
+from collections.abc import Sequence
219-234: Add explicit return type and parameter typing for kwargs.Return type should be None; consider typing filter kwargs as Mapping[str, Any] for clarity.
- async def remove_items(self, **kwargs): + async def remove_items(self, **kwargs) -> None:Optionally:
+from typing import Any, Mapping @@ - async def remove_items(self, **kwargs) -> None: + async def remove_items(self, **kwargs: Any) -> None:packages/nvidia_nat_agno/src/nat/plugins/agno/tool_wrapper.py (4)
289-289: Include exception type in the error message for quicker triage.Adding the exception class improves signal without reprinting the traceback.
- logger.error("Error executing Agno tool %s: %s", name, e) + logger.error("Error executing Agno tool %s (%s): %s", name, e.__class__.__name__, e)
134-139: Tighten type hints: return type is str; prefer Sequence[str] for required_fields.
process_resultalways returnsstr, soexecute_agno_toolreturnsstr. Also preferSequence[str]per guidelines.-def execute_agno_tool(name: str, - coroutine_fn: Callable[..., Awaitable[Any]], - required_fields: List[str], - loop: asyncio.AbstractEventLoop, - **kwargs: Any) -> Any: +def execute_agno_tool(name: str, + coroutine_fn: Callable[..., Awaitable[Any]], + required_fields: Sequence[str], + loop: asyncio.AbstractEventLoop, + **kwargs: Any) -> str:And update imports:
-from typing import List +from typing import Sequence
296-301: Docstring naming: use “nat” (not “NAT”) in documentation.Per repo style, use “nat” in docs; “NAT” is reserved for env vars and informal code comments.
- Wraps a NAT Function to be usable as an Agno tool. + Wraps a nat Function to be usable as an Agno tool. @@ - This wrapper handles the conversion of async NAT functions to + This wrapper handles the conversion of async nat functions to @@ - fn : Function - The NAT Function to wrap + fn : Function + The nat Function to wrapAlso applies to: 306-309
274-283: Avoid brittle TypeError string matching for call-style fallback.Matching on the exact error text (
"missing 1 required positional argument: 'input_obj'") is fragile. Consider inspectingcoroutine_fn’s signature to decide whether to pass a single positional dict or kwargs.Example approach:
import inspect sig = inspect.signature(coroutine_fn) params = list(sig.parameters.values()) expects_single_mapping = ( len(params) == 1 and params[0].kind in (inspect.Parameter.POSITIONAL_ONLY, inspect.Parameter.POSITIONAL_OR_KEYWORD) ) if expects_single_mapping: future = asyncio.run_coroutine_threadsafe(coroutine_fn(filtered_kwargs), loop) else: future = asyncio.run_coroutine_threadsafe(coroutine_fn(**filtered_kwargs), loop)examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.py (1)
54-55: Consider Path over str for filesystem paths.Using
Pathimproves clarity and portability.- df_path: str = Field(..., description="Path to the CSV file containing the dataframe") + df_path: str = Field(..., description="Path to the CSV file containing the dataframe") # consider pathlib.Pathsrc/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.py (1)
151-153: Prefer parameterized logger.exception; avoid embeddingein the message.Prevents redundant text and defers formatting.
- except Exception as e: - logger.exception(f"Error invoking function '{item.name}': {e}") + except Exception: + logger.exception("Error invoking function '%s'", item.name)examples/notebooks/retail_sales_agent/src/nat_retail_sales_agent/data_visualization_agent.py (1)
82-95: Prefer logger.exception when not re-raising.Here you recover (no re-raise) and route; include the traceback once at this layer for better diagnostics.
- except Exception as ex: - logger.error("Error in conditional_edge: %s", ex) + except Exception as ex: + logger.exception("Error in conditional_edge: %s", ex) if hasattr(state, 'retry_count') and state.retry_count >= config.max_retries: logger.warning("Max retries reached, returning without meaningful output") return "__end__" state.retry_count = getattr(state, 'retry_count', 0) + 1 logger.warning( "Error in the conditional edge: %s, retrying %d times out of %d", ex, state.retry_count, config.max_retries, ) return "data_visualization_agent"src/nat/eval/rag_evaluator/register.py (1)
106-108: Minor logging micro-optimization.Avoid pre-building the string; let the logger handle formatting.
- message = f"Ragas metric {metric_name} not found {e}." - logger.exception(message) + logger.exception("Ragas metric %s not found %s.", metric_name, e) return Noneexamples/advanced_agents/alert_triage_agent/src/nat_alert_triage_agent/network_connectivity_check_tool.py (2)
115-118: Correct re-raise pattern; add host context to the log.Bare
raise+logger.erroris correct per PR goals. Consider addinghost_idfor easier triage.- utils.logger.error("Error during connectivity check: %s", str(e)) + utils.logger.error("Error during connectivity check for host %s: %s", host_id, e)
31-38: Typo in config field description ("model" → "mode").Minor doc fix; appears in multiple tools.
- offline_mode: bool = Field(default=True, description="Whether to run in offline model") + offline_mode: bool = Field(default=True, description="Whether to run in offline mode")src/nat/eval/rag_evaluator/evaluate.py (1)
170-173: Redundant exc_info with logger.exception.
logger.exceptionalready setsexc_info=True. Safe to drop for cleaner logs. Functional no-op.- logger.exception("Error evaluating ragas metric, Error: %s", e, exc_info=True) + logger.exception("Error evaluating ragas metric, Error: %s", e)src/nat/eval/swe_bench_evaluator/evaluate.py (2)
67-69: Docstring typo.“Temorary” → “Temporary”.
- """ Temorary function to move the report and logs to the output directory""" + """Temporary function to move the report and logs to the output directory."""
85-88: Store unsupported repos deterministically.Appending
{repo, version}creates a set; prefer a tuple or dict for stable logging/serialization.- self._unsupported_repos.append({repo, version}) + self._unsupported_repos.append({"repo": repo, "version": version})examples/advanced_agents/alert_triage_agent/src/nat_alert_triage_agent/monitoring_process_check_tool.py (3)
117-120: Correct re-raise pattern; add host context to the log.Matches PR guidance. Add
host_idfor context.- utils.logger.error("Error during monitoring process check: %s", str(e)) + utils.logger.error("Error during monitoring process check for host %s: %s", host_id, e)
105-114: Duplicate header log call.
utils.log_header("LLM Reasoning", ...)appears twice; drop the second or change to footer.- utils.log_header("LLM Reasoning", dash_length=50) - utils.logger.debug(conclusion) + utils.logger.debug(conclusion)
29-36: Typo in config field description ("model" → "mode").Same nit as in other tool config.
- offline_mode: bool = Field(default=True, description="Whether to run in offline model") + offline_mode: bool = Field(default=True, description="Whether to run in offline mode")src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (3)
212-219: Drop redundant%s,ein logger.exception.
logger.exceptionalready includes the exception message/traceback; passingeduplicates it.- except Exception as e: - logger.exception("Error during %s job cleanup: %s", name, e) + except Exception: + logger.exception("Error during %s job cleanup", name)
288-306: Use logger.exception without embeddingstr(e).Avoid duplicating the exception text in the log message.
- except Exception as e: - logger.exception("Error in evaluation job %s: %s", job_id, str(e)) + except Exception: + logger.exception("Error in evaluation job %s", job_id) job_store.update_status(job_id, "failure", error=str(e))
171-174: Typo: “supreses” → “suppresses”.Minor docstring fix.
- Intercepts authentication request and supreses logs that contain sensitive data. + Intercepts authentication requests and suppresses logs that contain sensitive data.examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/flow_chart.py (2)
121-129: Avoid SettingWithCopy warnings when mutating grouped DataFrames.GroupBy slices can be views; copy before adding columns to be safe.
- df = df.sort_values("start_time") + df = df.sort_values("start_time").copy()
139-147: Remove unused y_positions or use it to draw connections.Dead code increases maintenance overhead.
examples/advanced_agents/alert_triage_agent/src/nat_alert_triage_agent/hardware_check_tool.py (1)
37-53: Add type hints to internal helper for clarity and tooling.Improves readability and static analysis.
-def _get_ipmi_monitor_data(ip_address, username, password): +def _get_ipmi_monitor_data(ip_address: str, username: str, password: str) -> str | None:src/nat/registry_handlers/package_utils.py (1)
45-51: Fix typos in comments.Minor cleanups improve professionalism.
- # will reading a file set of vun scan? + # Will reading this file trigger a vuln scan? @@ - # return firs module name + # Return first module namesrc/nat/front_ends/fastapi/step_adaptor.py (1)
316-318: Optional: drop “%s”, e — logger.exception already logs the exception and traceback.Avoids redundant message content.
- logger.exception("Error processing intermediate step: %s", e) + logger.exception("Error processing intermediate step")src/nat/front_ends/fastapi/response_helpers.py (2)
101-104: Re-raise style is correct; add logger.error for consistency with PR objective.Per PR guidance, when re-raising, log with logger.error (no traceback) in this layer. Currently no log is emitted here.
Apply this diff in the except block:
- except Exception: - # Handle exceptions here - raise + except Exception as ex: + logger.error("generate_streaming_response failed: %s", ex) + raiseAdd a module logger (outside this range):
import logging logger = logging.getLogger(__name__)
168-171: Same re-raise logging consistency for generate_streaming_response_full.Match the PR standard by logging with logger.error before bare raise.
- except Exception: - # Handle exceptions here - raise + except Exception as ex: + logger.error("generate_streaming_response_full failed: %s", ex) + raiseexamples/advanced_agents/profiler_agent/src/nat_profiler_agent/agent.py (1)
119-121: Avoid duplicate tracebacks: use logger.error when re-raising.These blocks log with logger.exception and then re-raise, likely causing duplicate stack traces upstream. Switch to logger.error per PR objective.
- logger.exception("Failed to build ProfilerAgent Graph: %s", ex, exc_info=True) + logger.error("Failed to build ProfilerAgent Graph: %s", ex) raise ... - logger.exception("Failed to call agent_node: %s", ex, exc_info=True) + logger.error("Failed to call agent_node: %s", ex) raise ... - logger.exception("Failed to call executor_node: %s", ex, exc_info=True) + logger.error("Failed to call executor_node: %s", ex) raise ... - logger.exception("Failed to call response_composer_node: %s", ex, exc_info=True) + logger.error("Failed to call response_composer_node: %s", ex) raiseAlso applies to: 145-147, 160-162, 173-175
src/nat/front_ends/fastapi/fastapi_front_end_controller.py (1)
28-30: Typo in docstring.“spawing” → “spawning”.
- _FastApiFrontEndController class controls the spawing and tear down of the API server in environments where + _FastApiFrontEndController class controls the spawning and tear down of the API server in environments wheresrc/nat/tool/retriever.py (2)
39-42: Field description is out of sync with behavior (mentions “warnings”).Either lower the log level to warning or update the description to reflect error/exception logging. Recommend updating the text for clarity.
- raise_errors: bool = Field( - default=True, - description="If true the tool will raise exceptions, otherwise it will log them as warnings and return []", - ) + raise_errors: bool = Field( + default=True, + description="If true, the tool re-raises exceptions; otherwise, it logs the exception and returns an empty result.", + )
35-37: Typo in docstring (“implementaiton”).- configuration uses clients, which are the vectorstore-specific implementaiton of the retriever interface. + configuration uses clients, which are the vectorstore-specific implementation of the retriever interface.src/nat/front_ends/fastapi/message_handler.py (1)
240-246: Fix typos in error log message.“vaidation” → “validation”; “ocurred” → “occurred”.
- logger.exception("A data vaidation error ocurred creating websocket message: %s", str(e)) + logger.exception("A data validation error occurred creating websocket message: %s", str(e))src/nat/front_ends/fastapi/message_validator.py (4)
100-105: Prefer not passing the exception object/message to logger.exception (traceback already included).This avoids redundant “: … Exception: ” logging and keeps messages concise.
Apply:
- logger.exception("A data validation error %s occurred for message: %s", str(e), str(message)) + logger.exception("A data validation error occurred for message: %s", str(message)) - logger.exception("Error retrieving schema for message type '%s': %s", message_type, str(e)) + logger.exception("Error retrieving schema for message type '%s'", message_type) - logger.exception("Input data could not be converted to validated message content: %s", str(e)) + logger.exception("Input data could not be converted to validated message content") - logger.exception("Error human response content not found: %s", str(e)) + logger.exception("Error human response content not found") - logger.exception("Error type not found converting data to validated websocket message content: %s", str(e)) + logger.exception("Error type not found converting data to validated websocket message content") - logger.exception("Error creating system response token message: %s", str(e)) + logger.exception("Error creating system response token message") - logger.exception("Error creating system intermediate step message: %s", str(e)) + logger.exception("Error creating system intermediate step message") - logger.exception("Error creating system interaction message: %s", str(e)) + logger.exception("Error creating system interaction message")Also applies to: 121-124, 158-161, 193-196, 220-223, 269-271, 308-311, 348-351
224-232: Docstring vs. return mismatch ("default" vs "root").Docstring says “default” but code returns "root".
Apply one of:
- Update docstring to “root”, or
- Change return to default for consistency:
- :return: Intermediate step parent_id or "default". + :return: Intermediate step parent_id or "root".Or:
- return data_model.parent_id or "root" + return data_model.parent_id or "default"
133-141: Type hint: allow None for validated_message_content.Variable is initialized to None but annotated as BaseModel only.
Apply:
- validated_message_content: BaseModel = None + validated_message_content: BaseModel | None = None
73-74: Remove unused field _message_parent_id if truly unused.Keeps the class lean.
src/nat/eval/remote_workflow.py (2)
77-79: Trim redundant exception arg in logger.exception calls.logger.exception already records the traceback; no need to pass e/str(e).
Apply:
- logger.exception("Failed to parse generate response chunk: %s", e) + logger.exception("Failed to parse generate response chunk") - logger.exception("Failed to parse intermediate step: %s", e) + logger.exception("Failed to parse intermediate step") - logger.exception("Request failed for question %s: %s", question, e) + logger.exception("Request failed for question %s", question)Also applies to: 93-95, 98-101
82-91: Shadowing ‘payload’ harms readability.The loop-level payload variable hides the earlier request payload variable.
Apply:
- payload = IntermediateStepPayload.model_validate_json(response_intermediate.payload) - intermediate_step = IntermediateStep(parent_id="remote", - function_ancestry=InvocationNode( - function_name=payload.name or "remote_function", - function_id=payload.UUID or "remote_function_id"), - payload=payload) + step_payload = IntermediateStepPayload.model_validate_json(response_intermediate.payload) + intermediate_step = IntermediateStep( + parent_id="remote", + function_ancestry=InvocationNode( + function_name=step_payload.name or "remote_function", + function_id=step_payload.UUID or "remote_function_id", + ), + payload=step_payload, + )src/nat/observability/processor/batching_processor.py (2)
196-197: Trim redundant exception arg in logger.exception calls.logger.exception already includes traceback; remove e/str(e) for signal-to-noise.
Apply:
- logger.exception("Error routing scheduled batch through pipeline: %s", e) + logger.exception("Error routing scheduled batch through pipeline") - logger.exception("Error in scheduled flush: %s", e) + logger.exception("Error in scheduled flush") - logger.exception("Error routing final batch through pipeline during shutdown: %s", e) + logger.exception("Error routing final batch through pipeline during shutdown") - logger.exception("Error during BatchingProcessor shutdown: %s", e) + logger.exception("Error during BatchingProcessor shutdown")Also applies to: 203-204, 274-275, 286-287
107-109: _done appears unused. Remove or wire it.Avoid dead state.
Apply:
- # Callback for immediate export of scheduled batches - self._done = None + # (removed) unusedexamples/advanced_agents/alert_triage_agent/src/nat_alert_triage_agent/telemetry_metrics_host_performance_check_tool.py (1)
105-111: Optional: make timestamps timezone-aware for consistency.Use UTC explicitly to avoid ambiguity in downstream parsing.
Apply:
- data = [[datetime.fromtimestamp(entry[0]).strftime("%Y-%m-%d %H:%M:%S"), entry[1]] + data = [[datetime.fromtimestamp(entry[0], tz=UTC).strftime("%Y-%m-%d %H:%M:%S"), entry[1]] for entry in timestamp_value_list]src/nat/observability/exporter_manager.py (1)
180-181: Trim redundant exception arg in logger.exception calls.Keep logs concise; traceback is already included.
Apply:
- logger.exception("Error preparing cleanup for isolated exporter '%s': %s", name, e) + logger.exception("Error preparing cleanup for isolated exporter '%s'", name) - logger.exception("Error stopping isolated exporter '%s': %s", name, e) + logger.exception("Error stopping isolated exporter '%s'", name) - logger.exception("Error during isolated exporter cleanup: %s", e) + logger.exception("Error during isolated exporter cleanup") - logger.exception("Failed to stop exporter '%s': %s", name, str(e)) + logger.exception("Failed to stop exporter '%s'", name)Also applies to: 198-199, 254-255, 310-311
src/nat/builder/workflow_builder.py (2)
423-425: Use plain raise + logger.error here is correct; consider adding wrapper info and exception type.Including
wrapper_typeandtype(e).__name__makes triage faster without adding stack traces.Apply:
- logger.error("Error fetching tool `%s`: %s", fn_name, e) + logger.error("Error fetching tool `%s` (wrapper=%s): %s: %s", + fn_name, wrapper_type, type(e).__name__, e)
820-820: Optional: include exception type in summary line.Keeps logs concise while adding useful context.
Apply:
- logger.error("Original error: %s", original_error) + logger.error("Original error (%s): %s", type(original_error).__name__, original_error)src/nat/observability/mixin/file_mixin.py (2)
106-107: Use logger.exception without appending the exception string.
logger.exceptionalready logs the stack and message; the trailing “: %s”, e is redundant noise.Apply:
- logger.exception("Error removing existing file %s: %s", self._current_file_path, e) + logger.exception("Error removing existing file: %s", self._current_file_path) @@ - logger.exception("Error removing old file %s: %s", old_file, e) + logger.exception("Error removing old file: %s", old_file) @@ - logger.exception("Error during initialization cleanup: %s", e) + logger.exception("Error during initialization cleanup")Also applies to: 157-161
194-195: Same redundancy with logger.exception across these sites.Tighten messages; keep context args, drop explicit “: %s”, e.
Apply:
- logger.exception("Error rolling file %s: %s", self._current_file_path, e) + logger.exception("Error rolling file: %s", self._current_file_path) @@ - logger.exception("Error removing old file %s: %s", old_file, e) + logger.exception("Error removing old file: %s", old_file) @@ - logger.exception("Error during cleanup: %s", e) + logger.exception("Error during cleanup") @@ - logger.exception("Error exporting event: %s", e) + logger.exception("Error exporting event")Also applies to: 212-216, 251-252
src/nat/builder/function.py (2)
189-197: Avoid dumping args/kwargs in logs; they can be large or sensitive.Keep schema context; omit raw values.
Apply:
- except Exception: - logger.error( - "Error in acall_invoke() converting input to function schema. Both args and kwargs were " - "supplied which could not be converted to the input schema. args: %s\nkwargs: %s\nschema: %s", - args, - kwargs, - self.input_schema) - raise + except Exception: + logger.error( + "acall_invoke() failed to convert input to schema=%s when both args and kwargs were supplied", + self.input_schema) + raise
290-298: Avoid logging raw args/kwargs for acall_stream as well.Apply:
- except Exception: - logger.error( - "Error in acall_stream() converting input to function schema. Both args and kwargs were " - "supplied which could not be converted to the input schema. args: %s\nkwargs: %s\nschema: %s", - args, - kwargs, - self.input_schema) - raise + except Exception: + logger.error( + "acall_stream() failed to convert input to schema=%s when both args and kwargs were supplied", + self.input_schema) + raisesrc/nat/observability/exporter/processing_exporter.py (1)
178-179: Tighten logger.exception messages (avoid explicit “: %s”, e).
logger.exceptionalready includes the exception message and traceback; remove redundant%s, e.Apply:
- logger.exception("Error in processor %s: %s", processor.__class__.__name__, e) + logger.exception("Error in processor %s", processor.__class__.__name__) @@ - logger.exception("Source processor %s not found in pipeline", source_processor.__class__.__name__) + logger.exception("Source processor %s not found in pipeline", source_processor.__class__.__name__) @@ - logger.exception("Failed to continue pipeline processing after %s: %s", - source_processor.__class__.__name__, - e) + logger.exception("Failed to continue pipeline processing after %s", + source_processor.__class__.__name__) @@ - logger.exception("Error shutting down processors: %s", e) + logger.exception("Error shutting down processors")Also applies to: 217-218, 228-231, 318-319
examples/advanced_agents/profiler_agent/src/nat_profiler_agent/register.py
Show resolved
Hide resolved
examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/flow_chart.py
Show resolved
Hide resolved
examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.py
Show resolved
Hide resolved
examples/notebooks/retail_sales_agent/src/nat_retail_sales_agent/data_visualization_agent.py
Show resolved
Hide resolved
src/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/nat/agent/test_rewoo.py (2)
242-244: Strengthen assertion: validate list shape and payload, not just typeAssert the list has one dict element and key details match the input to prevent false positives.
await mock_rewoo_agent.executor_node(mock_state) - # The actual behavior is that dict input is converted to list and stored as list content in ToolMessage - assert isinstance(mock_state.intermediate_results["#E1"].content, list) + # The actual behavior is that dict input is converted to a list and stored as list content in ToolMessage + assert isinstance(mock_state.intermediate_results["#E1"].content, list) + assert len(mock_state.intermediate_results["#E1"].content) == 1 + assert isinstance(mock_state.intermediate_results["#E1"].content[0], dict) + assert mock_state.intermediate_results["#E1"].content[0]["query"]["data"] == "This is a dict query" + assert mock_state.intermediate_results["#E1"].content[0]["query"]["metadata"]["key"] == "value"
246-247: Also assert content schema for E2 and placeholder resolutionVerify the dict-in-list structure and that "#E1" was substituted with E1’s output.
# Call executor node again to make sure the intermediate result is correctly processed in the next step await mock_rewoo_agent.executor_node(mock_state) - assert isinstance(mock_state.intermediate_results["#E2"].content, list) + assert isinstance(mock_state.intermediate_results["#E2"].content, list) + assert len(mock_state.intermediate_results["#E2"].content) == 1 + assert isinstance(mock_state.intermediate_results["#E2"].content[0], dict) + # "#E1" placeholder should resolve to E1's output (list) inside the dict + assert mock_state.intermediate_results["#E2"].content[0]["query"] == mock_state.intermediate_results["#E1"].content
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/nat/agent/base.py(2 hunks)tests/nat/agent/test_rewoo.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nat/agent/base.py
🧰 Additional context used
📓 Path-based instructions (7)
{tests/**/*.py,examples/*/tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{tests/**/*.py,examples/*/tests/**/*.py}: Unit tests live in tests/ (or examples/*/tests) and use markers defined in pyproject.toml (e2e, integration)
Use pytest with pytest-asyncio for asynchronous tests
Mock external services with pytest_httpserver or unittest.mock; do not hit live endpoints in tests
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration to skip by default
Files:
tests/nat/agent/test_rewoo.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Format Python code with yapf (PEP8 base, column_limit=120) run second in the toolchain
Run ruff (ruff check --fix) using configuration in pyproject.toml; fix warnings unless explicitly ignored; ruff is a linter only (not for formatting)
Respect naming: snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Prefer httpx with SSL verification enabled by default for HTTP requests; follow OWASP Top-10
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 vectorized operations when beneficial
Files:
tests/nat/agent/test_rewoo.py
**/*.{py,md,sh,yml,yaml,toml,json,ini,cfg,txt,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces (no tabs) and ensure every file ends with a single trailing newline
Files:
tests/nat/agent/test_rewoo.py
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}: Every file must start with the standard SPDX Apache-2.0 header
All source files must include the SPDX Apache-2.0 header template
Files:
tests/nat/agent/test_rewoo.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived by setuptools-scm; never hard-code versions in code or documentation
Files:
tests/nat/agent/test_rewoo.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: passDocumentation 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 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:
tests/nat/agent/test_rewoo.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/agent/test_rewoo.py
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/nat/observability/exporter/test_file_exporter.py (1)
209-214: Tighten assertion to prevent duplicate loggingAssert exactly one exception log and ensure logger.error wasn’t called in this path.
Apply this diff:
- mock_logger.exception.assert_called() + mock_logger.exception.assert_called_once() + mock_logger.error.assert_not_called()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/nat/observability/exporter/test_file_exporter.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
{tests/**/*.py,examples/*/tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{tests/**/*.py,examples/*/tests/**/*.py}: Unit tests live in tests/ (or examples/*/tests) and use markers defined in pyproject.toml (e2e, integration)
Use pytest with pytest-asyncio for asynchronous tests
Mock external services with pytest_httpserver or unittest.mock; do not hit live endpoints in tests
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration to skip by default
Files:
tests/nat/observability/exporter/test_file_exporter.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Format Python code with yapf (PEP8 base, column_limit=120) run second in the toolchain
Run ruff (ruff check --fix) using configuration in pyproject.toml; fix warnings unless explicitly ignored; ruff is a linter only (not for formatting)
Respect naming: snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Prefer httpx with SSL verification enabled by default for HTTP requests; follow OWASP Top-10
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 vectorized operations when beneficial
Files:
tests/nat/observability/exporter/test_file_exporter.py
**/*.{py,md,sh,yml,yaml,toml,json,ini,cfg,txt,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces (no tabs) and ensure every file ends with a single trailing newline
Files:
tests/nat/observability/exporter/test_file_exporter.py
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}: Every file must start with the standard SPDX Apache-2.0 header
All source files must include the SPDX Apache-2.0 header template
Files:
tests/nat/observability/exporter/test_file_exporter.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived by setuptools-scm; never hard-code versions in code or documentation
Files:
tests/nat/observability/exporter/test_file_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: passDocumentation 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 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:
tests/nat/observability/exporter/test_file_exporter.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/observability/exporter/test_file_exporter.py
🔇 Additional comments (1)
tests/nat/observability/exporter/test_file_exporter.py (1)
209-214: Good shift to logger.exception for non-re-raised errorsMatches the PR’s logging policy: when swallowing errors, use logger.exception to capture traceback without re-raising.
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.py (1)
81-87: Key mismatch: function_map is keyed by FunctionRef but lookup uses a str name.
function_mapusesFunctionRefkeys, but later checks/indexes withitem.name(a string). This will never match, causing all items to hit the “not found” path.Create a name-indexed map for lookups while keeping the existing map for descriptions.
- function_map = {} + function_map = {} # keyed by FunctionRef (for description) + function_map_by_name = {} # keyed by tool/function name (for lookups) for fn_ref in config.augmented_fns: # Retrieve the actual function from the builder fn_obj = builder.get_function(fn_ref) function_map[fn_ref] = fn_obj + ref_name = getattr(fn_ref, "name", str(fn_ref)) + function_map_by_name[ref_name] = fn_obj- for item in ttc_items: - if item.name not in function_map: - logger.error("Function '%s' not found in function map.", item.name) + for item in ttc_items: + if item.name not in function_map_by_name: + logger.error("Function '%s' not found in function map.", item.name) item.output = f"Error: Function '{item.name}' not found in function map. Check your input" else: - fn = function_map[item.name] + fn = function_map_by_name[item.name] tasks.append(_invoke_tool(item, fn))Also applies to: 156-162
examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.py (1)
205-208: Fix divide-by-zero when scaling tokens/sec axisIf all token counts and TPS are zero, scaling_factor becomes 0 and Line 254 divides by zero.
Apply:
- scaling_factor = max_token_count / max(max_tps, 1) * 0.8 # Scale tokens/sec to be visible but not dominate + # Guard against zero values to avoid division by zero when setting the secondary y-axis + if max_token_count <= 0 or max_tps <= 0: + scaling_factor = 1.0 + else: + scaling_factor = (max_token_count / max_tps) * 0.8 # Scale tokens/sec to be visible but not dominate- ax2.set_ylim(0, ax.get_ylim()[1] / scaling_factor) + ax2.set_ylim(0, ax.get_ylim()[1] / (scaling_factor if scaling_factor > 0 else 1.0))Also applies to: 254-254
src/nat/agent/tool_calling_agent/agent.py (2)
158-165: Same issue in build_graph: switch to logger.error when re-raising.
Avoid duplicate tracebacks and drop exc_info usage.- except Exception as ex: - logger.exception( - "%s Failed to build Tool Calling Agent Graph: %s", - AGENT_LOG_PREFIX, - ex, - exc_info=ex, - ) - raise + except Exception as ex: + logger.error( + "%s Failed to build Tool Calling Agent Graph: %s", + AGENT_LOG_PREFIX, + ex, + ) + raise
167-176: Docstring return type mismatch.
Docstring claims ChatPromptTemplate, but function returns str | None.- Returns: - ChatPromptTemplate: The Tool Calling Agent prompt. + Returns: + Optional[str]: The Tool Calling Agent prompt string, or None if no prompt parts provided.src/nat/agent/rewoo_agent/agent.py (4)
95-103: Use logger.error when re-raising to avoid duplicate tracebacks.Switch from logger.exception(..., exc_info=True) to logger.error(...) since you re-raise immediately.
- logger.exception("%s Unable to find tool with the name %s\n%s", - AGENT_LOG_PREFIX, - tool_name, - ex, - exc_info=True) + logger.error("%s Unable to find tool with the name %s: %s", AGENT_LOG_PREFIX, tool_name, ex) raise
235-246: Bug: intermediate_results stores raw tool_response, but downstream still dereferences .content.Line 275 writes raw tool_response; loops at Lines 238–246 and 293–305 still assume ToolMessage via .content. This will raise AttributeError or produce wrong substitution.
Apply these diffs to unwrap ToolMessage only when present and otherwise use the raw value:
@@ - for _placeholder, _tool_output in intermediate_results.items(): - _tool_output = _tool_output.content - # If the content is a list, get the first element which should be a dict - if isinstance(_tool_output, list): - _tool_output = _tool_output[0] - assert isinstance(_tool_output, dict) - - tool_input = self._replace_placeholder(_placeholder, tool_input, _tool_output) + for _placeholder, _tool_output in intermediate_results.items(): + # Unwrap ToolMessage for backward compatibility; otherwise use raw value + if isinstance(_tool_output, ToolMessage): + _tool_output = _tool_output.content + # If list and first element is a dict, use it (keeps existing behavior in tests) + if isinstance(_tool_output, list) and _tool_output and isinstance(_tool_output[0], dict): + _tool_output = _tool_output[0] + tool_input = self._replace_placeholder(_placeholder, tool_input, _tool_output) @@ - for _placeholder, _tool_output in intermediate_results.items(): - _tool_output = _tool_output.content - # If the content is a list, get the first element which should be a dict - if isinstance(_tool_output, list): - _tool_output = _tool_output[0] - assert isinstance(_tool_output, dict) - - tool_input = self._replace_placeholder(_placeholder, tool_input, _tool_output) - - placeholder = placeholder.replace(_placeholder, str(_tool_output)) + for _placeholder, _tool_output in intermediate_results.items(): + if isinstance(_tool_output, ToolMessage): + _tool_output = _tool_output.content + if isinstance(_tool_output, list) and _tool_output and isinstance(_tool_output[0], dict): + _tool_output = _tool_output[0] + tool_input = self._replace_placeholder(_placeholder, tool_input, _tool_output) + placeholder = placeholder.replace(_placeholder, str(_tool_output))Also widen _replace_placeholder’s tool_output type to accept lists (see separate comment) and update the ReWOOGraphState.intermediate_results type accordingly (see separate comment).
Also applies to: 275-276, 293-305
394-396: Use logger.error when re-raising in validate_planner_prompt.Avoid double stack traces; keep behavior consistent with this PR.
- logger.exception("%s %s", AGENT_LOG_PREFIX, error_text) + logger.error("%s %s", AGENT_LOG_PREFIX, error_text) raise ValueError(error_text)
52-55: Update ReWOOGraphState.intermediate_results type to reflect raw tool_response.The new storage at Line 275 saves raw values, not ToolMessage; update the schema to prevent type/pyright mismatches.
- intermediate_results: dict[str, ToolMessage] = Field(default_factory=dict) # the intermediate results of each step + intermediate_results: dict[str, Any] = Field(default_factory=dict) # the intermediate results of each stepAdd the missing import (see next comment).
♻️ Duplicate comments (3)
src/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.py (1)
156-159: Don’t pass exc_info outside an exception context; log a clean error.
exc_info=Truehere can emit a misleading “NoneType: None” traceback and conflicts with the PR’s logging policy. Use parameterizedlogger.errorwithoutexc_info.- if item.name not in function_map: - logger.error(f"Function '{item.name}' not found in function map.", exc_info=True) + if item.name not in function_map: + logger.error("Function '%s' not found in function map.", item.name) item.output = f"Error: Function '{item.name}' not found in function map. Check your input"src/nat/agent/tool_calling_agent/agent.py (2)
103-104: Past feedback addressed: switched to logger.error + bare raise.
This resolves the duplicate traceback issue previously noted.
146-147: Fix: logger.exception + re-raise causes duplicate stack traces; use logger.error before bare raise.
Also avoid passing exc_info=ex; prefer no traceback at this layer since you re-raise.- except Exception as ex: - logger.exception("%s Failed to call tool_node: %s", AGENT_LOG_PREFIX, ex, exc_info=ex) - raise + except Exception as ex: + logger.error("%s Failed to call tool_node: %s", AGENT_LOG_PREFIX, ex) + raise
🧹 Nitpick comments (13)
src/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.py (2)
151-152: Use parameterized logger.exception; avoid duplicating exception text.
logger.exceptionalready records the traceback and exception message. Prefer parameterized logging to avoid eager f-string formatting and duplicate error text.- logger.exception(f"Error invoking function '{item.name}': {e}") + logger.exception("Error invoking function '%s'", item.name)
119-121: Optional: make function names in description stable and user-friendly.If
FunctionRef.__str__isn’t guaranteed to be the user-facing name, prefer the explicitnamefield when present.- fn_description = ("\n".join(f"- **{fn_ref}**: {function_map[fn_ref].description or 'No description provided.'}" - for fn_ref in config.augmented_fns)) + fn_description = "\n".join( + f"- **{getattr(fn_ref, 'name', str(fn_ref))}**: " + f"{function_map[fn_ref].description or 'No description provided.'}" + for fn_ref in config.augmented_fns + )examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.py (2)
75-77: Avoid blocking the event loop: offload CSV read to a threadpd.read_csv is blocking; run it in a thread in this async context.
Apply:
- df = pd.read_csv(input_obj.df_path) + df = await asyncio.to_thread(pd.read_csv, input_obj.df_path)Add the import near the other imports:
import asyncio
58-60: Add return type for async generatorAnnotate the yielded FunctionInfo for pyright and readability.
Apply:
-async def token_usage(config: TokenUsageConfig, builder: Builder): +async def token_usage(config: TokenUsageConfig, builder: Builder) -> AsyncIterator[FunctionInfo]:And add:
from collections.abc import AsyncIteratorexamples/advanced_agents/profiler_agent/src/nat_profiler_agent/register.py (2)
112-114: Log the exception message without a traceback to aid triageCapture the exception object in the message while still avoiding duplicate tracebacks.
Apply:
- except Exception: - logger.error("Error in profiler agent, exit early") + except Exception as e: + logger.error("Error in profiler agent, exit early: %s", e) raise
59-61: Add return type for async generatorAnnotate to make intent explicit and satisfy type checkers.
Apply:
-async def profiler_agent(config: ProfilerAgentConfig, builder: Builder): +async def profiler_agent(config: ProfilerAgentConfig, builder: Builder) -> AsyncIterator[FunctionInfo]:And add (if not already present in this module):
from collections.abc import AsyncIteratorsrc/nat/agent/tool_calling_agent/agent.py (3)
118-123: Drop redundant exc_info=True with logger.exception.
logger.exception already includes the traceback.- logger.exception( - "%s Failed to determine whether agent is calling a tool: %s", - AGENT_LOG_PREFIX, - ex, - exc_info=True, - ) + logger.exception( + "%s Failed to determine whether agent is calling a tool: %s", + AGENT_LOG_PREFIX, + ex, + )
51-59: Annotate Optional for callbacks parameter.
PEP 484/pyright: default None should be typed as optional.- callbacks: list[AsyncCallbackHandler] = None, + callbacks: list[AsyncCallbackHandler] | None = None,
87-105: Add explicit return type annotations to public async methods.
Required by repo guidelines for public APIs.- async def agent_node(self, state: ToolCallAgentGraphState): + async def agent_node(self, state: ToolCallAgentGraphState) -> ToolCallAgentGraphState: - async def conditional_edge(self, state: ToolCallAgentGraphState): + async def conditional_edge(self, state: ToolCallAgentGraphState) -> AgentDecision: - async def tool_node(self, state: ToolCallAgentGraphState): + async def tool_node(self, state: ToolCallAgentGraphState) -> ToolCallAgentGraphState: - async def build_graph(self): + async def build_graph(self) -> typing.Any:Also applies to: 106-126, 127-149, 149-156
src/nat/agent/rewoo_agent/agent.py (4)
338-344: Nit: logger.exception already includes the traceback; drop exc_info=True.You’re not re-raising here, so logger.exception is correct, but exc_info=True is redundant.
- logger.exception("%s Failed to determine whether agent is calling a tool: %s", - AGENT_LOG_PREFIX, - ex, - exc_info=True) + logger.exception("%s Failed to determine whether agent is calling a tool: %s", + AGENT_LOG_PREFIX, + ex)
16-19: Add typing.Any import for the updated state type.import json import logging from json import JSONDecodeError +from typing import Any
128-146: Broaden _replace_placeholder type for tool_output to accept lists.Needed to support list outputs without wrapping in ToolMessage.
- def _replace_placeholder(placeholder: str, tool_input: str | dict, tool_output: str | dict) -> str | dict: + def _replace_placeholder(self, placeholder: str, tool_input: str | dict, + tool_output: str | dict | list) -> str | dict:
178-207: Add return type hints to public async node methods.planner_node (and executor_node/solver_node/conditional_edge/build_graph) are public APIs here; add return annotations per repo guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
examples/advanced_agents/profiler_agent/src/nat_profiler_agent/register.py(1 hunks)examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/flow_chart.py(1 hunks)examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.py(1 hunks)examples/notebooks/retail_sales_agent/src/nat_retail_sales_agent/data_visualization_agent.py(1 hunks)src/nat/agent/react_agent/agent.py(5 hunks)src/nat/agent/rewoo_agent/agent.py(7 hunks)src/nat/agent/tool_calling_agent/agent.py(4 hunks)src/nat/eval/evaluate.py(1 hunks)src/nat/eval/rag_evaluator/evaluate.py(1 hunks)src/nat/eval/swe_bench_evaluator/evaluate.py(2 hunks)src/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.py(1 hunks)src/nat/registry_handlers/package_utils.py(1 hunks)src/nat/tool/retriever.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/nat/registry_handlers/package_utils.py
🚧 Files skipped from review as they are similar to previous changes (7)
- examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/flow_chart.py
- src/nat/eval/swe_bench_evaluator/evaluate.py
- src/nat/agent/react_agent/agent.py
- src/nat/tool/retriever.py
- src/nat/eval/rag_evaluator/evaluate.py
- examples/notebooks/retail_sales_agent/src/nat_retail_sales_agent/data_visualization_agent.py
- src/nat/eval/evaluate.py
🧰 Additional context used
📓 Path-based instructions (10)
{src/**/*.py,packages/*/src/**/*.py,examples/*/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' only for env var prefixes and informal code comments (never in documentation)
Files:
examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.pysrc/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.pyexamples/advanced_agents/profiler_agent/src/nat_profiler_agent/register.pysrc/nat/agent/tool_calling_agent/agent.pysrc/nat/agent/rewoo_agent/agent.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Format Python code with yapf (PEP8 base, column_limit=120) run second in the toolchain
Run ruff (ruff check --fix) using configuration in pyproject.toml; fix warnings unless explicitly ignored; ruff is a linter only (not for formatting)
Respect naming: snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Prefer httpx with SSL verification enabled by default for HTTP requests; follow OWASP Top-10
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 vectorized operations when beneficial
Files:
examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.pysrc/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.pyexamples/advanced_agents/profiler_agent/src/nat_profiler_agent/register.pysrc/nat/agent/tool_calling_agent/agent.pysrc/nat/agent/rewoo_agent/agent.py
**/*.{py,md,sh,yml,yaml,toml,json,ini,cfg,txt,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces (no tabs) and ensure every file ends with a single trailing newline
Files:
examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.pysrc/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.pyexamples/advanced_agents/profiler_agent/src/nat_profiler_agent/register.pysrc/nat/agent/tool_calling_agent/agent.pysrc/nat/agent/rewoo_agent/agent.py
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}: Every file must start with the standard SPDX Apache-2.0 header
All source files must include the SPDX Apache-2.0 header template
Files:
examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.pysrc/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.pyexamples/advanced_agents/profiler_agent/src/nat_profiler_agent/register.pysrc/nat/agent/tool_calling_agent/agent.pysrc/nat/agent/rewoo_agent/agent.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived by setuptools-scm; never hard-code versions in code or documentation
Files:
examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.pysrc/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.pyexamples/advanced_agents/profiler_agent/src/nat_profiler_agent/register.pysrc/nat/agent/tool_calling_agent/agent.pysrc/nat/agent/rewoo_agent/agent.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: passDocumentation 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 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/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.pysrc/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.pyexamples/advanced_agents/profiler_agent/src/nat_profiler_agent/register.pysrc/nat/agent/tool_calling_agent/agent.pysrc/nat/agent/rewoo_agent/agent.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/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.pyexamples/advanced_agents/profiler_agent/src/nat_profiler_agent/register.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ so namespace packages resolve correctly
Files:
src/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.pysrc/nat/agent/tool_calling_agent/agent.pysrc/nat/agent/rewoo_agent/agent.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All public APIs require Python 3.11+ type hints on parameters and return values
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
Files:
src/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.pysrc/nat/agent/tool_calling_agent/agent.pysrc/nat/agent/rewoo_agent/agent.py
src/nat/**/*
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.pysrc/nat/agent/tool_calling_agent/agent.pysrc/nat/agent/rewoo_agent/agent.py
🔇 Additional comments (7)
src/nat/experimental/test_time_compute/functions/ttc_tool_orchestration_function.py (1)
146-149: Confirm correct payload: are we meant to invoke with item.output or item.input?If search/editing populates
item.output, this is fine; otherwise the tool may receiveNone. Please verify intended data flow. If needed, use a fallback.- result = await fn.acall_invoke(item.output) + payload = item.output if item.output is not None else item.input + result = await fn.acall_invoke(payload)examples/advanced_agents/profiler_agent/src/nat_profiler_agent/tool/token_usage.py (1)
106-108: Exception handling aligned with PR objectives — LGTMSwitched to logger.error without exc_info and bare raise. This prevents duplicate stack traces while preserving the original traceback.
src/nat/agent/tool_calling_agent/agent.py (1)
67-68: Good: using logger.error + bare raise preserves traceback and avoids duplicate stack traces.
Matches the PR’s logging policy for re-raises.src/nat/agent/rewoo_agent/agent.py (4)
204-206: LGTM: planner_node now logs with logger.error before re-raising.Matches the PR objective; avoids double stack traces.
279-280: LGTM: executor_node now logs with logger.error before re-raising.Consistent with the project-wide pattern.
323-324: LGTM: solver_node now logs with logger.error before re-raising.Aligned with the PR objective.
399-407: LGTM: validate_solver_prompt now uses logger.error before raising.Consistent with the new error-handling pattern.
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.coderabbit.yaml (1)
55-59: Clarify logging guidance to avoid ambiguity and duplicationSpell out that
logger.errorhere should not passexc_info=True, add a note on parameterized logging, and cover the exception translation case.- - For **Python exception handling**, ensure proper stack trace preservation: - - When re-raising exceptions: use bare `raise` statements to maintain the original stack trace, - and use `logger.error()` (not `logger.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. + - For **Python exception handling**, ensure proper stack trace preservation: + - When re-raising exceptions: use bare `raise` to propagate the original traceback, and use + `logger.error(...)` without `exc_info=True` (not `logger.exception(...)`) in that layer to + avoid duplicate stack traces. + - When catching and logging exceptions without re-raising: use `logger.exception(...)` to + capture the full traceback. + - Prefer parameterized logging over f-strings (e.g., `logger.error("Failed to fetch %s", url)`). + - If you must translate to a different exception type, use `raise NewError(...) from exc` to + preserve the cause, and ensure the stack trace is logged once at an appropriate boundary.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.coderabbit.yaml(1 hunks)
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (46)
src/nat/tool/nvidia_rag.py (1)
45-46: Add return type and docstring to the public API.Public APIs require return annotations and Google-style docstrings.
Apply:
+from collections.abc import AsyncIterator @@ -async def nvidia_rag_tool(config: NVIDIARAGToolConfig, builder: Builder): +async def nvidia_rag_tool(config: NVIDIARAGToolConfig, builder: Builder) -> AsyncIterator[FunctionInfo]: + """Register the NVIDIA RAG tool for querying NVIDIA Developer documents. + + Yields: + FunctionInfo: Runnable that performs RAG search and returns formatted documents. + """src/nat/cli/commands/workflow/workflow_commands.py (1)
249-260: Bug: creating a file symlink inside a non-existent (soon-to-be symlinked) directory.You symlink config.yml to new_workflow_dir/configs/config.yml before creating new_workflow_dir/configs (and then you symlink the entire configs directory). On most systems this fails (parent path missing) and the file-level symlink is redundant once the directory symlink is created.
Apply this diff to remove the redundant/broken file-level symlink:
- # Create symlink for config.yml - config_link = new_workflow_dir / 'configs' / 'config.yml' - os.symlink(config_source, config_link) - # Create symlinks for config and data directories config_dir_source = configs_dir config_dir_link = new_workflow_dir / 'configs' data_dir_source = data_dir data_dir_link = new_workflow_dir / 'data' os.symlink(config_dir_source, config_dir_link) os.symlink(data_dir_source, data_dir_link)examples/evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.py (1)
376-379: Use logger.exception when handling and returning an error string (no re-raise).Aligns with repo-wide rule and preserves traceback.
- except Exception as e: - logger.error("Error processing %s: %s", swebench_input.instance_id, e) - return f"Error: {str(e)}" + except Exception as e: + logger.exception("Error processing %s: %s", swebench_input.instance_id, e) + return f"Error: {str(e)}"src/nat/agent/react_agent/register.py (1)
16-17: Don’t swallow cancellations; re-raiseasyncio.CancelledError(log with error per policy).Catching broad
Exceptionhere will also catch cancellations, breaking cooperative shutdown. Add a dedicatedexcept asyncio.CancelledErrorthat logs withlogger.error()and thenraiseto preserve the stack trace, aligning with the PR’s logging guidance.Apply this diff:
+import asyncio import logging @@ try: @@ - except Exception as ex: + except asyncio.CancelledError: + logger.error("%s ReAct Agent cancelled", AGENT_LOG_PREFIX) + raise + except Exception as ex: logger.exception("%s ReAct Agent failed with exception: %s", AGENT_LOG_PREFIX, ex)Also applies to: 135-141
src/nat/cli/cli_utils/config_override.py (2)
208-218: Use logger.exception and exception chaining when transforming exceptions (preserve stack).This block converts the original exception into click.BadParameter. Since we're not re-raising the original, per repo guidance use logger.exception() and chain the new exception with "from e" to keep the original traceback available without duplicating it elsewhere.
- except Exception as e: - logger.error("Modified configuration failed validation: %s", e) - raise click.BadParameter(f"Modified configuration failed validation: {str(e)}") + except Exception as e: + logger.exception("Modified configuration failed validation") + raise click.BadParameter(f"Modified configuration failed validation: {e}") from e
211-214: Avoid dumping full config at INFO; redact secrets and lower to DEBUG.Config may include tokens/keys; logging it verbatim risks leakage. Redact sensitive fields and move to DEBUG. Also prefer safe_dump for consistency.
- logger.info( - "\n\nConfiguration after overrides:\n\n%s", - yaml.dump(effective_config, default_flow_style=False), - ) + safe_config = _redact_sensitive(effective_config) + logger.debug( + "\n\nConfiguration after overrides (redacted):\n\n%s", + yaml.safe_dump(safe_config, default_flow_style=False), + )Add helper (outside this hunk):
import re SENSITIVE_KEYS = re.compile(r"(secret|token|password|api[_-]?key|client_secret|bearer)", re.I) def _redact_sensitive(obj: Any) -> Any: if isinstance(obj, dict): return {k: ("***REDACTED***" if SENSITIVE_KEYS.search(k) else _redact_sensitive(v)) for k, v in obj.items()} if isinstance(obj, list): return [_redact_sensitive(v) for v in obj] return objsrc/nat/eval/intermediate_step_adapter.py (1)
80-86: Type mismatch: pass None instead of '' to get_agent_action_single.An empty string violates the annotated type (IntermediateStep | None) and will trip pyright; use None for clarity and correctness.
Apply this diff:
- action = self.get_agent_action_single(step, "") + action = self.get_agent_action_single(step, None)src/nat/settings/global_settings.py (2)
189-194: Bug: instance attribute shadows ClassVar for configuration directory.
_configuration_directoryis declared asClassVar[str], but here it’s set on the instance, diverging from the class-level state used elsewhere.Apply this diff:
- self._configuration_directory = os.getenv("NAT_CONFIG_DIR", user_config_dir(appname="nat")) + self.__class__._configuration_directory = os.getenv("NAT_CONFIG_DIR", user_config_dir(appname="nat"))
247-254: Bug: filtering iterates the whole settings dict instead of the channels map.Looping over
remote_channels.items()and accessingsettings["type"]will KeyError on top-level keys and deletes wrong entries.Apply this diff:
- if (channel_type is not None): - filter_channels = [] - for channel, settings in remote_channels.items(): - if (settings["type"] != channel_type): - filter_channels.append(channel) - for channel in filter_channels: - del remote_channels[channel] + if (channel_type is not None): + channels = remote_channels.get("channels", {}) + filter_channels = [name for name, cfg in channels.items() if cfg.get("type") != channel_type] + for name in filter_channels: + del channels[name]src/nat/retriever/milvus/retriever.py (2)
137-155: Bug: premature return inside iterator loop stops after first batch.Return is inside the while True loop; results beyond the first page are never collected. Move return after the loop.
results = [] try: while True: _res = search_iterator.next() res = _res.get_res() if len(_res) == 0: search_iterator.close() break @@ results.extend(res[0]) - - return _wrap_milvus_results(results, content_field=self.content_field) + return _wrap_milvus_results(results, content_field=self.content_field)
114-121: Iterator path may omit content_field from output_fields (KeyError risk).Match the non-iterator path by ensuring content_field is included.
# If no output fields are specified, return all of them if not output_fields: collection_schema = self._client.describe_collection(collection_name) output_fields = [ field["name"] for field in collection_schema.get("fields") if field["name"] != vector_field_name ] + if self.content_field not in output_fields: + output_fields.append(self.content_field)src/nat/retriever/nemo_retriever/retriever.py (2)
115-121: Fix type hints: top_k should be int; output_fields should be Optional[List[str]].Current hints can trip pyright and violate repo typing rules.
async def _search( self, query: str, collection_name: str, - top_k: str, - output_fields: list[str] = None, + top_k: int, + output_fields: list[str] | None = None, ):
163-180: _flatten signature/annotation is incorrect and doesn’t accept None.It returns a dict and caller can pass None for output_fields.
-def _flatten(obj: dict, output_fields: list[str]) -> list[str]: +def _flatten(obj: dict, output_fields: list[str] | None) -> dict: base_fields = [ "format", "id", ] if not output_fields: output_fields = [ "format", "id", ] output_fields.extend(list(obj["metadata"].keys())) data = {"content": obj.get("content")} for field in base_fields: if field in output_fields: data.update({field: obj[field]}) data.update({k: v for k, v in obj['metadata'].items() if k in output_fields}) return datasrc/nat/data_models/discovery_metadata.py (1)
54-64: Fix typos and follow “nat” naming in docs.Correct misspellings (“faciliate”, “framwork”), grammar (“A an object”), and use “nat” (not “NAT”) in documentation per guidelines.
- """A data model representing metadata about each registered component to faciliate its discovery. + """A data model representing metadata about each registered component to facilitate its discovery. @@ - package (str): The name of the package containing the NAT component. + package (str): The name of the package containing the nat component. @@ - component_type (ComponentEnum): The type of NAT component this metadata represents. + component_type (ComponentEnum): The type of nat component this metadata represents. @@ - description (str): Description of the NAT component pulled from its config objects docstrings. + description (str): Description of the nat component pulled from its config object docstrings. @@ - str: The distribution name of the NAT component. + str: The distribution name of the nat component. @@ - # This is because the module package is the root package for the NAT component + # This is because the module package is the root package for the nat component @@ - """Get the distribution name from the config type using the mapping of module names to distro names. + """Get the distribution name from the config type using the mapping of module names to distribution names. @@ - """Generates discovery metadata from function with specified wrapper type. + """Generates discovery metadata from a function with the specified wrapper type. @@ - wrapper_type (LLMFrameworkEnum): The wrapper to apply to the callable to faciliate inter-framwork - interoperability. + wrapper_type (LLMFrameworkEnum): The wrapper to apply to the callable to facilitate inter-framework + interoperability. @@ - Returns: - DiscoveryMetadata: A an object containing component metadata to facilitate discovery and reuse. + Returns: + DiscoveryMetadata: An object containing component metadata to facilitate discovery and reuse. @@ - """Generates discovery metadata from an installed package name. + """Generates discovery metadata from an installed package name. @@ - Returns: - DiscoveryMetadata: A an object containing component metadata to facilitate discovery and reuse. + Returns: + DiscoveryMetadata: An object containing component metadata to facilitate discovery and reuse. @@ - """Generates discovery metadata from provider and framework mapping information. + """Generates discovery metadata from provider and framework mapping information. @@ - Returns: - DiscoveryMetadata: A an object containing component metadata to facilitate discovery and reuse. + Returns: + DiscoveryMetadata: An object containing component metadata to facilitate discovery and reuse.Also applies to: 105-113, 126-128, 141-147, 195-205, 240-244, 280-284
examples/frameworks/multi_frameworks/src/nat_multi_frameworks/langchain_research_tool.py (3)
45-51: Fix env var assignment before validation (avoids TypeError when unset).If NVIDIA_API_KEY is missing, Line 46 will raise before the intended ValueError. Check first, then assign.
- api_token = os.getenv("NVIDIA_API_KEY") - os.environ["NVIDIA_API_KEY"] = api_token - - if not api_token: - raise ValueError( - "API token must be provided in the configuration or in the environment variable `NVIDIA_API_KEY`") + api_token = os.getenv("NVIDIA_API_KEY") + if not api_token: + raise ValueError( + "API token must be provided in the configuration or in the environment variable `NVIDIA_API_KEY`") + os.environ["NVIDIA_API_KEY"] = api_token
55-60: Return type mismatch and brittle splitting in web_search.Annotation says list[dict] but a str is returned; also guard non-str outputs.
- async def web_search(topic: str) -> list[dict]: - output = (await tavily_tool.ainvoke(topic)) - output = output.split("\n\n---\n\n") - - return output[0] + async def web_search(topic: str) -> str: + raw = await tavily_tool.ainvoke(topic) + if isinstance(raw, str): + parts = raw.split("\n\n---\n\n") + return parts[0] if parts else raw + return str(raw)
84-105: Harden execute_tool: initialize topic, tighten checks, and streamline exception logging.Predefine topic to avoid UnboundLocalError; use truthy/strip check; rely on logger.exception’s traceback and remove redundant pass.
- async def execute_tool(out): - try: - topic = out.topic - if topic is not None and topic not in ['', '\n']: - output_summary = (await web_search(topic)) + async def execute_tool(out: TopicExtract) -> str: + topic = None + try: + topic = getattr(out, "topic", None) + if topic and topic.strip(): + output_summary = await web_search(topic) # Clean HTML tags from the output if isinstance(output_summary, str): # Remove HTML tags using BeautifulSoup soup = BeautifulSoup(output_summary, 'html.parser') output_summary = soup.get_text() # Clean up any extra whitespace output_summary = re.sub(r'\s+', ' ', output_summary).strip() else: - output_summary = f"this search on web search with topic:{topic} yield not results" + output_summary = f"This web search with topic '{topic}' yielded no results" except Exception as e: - output_summary = f"this search on web search with topic:{topic} yield not results with an error:{e}" - logger.exception("error in executing tool: %s", e) - pass + output_summary = f"This web search with topic '{topic}' yielded no results due to error: {e}" + logger.exception("error executing tool for topic %r", topic) return output_summaryexamples/HITL/por_to_jiratickets/src/nat_por_to_jiratickets/extract_por_tool.py (3)
164-173: Save failure is swallowed; function reports success even when persistence fails.This returns “Extraction complete” despite an I/O error. Re-raise after logging, using logger.error to avoid duplicate stack traces per the PR convention.
- except Exception as e: - logger.exception("An error occurred while saving the file: %s", e) + except Exception: + logger.error("Failed to save extraction to %s.", filename) + raise
191-201: Use logger.error when re-raising to avoid duplicate tracebacks.You log with logger.exception and then bare raise, which will double-print the traceback.
- except Exception as e: - logger.exception("An error occurred while loading the file: %s", e) - raise + except Exception: + logger.error("Failed to load tickets file: %s", filename) + raise
186-201: Remove immediate exception logging before re-raising to avoid duplicate tracebacksIn each of these handlers we log the exception and then immediately re-raise it, which results in two stack traces. To align with our convention, switch to using
logger.error(…, exc_info=True)and a bareraise, or remove the log entirely if it’s handled upstream.Affected locations:
scripts/web_utils.py, lines 112–114src/nat/agent/react_agent/agent.py, lines 131–133examples/HITL/por_to_jiratickets/src/nat_por_to_jiratickets/extract_por_tool.py, lines 199–201Proposed diffs:
--- scripts/web_utils.py @@ -112,7 +112,7 @@ def save_to_cache(input_dict, base_path): except Exception as e: - logger.exception("Unable to save data for %s", url, exc_info=True) - raise e + logger.error("Unable to save data for %s", url, exc_info=True) + raise --- src/nat/agent/react_agent/agent.py @@ -131,7 +131,7 @@ class ReactAgent: def _get_tool(self, tool_name: str): try: return self.tools_dict.get(tool_name) - except Exception as ex: - logger.exception("%s Unable to find tool with the name %s\n%s", AGENT_LOG_PREFIX, tool_name, ex) - raise + except Exception as ex: + logger.error("%s Unable to find tool with the name %s", AGENT_LOG_PREFIX, tool_name, exc_info=True) + raise --- examples/HITL/por_to_jiratickets/src/nat_por_to_jiratickets/extract_por_tool.py @@ -199,7 +199,7 @@ async def show_tickets_tool(config: ShowTicketsToolConfig, builder: Builder): try: with open(filename, 'r', encoding='utf-8') as json_file: data = json.load(json_file) - except Exception as e: - logger.exception("An error occurred while loading the file: %s", e) - raise + except Exception: + logger.error("An error occurred while loading the file", exc_info=True) + raiseexamples/frameworks/multi_frameworks/src/nat_multi_frameworks/register.py (3)
118-121: Bug: route target typo breaks conditional edge"supevisor" will not match any edge key; this will misroute/raise at runtime.
Fix:
- route_to = "supevisor" + route_to = "supervisor"
130-131: Potential None deref on chosen_worker_agent
worker_choice.lower()will crash ifchosen_worker_agentis None.Harden:
- worker_choice = state["chosen_worker_agent"] + worker_choice = state["chosen_worker_agent"] or ""
88-96: Type mismatch: chat_history declared as list but is ChatMessageHistoryThis will trip pyright and is misleading.
Update:
- chat_history: list[BaseMessage] | None + chat_history: ChatMessageHistorysrc/nat/registry_handlers/rest/rest_handler.py (2)
60-60: Authorization header value has an extra colon (“Bearer: …”) and will be invalid.
Use “Bearer ” (no colon) per RFC 6750.- self._headers = RequestHeaders(Authorization=f"Bearer: {token}").model_dump(by_alias=True) + self._headers = RequestHeaders(Authorization=f"Bearer {token}").model_dump(by_alias=True)
149-159: Correct typo and consolidate logging in exception handlerThe exception block in
src/nat/registry_handlers/rest/rest_handler.pycurrently logs the same error twice and contains a spelling mistake. Please update as follows:• Fix the typo “occured” → “occurred”
• Remove the separatelogger.errorcall and use a single, parameterizedlogger.exceptioncallSuggested diff:
--- a/src/nat/registry_handlers/rest/rest_handler.py +++ b/src/nat/registry_handlers/rest/rest_handler.py @@ -149,12 +149,11 @@ except Exception as e: - msg = f"Error occured when installing packages: {e}" - logger.error(msg) + msg = f"Error occurred when installing packages: {e}" if os.path.exists(tmp_dir): shutil.rmtree(tmp_dir) validated_pull_response = PullResponse(status={ "status": StatusEnum.ERROR, - "message": msg, "action": ActionEnum.PULL + "message": msg, + "action": ActionEnum.PULL }) - logger.exception(validated_pull_response.status.message) + logger.exception("Error occurred when installing packages: %s", e)src/nat/cli/commands/registry/pull.py (3)
51-54: Wheel detection check is wrong; use endswith or Path.suffix.
Current: package[:-4] == ".whl" always compares all-but-last-4 chars to “.whl”.- if package[:-4] == ".whl": + if package.endswith(".whl"):Alternative:
- if package[:-4] == ".whl": + if Path(package).suffix == ".whl":
61-63: Logic bug: comparing list to int.
Should check length of split, not equality of the list itself to 2.- if (package_split == 2): + if len(package_split) == 2: package_data["version"] = package_split[1]
49-58: Avoid assert for user input validation in CLI; raise click.BadParameter instead.
Asserts can be stripped with -O; use explicit errors for stable UX.- assert len(package) > 0, f"Supplied invalid package '{package}'." + if not package: + raise click.BadParameter(f"Supplied invalid package '{package}'.") @@ - assert len(package_split) in (1, 2), f"Supplied invalid package '{package}'." + if len(package_split) not in (1, 2): + raise click.BadParameter(f"Supplied invalid package '{package}'.")src/nat/registry_handlers/local/local_handler.py (3)
131-135: Bug: Error path returns StatusEnum.SUCCESS.This makes clients treat failures as successes.
Apply:
- validated_search_response = SearchResponse(params=query, - status={ - "status": StatusEnum.SUCCESS, - "message": msg, - "action": ActionEnum.SEARCH - }) + validated_search_response = SearchResponse(params=query, + status={ + "status": StatusEnum.ERROR, + "message": msg, + "action": ActionEnum.SEARCH + })
55-62: Wrong response type in publish().Yielding RemoveResponse from publish() breaks the declared API and typing.
Apply:
- try: - validated_remove_response = RemoveResponse(status={ - "status": StatusEnum.ERROR, "message": "Local publish not supported.", "action": ActionEnum.PUBLISH - }) - yield validated_remove_response - finally: - logger.warning(validated_remove_response.status.message) + try: + validated_publish_response = PublishResponse(status={ + "status": StatusEnum.ERROR, "message": "Local publish not supported.", "action": ActionEnum.PUBLISH + }) + yield validated_publish_response + finally: + logger.warning(validated_publish_response.status.message)
75-83: Wrong response type in pull().Yielding RemoveResponse from pull() breaks the declared API and typing.
Apply:
- try: - validated_remove_response = RemoveResponse(status={ - "status": StatusEnum.ERROR, "message": "Local pull not supported.", "action": ActionEnum.PULL - }) - - yield validated_remove_response - finally: - logger.warning(validated_remove_response.status.message) + try: + validated_pull_response = PullResponse(status={ + "status": StatusEnum.ERROR, "message": "Local pull not supported.", "action": ActionEnum.PULL + }) + + yield validated_pull_response + finally: + logger.warning(validated_pull_response.status.message)src/nat/registry_handlers/pypi/pypi_handler.py (4)
95-99: Return type mismatch for _upload_to_pypi.Function returns CompletedProcess but is annotated as None.
Apply:
- def _upload_to_pypi(self, wheel_path: str) -> None: + def _upload_to_pypi(self, wheel_path: str) -> subprocess.CompletedProcess[bytes]: @@ - return subprocess.run( + return subprocess.run( ["twine", "upload", "--repository-url", f"{self._endpoint}/{self._publish_route}", f"{wheel_path}"], check=True)
126-137: Packages passed as a single space-joined arg; installer likely treats it as one token.Pass each package as a separate argument.
Apply:
- versioned_packages_str = " ".join(versioned_packages) - - result = subprocess.run([ + result = subprocess.run([ "uv", "pip", "install", "--prerelease=allow", "--index-url", f"{self._endpoint}/{self._pull_route}/", - versioned_packages_str - ], + *versioned_packages + ], check=True)
200-201: Off-by-one and “no limit when top_k<=0” handling in search().Current logic can return top_k+1 items and still limit when top_k=0. Compute an effective limit and use >=.
Apply:
package_results = search_results.split("\n") for package_result in package_results: @@ - if (search_resp_item not in search_response_list): + if (search_resp_item not in search_response_list): search_response_list.append(search_resp_item) - if (len(search_response_list) > query.top_k): + limit = query.top_k if query.top_k > 0 else float("inf") + if (len(search_response_list) >= limit): breakAlso applies to: 171-179
229-229: Remove signature returns wrong type.Should yield RemoveResponse, not SearchResponse.
Apply:
- async def remove(self, packages: PackageNameVersionList) -> AsyncGenerator[SearchResponse]: + async def remove(self, packages: PackageNameVersionList) -> AsyncGenerator[RemoveResponse]:examples/HITL/por_to_jiratickets/src/nat_por_to_jiratickets/jira_tickets_tool.py (10)
16-24: Add typing imports to support corrected type hints belowPrepares for accurate return types and Optional.
import asyncio import json import logging import os import re import httpx -import requests +from typing import Any, Mapping, Optional +import requests
34-47: Return type is incorrect and path concat is fragileFunction promises str but returns parsed JSON or None; and root_path + "epics_tasks.json" breaks without a trailing slash. Fix signature and path join; keep exception policy consistent.
-def get_epics_tool(root_path: str) -> str: +def get_epics_tool(root_path: str) -> Optional[Mapping[str, Any]]: @@ - filename = root_path + "epics_tasks.json" + filename = os.path.join(root_path, "epics_tasks.json") @@ - except Exception as e: - logger.exception("An error occurred while loading the file: %s", e) + except Exception: + logger.exception("Failed to load %s", filename) return None @@ - return data + return data
68-107: create_epic: annotated return type vs actual return (tuple) mismatch; return a consistent shapeReturning a tuple on success and a dict on error causes downstream breakage. Return a dict in all cases.
- async def create_epic(self, client: httpx.AsyncClient, ticket_data: dict) -> str: + async def create_epic(self, client: httpx.AsyncClient, ticket_data: dict) -> dict[str, Any]: @@ - data = r.json() - return data["key"], data["self"] + data = r.json() + return {"key": data["key"], "self": data["self"]}
134-152: create_task: return a dict and add type hint to align with callersUnify with create_epic to prevent type errors.
- async def create_task(self, client: httpx.AsyncClient, ticket_data: dict): + async def create_task(self, client: httpx.AsyncClient, ticket_data: dict) -> dict[str, Any]: @@ - data = r.json() - return data["key"], data["self"] + data = r.json() + return {"key": data["key"], "self": data["self"]}
181-199: create_bug: return a dict and add type hint to match unified contractAligns with upstream handling.
- async def create_bug(self, client: httpx.AsyncClient, ticket_data: dict): + async def create_bug(self, client: httpx.AsyncClient, ticket_data: dict) -> dict[str, Any]: @@ - data = r.json() - return data["key"], data["self"] + data = r.json() + return {"key": data["key"], "self": data["self"]}
229-246: create_feature: return a dict and add type hintKeep return types uniform.
- async def create_feature(self, client: httpx.AsyncClient, ticket_data: dict): + async def create_feature(self, client: httpx.AsyncClient, ticket_data: dict) -> dict[str, Any]: @@ - data = r.json() - return data["key"], data["self"] + data = r.json() + return {"key": data["key"], "self": data["self"]}
284-308: Guard against missing/invalid pre-extracted data to avoid crashesjira_issues can be None or missing the requested key; current code would raise.
# input_text = process_input_text(input_text) jira_issues = get_epics_tool(config.root_path) logger.debug("Creating %s in Jira", input_text) jira = JiraTool(domain=config.jira_domain, project_key=config.jira_project_key, ticket_type=input_text) timeout_config = httpx.Timeout(config.timeout, connect=config.connect) lines = ["### Created " + str(input_text) + ":"] results = [] + if not jira_issues: + logger.error("No pre-extracted issues found under %s", config.root_path) + return ("Could not load pre-extracted issues (epics/tasks/bugs/features). " + "Please run the extraction step first.") + if input_text not in jira_issues or not isinstance(jira_issues.get(input_text), list): + logger.error("Input type '%s' not present in dataset. Available keys: %s", + input_text, ", ".join(jira_issues.keys())) + return (f"Input type '{input_text}' not present. " + f"Available: {', '.join(jira_issues.keys())}")
309-316: Results iteration assumes tuple; will break on error dictsWith unified dict return, format accordingly and surface per-item errors.
- for _, result in enumerate(results, start=1): - lines.append(f"- **{result[0]}**: {config.jira_domain + '/browse/' + str(result[0])}") + for res in results: + if isinstance(res, dict) and "error" in res: + detail = res.get("message") or res.get("details", "") + lines.append(f"- Error creating issue: {res['error']} {detail}".rstrip()) + else: + key = res["key"] + lines.append(f"- **{key}**: {config.jira_domain}/browse/{key}")
346-380: Avoid blocking the event loop: replace requests with httpx.AsyncClient and add error handlingrequests.get blocks inside async; switch to httpx and follow the exception-logging policy.
- async def _arun(input_text: str) -> str: - response = requests.get(api_endpoint, headers=headers, params=query_params, timeout=30) - - if response.status_code == 200: - data = response.json()["issues"] + async def _arun(input_text: str) -> str: + async with httpx.AsyncClient(timeout=30) as client: + try: + response = await client.get(api_endpoint, headers=headers, params=query_params) + response.raise_for_status() + except httpx.HTTPStatusError as err: + logger.exception("JIRA API HTTP %s: %s", err.response.status_code, err.response.text) + return f"Failed to fetch JIRA issues: HTTP {err.response.status_code}" + except httpx.RequestError as err: + logger.exception("Network error contacting JIRA: %s", err) + return "Failed to fetch JIRA issues due to a network error." + + data = response.json().get("issues", []) result = {"tasks": [], "epics": [], "new_features": [], "bugs": []} @@ - return "JIRA issues have been successfully saved to jira_tickets.json" + return "JIRA issues have been successfully saved to jira_tickets.json"
279-283: Switch to logger.exception in the exception handlerUse
logger.exceptioninstead oflogger.errorto capture the full traceback per PR logging policy.• Location:
- File:
examples/HITL/por_to_jiratickets/src/nat_por_to_jiratickets/jira_tickets_tool.py(around lines 278–283)• Diff to apply:
- except Exception as e: - logger.error("An error occurred when getting interaction content: %s", e) + except Exception: + logger.exception("An error occurred when getting interaction content") logger.info("Defaulting to not uploading to Jira") return ("Did not upload to Jira because human confirmation was not received. " "You can exit with a final answer")No other
logger.errorcalls were found insideexceptblocks in this directory.examples/frameworks/semantic_kernel_demo/src/nat_semantic_kernel_demo/register.py (1)
108-120: Fix return type of _response_fn (annotated str but returns dict)The function returns a dict consumed by the converter. Update the annotation to a mapping.
- async def _response_fn(input_message: str) -> str: + async def _response_fn(input_message: str) -> Mapping[str, str]:If not already imported per another comment, add:
+from collections.abc import AsyncIterator, Mapping, Sequence
♻️ Duplicate comments (3)
src/nat/agent/base.py (1)
191-191: Use logger.exception when not re-raising (align with PR convention).We’re returning an error ToolMessage (not re-raising). Switch to logger.exception to include the traceback and match the PR’s logging standard. This also addresses the earlier bot suggestion.
- logger.error("%s %s", AGENT_LOG_PREFIX, error_content, exc_info=True) + logger.exception("%s %s", AGENT_LOG_PREFIX, error_content)src/nat/eval/swe_bench_evaluator/evaluate.py (2)
116-119: Don’t emit a traceback for non-exceptional control flow; also fix return type mismatch.
- This is a guard, not an exception. Use logger.error (or warning) without a traceback.
- process_eval_input returns (None, None) along this path, but its signature is tuple[Path, Path]; adjust to Optional.
- if not supported_inputs: - logger.exception("No supported instances; nothing to evaluate") - return None, None + if not supported_inputs: + logger.error("No supported instances; nothing to evaluate") + return None, NoneOutside this hunk, update the signature to reflect Optional returns:
def process_eval_input(self, eval_input: EvalInput) -> tuple[Path | None, Path | None]: """Converts EvalInput into lists of SWEBenchInput and SWEBenchOutput models and applies filtering. Returns: Tuple of (workflow_input_file, workflow_output_file). Either may be None when there is nothing to evaluate. """
137-140: Remove exc_info=True; no active exception here.There’s no exception in flight; exc_info=True adds noise (“NoneType: None”). Log a plain error. The (None, None) return also relies on the signature fix noted above.
- if not filtered_outputs: - logger.error("No supported outputs; nothing to evaluate", exc_info=True) - return None, None + if not filtered_outputs: + logger.error("No supported outputs; nothing to evaluate") + return None, None
...evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.py
Show resolved
Hide resolved
...evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.py
Show resolved
Hide resolved
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
examples/evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.py (2)
215-217: Good: switched to logger.exception when swallowing.Resolves the earlier bot comment and keeps traceback without re-raising.
295-297: Good: use logger.exception when returning None.Traceback retained; aligns with repo-wide pattern and prior feedback.
🧹 Nitpick comments (5)
examples/evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.py (5)
288-294: Prefer ‘b/…’ path in diff --git to handle renames correctly.Using parts[2] picks the old path (a/…), which breaks on renames. Use parts[3] (b/…).
Apply:
- if line.startswith('diff --git'): - parts = line.split() - if len(parts) >= 3: - return parts[2][2:] # Remove 'b/' prefix + if line.startswith('diff --git'): + parts = line.split() + if len(parts) >= 4: + # Prefer new path (b/...), robust to renames + return parts[3][2:] # strip 'b/'
311-315: System prompt conflicts with user prompt; ask for full file, not “only changes.”Current wording risks partial outputs. Align with _build_fix_prompt requirements.
- "You are a skilled Python developer. Do not refactor code unnecessarily. Also, when printing output, print only the changes you made and not unchanged code. Generate only the fixed code without any explanations or formatting. " + "You are a skilled Python developer. Do not refactor code unnecessarily. Return the complete fixed file content only, with no explanations or markdown."
26-27: Avoid blocking the event loop: use AsyncOpenAI (or run sync call in a thread).current code calls the sync client inside an async function.
- from openai import OpenAI + from openai import AsyncOpenAIdef __init__(self, config: SweBenchWorkflowConfig, builder: Builder): super().__init__(config, builder) self.setup_predictor() - self.openai_client = OpenAI(api_key=config.predictor.openai_api_key) + self.openai_client = AsyncOpenAI(api_key=config.predictor.openai_api_key)- response = self.openai_client.chat.completions.create( + response = await self.openai_client.chat.completions.create( model="gpt-4o", messages=[{Also applies to: 62-66, 308-319
67-72: Add explicit return type for setup method.Minor clarity and typing consistency.
- def setup_predictor(self): + def setup_predictor(self) -> None:
73-77: Type the return of _parse_ast.Improves readability and static analysis.
- def _parse_ast(self, file_path: str): + def _parse_ast(self, file_path: str) -> tuple[list[dict], list[dict], str]:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
docs/source/extend/telemetry-exporters.md(1 hunks)examples/HITL/por_to_jiratickets/src/nat_por_to_jiratickets/extract_por_tool.py(3 hunks)examples/evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.py(4 hunks)src/nat/agent/react_agent/agent.py(6 hunks)src/nat/profiler/calc/calc_runner.py(2 hunks)src/nat/runtime/loader.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/source/extend/telemetry-exporters.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/nat/profiler/calc/calc_runner.py
- src/nat/runtime/loader.py
- examples/HITL/por_to_jiratickets/src/nat_por_to_jiratickets/extract_por_tool.py
- src/nat/agent/react_agent/agent.py
🧰 Additional context used
📓 Path-based instructions (7)
{src/**/*.py,packages/*/src/**/*.py,examples/*/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' only for env var prefixes and informal code comments (never in documentation)
Files:
examples/evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Format Python code with yapf (PEP8 base, column_limit=120) run second in the toolchain
Run ruff (ruff check --fix) using configuration in pyproject.toml; fix warnings unless explicitly ignored; ruff is a linter only (not for formatting)
Respect naming: snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Prefer httpx with SSL verification enabled by default for HTTP requests; follow OWASP Top-10
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 vectorized operations when beneficial
Files:
examples/evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.py
**/*.{py,md,sh,yml,yaml,toml,json,ini,cfg,txt,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces (no tabs) and ensure every file ends with a single trailing newline
Files:
examples/evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.py
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}: Every file must start with the standard SPDX Apache-2.0 header
All source files must include the SPDX Apache-2.0 header template
Files:
examples/evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived by setuptools-scm; never hard-code versions in code or documentation
Files:
examples/evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.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 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/evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.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/evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.py
🔇 Additional comments (2)
examples/evaluation_and_profiling/swe_bench/src/nat_swe_bench/predictors/predict_full/predict_full.py (2)
139-141: Re-raise pattern correctly applied (logger.error + bare raise).Stack trace preserved, no duplicate traceback. LGTM.
376-379: Good: logger.exception when returning an error string.Consistent with the agreed logging convention.
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
|
/merge |
2 participants
Description
Include clean-ups to ensure error reporting and propagating in a consistent pattern.
raisestatement to keep and propagate the stack trace, and uselogger.error()so that the stack trace is not printed in this layer (to avoid duplicate log)logger.exception()(which is an equivalence oflogger.error(exc_info=True)).coderabbit.yamlso this pattern is always checked at the coderabbit reviewCloses AIQ-1058
By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Bug Fixes
Observability
Tests
Documentation