Skip to content

add checks against service mode params#2148

Merged
jperez999 merged 5 commits into
NVIDIA:26.05from
jperez999:fix-cli-params-service-mode
May 28, 2026
Merged

add checks against service mode params#2148
jperez999 merged 5 commits into
NVIDIA:26.05from
jperez999:fix-cli-params-service-mode

Conversation

@jperez999
Copy link
Copy Markdown
Collaborator

Description

This PR fixes issues where the user is able to provide specific arguments via cli and they are ignored in the service run mode. Now, when an argument that is not accepted by the service is added to a cli command for service mode it will raise an error.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • If adjusting docker-compose.yaml environment variables have you ensured those are mimicked in the Helm values.yaml file.

@jperez999 jperez999 self-assigned this May 28, 2026
@jperez999 jperez999 requested review from a team as code owners May 28, 2026 14:52
@jperez999 jperez999 requested review from charlesbluca and removed request for a team May 28, 2026 14:52
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 28, 2026

Greptile Summary

This PR adds an explicit validation gate for --run-mode=service that rejects any CLI flags that configure the local ingest graph but are silently ignored by ServiceIngestor. A new module-level constant _SERVICE_INCOMPATIBLE_FLAGS enumerates every such flag, and _reject_service_incompatible_flags checks ctx.get_parameter_source at command entry to raise typer.BadParameter with a clear remediation message.

  • Introduces _SERVICE_INCOMPATIBLE_FLAGS tuple (~90 flag/param pairs) covering extraction, embedding, Ray tuning, VDB/sidecar, audio, video, and caption flags.
  • Adds _reject_service_incompatible_flags(ctx) called early in run() (before any graph work) and removes the now-meaningful _ = ctx stub.
  • Adds four new test functions covering rejection of single flags, rejection of multiple simultaneous flags, VDB-specific flags (--no-vdb, --vdb-overwrite, --vdb-append), and a happy-path allowlist test that monkeypatches ServiceIngestor.

Confidence Score: 5/5

Safe to merge. The validation logic is correct and fires before any graph work begins; no existing behavior is changed for batch or inprocess runs.

The new guard is purely additive — it rejects previously silently-dropped flags and leaves all other code paths untouched. The ParameterSource comparison-by-name pattern correctly handles the vendored-enum divergence between Typer and Click. The finally cleanup in run() executes correctly when typer.BadParameter propagates. The test suite exercises rejection, multi-flag rejection, VDB-specific flags, and the happy-path allowlist, giving solid coverage of the new gate.

No files require special attention.

Important Files Changed

Filename Overview
nemo_retriever/src/nemo_retriever/pipeline/main.py Adds _SERVICE_INCOMPATIBLE_FLAGS constant and _reject_service_incompatible_flags helper; removes the _ = ctx placeholder and wires the check early in run(). Logic is sound: ParameterSource comparison is done by name to handle vendored-enum divergence, and typer.BadParameter propagates cleanly through the finally cleanup block.
nemo_retriever/tests/test_graph_pipeline_cli.py Four new test functions added. test_graph_pipeline_cli_service_mode_rejects_vdb_flags bundles three independent flag invocations in one function, violating the one-behavior-per-test rule. The _FakeServiceIngestor stub is not spec-constrained against the real class, so interface drift won't be caught.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[CLI: retriever pipeline run] --> B{--run-mode?}
    B -- batch/inprocess --> D[Proceed normally]
    B -- service --> C[_reject_service_incompatible_flags ctx]
    C --> E{Any flag with source COMMANDLINE or ENVIRONMENT?}
    E -- yes --> F[raise typer.BadParameter list incompatible flags + remediation]
    E -- no --> G[Continue run function]
    G --> H[Build ServiceIngestor]
    H --> I[ingestor.ingest]
    I --> J[Collect results VDB write is server-side]
    F --> K[Exit code 2 User sees error message]
    D --> L[Build GraphIngestor batch or inprocess]
    L --> M[ingestor.ingest local VDB upload if enabled]
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
nemo_retriever/tests/test_graph_pipeline_cli.py:524-548
**Multiple independent scenarios in one test function**

`test_graph_pipeline_cli_service_mode_rejects_vdb_flags` exercises three distinct flag inputs (`--no-vdb`, `--vdb-overwrite`, `--vdb-append`) inside a single function. If the first `RUNNER.invoke` fails an assertion, the other two cases are silently skipped, making it harder to see which scenario actually broke. Per the project's test standards, each scenario should have its own function: `test_graph_pipeline_cli_service_mode_rejects_no_vdb_flag`, `test_graph_pipeline_cli_service_mode_rejects_vdb_overwrite_flag`, and `test_graph_pipeline_cli_service_mode_rejects_vdb_append_flag`.

### Issue 2 of 2
nemo_retriever/tests/test_graph_pipeline_cli.py:559-574
**Stub not constrained to `ServiceIngestor`'s interface**

`_FakeServiceIngestor` inherits from `list` with no relationship to the real `ServiceIngestor`. If `ServiceIngestor` gains or renames a method called inside the service-mode path of `run()` (e.g., `.embed()`, `.ingest()`), this stub will still satisfy any call and the test will keep passing while the real code path has changed. Prefer `unittest.mock.create_autospec(ServiceIngestor)` (or a `MagicMock(spec=ServiceIngestor)`) configured with the needed return values so that API drift is caught at test time.

Reviews (4): Last reviewed commit: "fix bug in env error" | Re-trigger Greptile

Comment thread nemo_retriever/src/nemo_retriever/pipeline/__main__.py
Comment thread nemo_retriever/src/nemo_retriever/pipeline/__main__.py
# everything in I/O, Service, Evaluation, Observability panels
_SERVICE_INCOMPATIBLE_FLAGS: tuple[tuple[str, str], ...] = (
# Extract
("--method", "method"),
Copy link
Copy Markdown
Collaborator

@charlesbluca charlesbluca May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing before this goes green: the CI fixes above make the tests behave as intended, but this reject list still looks too broad after the rebase.

Current 26.05 has coverage asserting service mode forwards method, extract_text, dpi, split_config, and embed_granularity into the ServiceIngestor payload.

So flags like --method, --dpi, --text-chunk*, and --embed-granularity should probably not be rejected anymore unless this PR is intentionally reverting that service override support. VDB flags still look valid to reject.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this is relevant still? Are the above flags not getting used in service-mode?

Comment thread nemo_retriever/src/nemo_retriever/pipeline/__main__.py Outdated
Comment thread nemo_retriever/tests/test_graph_pipeline_cli.py
Comment thread nemo_retriever/tests/conftest.py
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 28, 2026

Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance.

@jperez999 jperez999 merged commit db2c87d into NVIDIA:26.05 May 28, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants