LLM Based Task#2283
Conversation
Signed-off-by: Kyle Zheng <126034466+KyleZheng1284@users.noreply.github.com>
Greptile SummaryThis PR introduces a reusable
|
| Filename | Overview |
|---|---|
| nemo_retriever/src/nemo_retriever/operators/generation/base.py | Core TextGenerationOperator: well-structured thread-pool processing with good security practices, but the outer except Exception: in process (line 307) silently reclassifies logic-level RuntimeErrors as generic request_error without a traceback, making position-tracking bugs invisible. |
| nemo_retriever/src/nemo_retriever/common/params/models.py | Adds LLMSamplingOverrides, TextGenerationParams, and credential-provenance tracking to _ParamsModel; API-key env-reference serialization and model_fields_set-aware serializer look correct; making LLMInferenceParams.temperature Optional is backward-compatible since callers must opt in explicitly. |
| nemo_retriever/src/nemo_retriever/models/llm/tasks/base.py | GenerationTask/GenerationTaskError lifecycle is clean; every exception phase is type-isolated; execute() compatibility facade correctly suppresses provider text; no new issues beyond the previously flagged silent swallow. |
| nemo_retriever/src/nemo_retriever/tools/evaluation/generation.py | QAGenerationOperator correctly adapts legacy LLMClient via _execute_task, preserves flat constructor kwargs for graph workers, and dispatches TextCompletionClient over LLMClient; reasoning_enabled forwarding to legacy path is omitted (flagged in prior review thread). |
| nemo_retriever/src/nemo_retriever/graph/graph_pipeline_registry.py | Significant expansion with API-key env-reference serialization, v2 graph format, cycle detection, and opaque-mapping rejection; logic is thorough and well-tested; no new issues found. |
| nemo_retriever/src/nemo_retriever/models/llm/clients/litellm.py | LiteLLMClient.complete enforces text-only contract, validates extra_params, handles no-auth/env-ref API keys, and properly surfaces temperature+top_p provider errors; no issues found. |
| nemo_retriever/tests/test_generation_tasks.py | Comprehensive tests covering happy paths, error paths, graph round-trips, credential redaction, sampling overrides, legacy client adaptation, and concurrency controls; well-structured and isolated. |
| nemo_retriever/tests/test_generation_hardening.py | Security-focused tests verifying transport secret sanitization, immutable task/result contracts, and worker reconstruction; thorough coverage of the strict lifecycle. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller
participant TextGenerationOperator
participant ThreadPool
participant _execute_row
participant GenerationTask
participant TextCompletionClient
Caller->>TextGenerationOperator: process(DataFrame)
TextGenerationOperator->>TextGenerationOperator: _validate_and_resolve_dataframe()
loop per row (parallel)
TextGenerationOperator->>ThreadPool: submit(_execute_row, position, inputs)
ThreadPool->>_execute_row: run
_execute_row->>GenerationTask: "invoke(client, **inputs)"
GenerationTask->>GenerationTask: _preflight_error()
GenerationTask->>GenerationTask: "build_request(**inputs)"
GenerationTask->>TextCompletionClient: complete(messages, max_tokens, extra_params)
TextCompletionClient-->>GenerationTask: (raw_text, latency_s)
GenerationTask->>GenerationTask: parse(raw_text)
GenerationTask-->>_execute_row: GeneratedTextResult
_execute_row-->>ThreadPool: (position, result)
end
ThreadPool-->>TextGenerationOperator: as_completed futures
TextGenerationOperator->>TextGenerationOperator: write output columns
TextGenerationOperator-->>Caller: DataFrame with generated columns
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Caller
participant TextGenerationOperator
participant ThreadPool
participant _execute_row
participant GenerationTask
participant TextCompletionClient
Caller->>TextGenerationOperator: process(DataFrame)
TextGenerationOperator->>TextGenerationOperator: _validate_and_resolve_dataframe()
loop per row (parallel)
TextGenerationOperator->>ThreadPool: submit(_execute_row, position, inputs)
ThreadPool->>_execute_row: run
_execute_row->>GenerationTask: "invoke(client, **inputs)"
GenerationTask->>GenerationTask: _preflight_error()
GenerationTask->>GenerationTask: "build_request(**inputs)"
GenerationTask->>TextCompletionClient: complete(messages, max_tokens, extra_params)
TextCompletionClient-->>GenerationTask: (raw_text, latency_s)
GenerationTask->>GenerationTask: parse(raw_text)
GenerationTask-->>_execute_row: GeneratedTextResult
_execute_row-->>ThreadPool: (position, result)
end
ThreadPool-->>TextGenerationOperator: as_completed futures
TextGenerationOperator->>TextGenerationOperator: write output columns
TextGenerationOperator-->>Caller: DataFrame with generated columns
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
nemo_retriever/src/nemo_retriever/operators/generation/base.py:297-309
**Logic-bug exceptions silently reclassified as `request_error`**
The outer `except Exception:` in `process` catches all exceptions — including the explicitly raised `RuntimeError` when `result_position != position` (a programming-error invariant) — without a traceback or the caught exception bound to a name. If that invariant ever fires, the error would appear in logs only as `"Row N generation failed (request_error)"` with no indication that an internal position-tracking bug caused it, violating the `no-bare-except` rule that requires `exc_info=True` at every broad handler. The future-result loop is already at a pipeline boundary, so using `except Exception as exc:` with `exc_info=True` is the correct fix.
Reviews (2): Last reviewed commit: "Harden text generation abstraction and g..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _failure_model(self) -> str: | ||
| try: | ||
| model = self._client.model | ||
| except Exception: | ||
| return self._configured_model | ||
| return model if isinstance(model, str) and model else self._configured_model |
There was a problem hiding this comment.
except Exception without logging
_failure_model is not a defined boundary (not a FastAPI handler, CLI entry point, Ray actor loop, or top-level pipeline handler), so the no-bare-except rule applies strictly. Any exception that occurs while reading self._client.model is silently discarded, which hides misconfigured clients and makes failures hard to diagnose.
| def _failure_model(self) -> str: | |
| try: | |
| model = self._client.model | |
| except Exception: | |
| return self._configured_model | |
| return model if isinstance(model, str) and model else self._configured_model | |
| def _failure_model(self) -> str: | |
| try: | |
| model = self._client.model | |
| except Exception: | |
| logger.debug("Could not read client model name, falling back to configured model", exc_info=True) | |
| return self._configured_model | |
| return model if isinstance(model, str) and model else self._configured_model |
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/operators/generation/base.py
Line: 238-243
Comment:
**Bare `except Exception` without logging**
`_failure_model` is not a defined boundary (not a FastAPI handler, CLI entry point, Ray actor loop, or top-level pipeline handler), so the `no-bare-except` rule applies strictly. Any exception that occurs while reading `self._client.model` is silently discarded, which hides misconfigured clients and makes failures hard to diagnose.
```suggestion
def _failure_model(self) -> str:
try:
model = self._client.model
except Exception:
logger.debug("Could not read client model name, falling back to configured model", exc_info=True)
return self._configured_model
return model if isinstance(model, str) and model else self._configured_model
```
How can I resolve this? If you propose a fix, please make it concise.| if isinstance(client, LLMClient): | ||
| result = client.generate(inputs["query"], inputs["chunks"]) | ||
| return GeneratedTextResult( |
There was a problem hiding this comment.
reasoning_enabled is not forwarded to legacy generate() callers
When a legacy LLMClient (one that only exposes generate, not complete) is injected, the adapter silently drops the operator-level reasoning_enabled. A caller who constructs QAGenerationOperator(reasoning_enabled=False) and injects a legacy client will find that reasoning is not disabled — the legacy client receives reasoning_enabled=None and falls back to its own transport default (True). The LLMClient.generate protocol already accepts reasoning_enabled as a keyword argument, so it can be forwarded without breaking anything.
| if isinstance(client, LLMClient): | |
| result = client.generate(inputs["query"], inputs["chunks"]) | |
| return GeneratedTextResult( | |
| if isinstance(client, LLMClient): | |
| task_reasoning = getattr(self._task, "reasoning_enabled", None) | |
| result = client.generate(inputs["query"], inputs["chunks"], reasoning_enabled=task_reasoning) | |
| return GeneratedTextResult( |
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/tools/evaluation/generation.py
Line: 95-97
Comment:
**`reasoning_enabled` is not forwarded to legacy `generate()` callers**
When a legacy `LLMClient` (one that only exposes `generate`, not `complete`) is injected, the adapter silently drops the operator-level `reasoning_enabled`. A caller who constructs `QAGenerationOperator(reasoning_enabled=False)` and injects a legacy client will find that reasoning is *not* disabled — the legacy client receives `reasoning_enabled=None` and falls back to its own transport default (`True`). The `LLMClient.generate` protocol already accepts `reasoning_enabled` as a keyword argument, so it can be forwarded without breaking anything.
```suggestion
if isinstance(client, LLMClient):
task_reasoning = getattr(self._task, "reasoning_enabled", None)
result = client.generate(inputs["query"], inputs["chunks"], reasoning_enabled=task_reasoning)
return GeneratedTextResult(
```
How can I resolve this? If you propose a fix, please make it concise.| # SPDX-FileCopyrightText: Copyright (c) 2024-26, NVIDIA CORPORATION & AFFILIATES. | ||
| # All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
The tasks/ package and operator files use three different year formats: 2024-26 (non-standard shorthand), 2024-25, and 2026 (standalone). The spdx-license-header rule requires the current year. New files in this PR use 2024-26 (tasks/__init__.py, tasks/base.py, tasks/generic.py, tasks/rag_answer.py, tasks/summarize.py) while test_generation_tasks.py uses 2026. The standard form in the rest of the repo is the four-digit year; the abbreviated 2024-26 is not conventional. All new files should use the same format as the existing codebase.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/models/llm/tasks/__init__.py
Line: 1-3
Comment:
**SPDX copyright year format inconsistency across new files**
The `tasks/` package and operator files use three different year formats: `2024-26` (non-standard shorthand), `2024-25`, and `2026` (standalone). The `spdx-license-header` rule requires the current year. New files in this PR use `2024-26` (`tasks/__init__.py`, `tasks/base.py`, `tasks/generic.py`, `tasks/rag_answer.py`, `tasks/summarize.py`) while `test_generation_tasks.py` uses `2026`. The standard form in the rest of the repo is the four-digit year; the abbreviated `2024-26` is not conventional. All new files should use the same format as the existing codebase.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Signed-off-by: Kyle Zheng <126034466+KyleZheng1284@users.noreply.github.com>
| for future in as_completed(futures): | ||
| position = futures[future] | ||
| try: | ||
| result_position, result = future.result() | ||
| if result_position != position: | ||
| raise RuntimeError( | ||
| f"generation result position {result_position} does not match " | ||
| f"submitted position {position}" | ||
| ) | ||
| results[position] = result | ||
| except Exception: | ||
| logger.warning("Row %d generation failed (request_error)", position) | ||
| results[position] = self._failure_result("request_error", 0.0) |
There was a problem hiding this comment.
request_error
The outer except Exception: in process catches all exceptions — including the explicitly raised RuntimeError when result_position != position (a programming-error invariant) — without a traceback or the caught exception bound to a name. If that invariant ever fires, the error would appear in logs only as "Row N generation failed (request_error)" with no indication that an internal position-tracking bug caused it, violating the no-bare-except rule that requires exc_info=True at every broad handler. The future-result loop is already at a pipeline boundary, so using except Exception as exc: with exc_info=True is the correct fix.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/operators/generation/base.py
Line: 297-309
Comment:
**Logic-bug exceptions silently reclassified as `request_error`**
The outer `except Exception:` in `process` catches all exceptions — including the explicitly raised `RuntimeError` when `result_position != position` (a programming-error invariant) — without a traceback or the caught exception bound to a name. If that invariant ever fires, the error would appear in logs only as `"Row N generation failed (request_error)"` with no indication that an internal position-tracking bug caused it, violating the `no-bare-except` rule that requires `exc_info=True` at every broad handler. The future-result loop is already at a pipeline boundary, so using `except Exception as exc:` with `exc_info=True` is the correct fix.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Description
Checklist