Remove retired retriever pipeline CLI#2374
Conversation
Greptile SummaryThis PR removes the hidden
|
| Filename | Overview |
|---|---|
| nemo_retriever/src/nemo_retriever/cli/main.py | Removes the pipeline entry from the lazy subapp registry; one-line clean deletion. |
| nemo_retriever/src/nemo_retriever/tools/recall/core.py | Consolidates print_run_summary, _fmt_time, and _evaluation_metric_sort_key from the deleted detection_summary.py into this module; logic is identical to the original. |
| nemo_retriever/src/nemo_retriever/common/vdb/lancedb.py | Adds lancedb_row_count (moved from the deleted lancedb_read.py) and updates a docstring; all call sites are updated accordingly. |
| nemo_retriever/tests/test_ingest_service.py | New test file preserving the three ServiceIngestor tests migrated from the deleted test_pipeline_helpers.py; SPDX header present and tests are well-structured. |
| nemo_retriever/tests/test_recall_summary.py | New test file (renamed from test_detection_summary.py) covering metric sort order in print_run_summary; missing required SPDX license header. |
| nemo_retriever/tests/test_root_cli_workflow.py | Removes the pipeline --help callable assertion; adds pipeline to the parametrized list of commands that must be rejected as unknown. |
| nemo_retriever/src/nemo_retriever/harness/portal/app.py | Updates lancedb_row_count import path from deleted lancedb_read to the new location in lancedb; no logic change. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI["retriever CLI\n(cli/main.py)"] --> INGEST["retriever ingest"]
CLI --> QUERY["retriever query"]
CLI --> RECALL["retriever recall"]
CLI --> EVAL["retriever eval"]
CLI -. removed .-> PIPELINE["retriever pipeline\n❌ DELETED"]
INGEST --> SI["ServiceIngestor\n(ingest/service.py)"]
INGEST --> GI["GraphIngestor\n(ingestor/graph_ingestor.py)"]
SI --> VDB["IngestVdbOperator\n(operators/vdb.py)"]
GI --> VDB
VDB --> RECORDS["to_client_vdb_records\n(common/vdb/records.py)"]
RECALL --> CORE["print_run_summary\n(tools/recall/core.py)\n← moved from detection_summary.py"]
EVAL --> CORE
PORTAL["harness/portal/app.py"] --> LANCEDB_FN["lancedb_row_count\n(common/vdb/lancedb.py)\n← moved from lancedb_read.py"]
style PIPELINE fill:#ffcccc,stroke:#cc0000
style CORE fill:#ccffcc,stroke:#009900
style LANCEDB_FN fill:#ccffcc,stroke:#009900
%%{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"}}}%%
flowchart TD
CLI["retriever CLI\n(cli/main.py)"] --> INGEST["retriever ingest"]
CLI --> QUERY["retriever query"]
CLI --> RECALL["retriever recall"]
CLI --> EVAL["retriever eval"]
CLI -. removed .-> PIPELINE["retriever pipeline\n❌ DELETED"]
INGEST --> SI["ServiceIngestor\n(ingest/service.py)"]
INGEST --> GI["GraphIngestor\n(ingestor/graph_ingestor.py)"]
SI --> VDB["IngestVdbOperator\n(operators/vdb.py)"]
GI --> VDB
VDB --> RECORDS["to_client_vdb_records\n(common/vdb/records.py)"]
RECALL --> CORE["print_run_summary\n(tools/recall/core.py)\n← moved from detection_summary.py"]
EVAL --> CORE
PORTAL["harness/portal/app.py"] --> LANCEDB_FN["lancedb_row_count\n(common/vdb/lancedb.py)\n← moved from lancedb_read.py"]
style PIPELINE fill:#ffcccc,stroke:#cc0000
style CORE fill:#ccffcc,stroke:#009900
style LANCEDB_FN fill:#ccffcc,stroke:#009900
Reviews (5): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile
|
|
||
|
|
||
| def _make_df(rows: list[dict]) -> pd.DataFrame: | ||
| return pd.DataFrame(rows) |
There was a problem hiding this comment.
Direct IoU unit tests removed without replacement
The TestCalculateIou class (testing identical boxes → 1.0, no overlap → 0.0, zero-area boxes → 0.0, and partial overlap ratio) was deleted alongside deduplicate.py. The inlined _calculate_iou in dedup.py is now exercised only through the higher-level dedup_images call in test_drops_partial_overlap_above_threshold. Boundary conditions like identical boxes, non-overlapping boxes, and zero-area degenerate cases are no longer explicitly covered — a regression in the IoU formula would not be caught at the unit level.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemo_retriever/tests/test_dedup.py
Line: 20
Comment:
**Direct IoU unit tests removed without replacement**
The `TestCalculateIou` class (testing identical boxes → 1.0, no overlap → 0.0, zero-area boxes → 0.0, and partial overlap ratio) was deleted alongside `deduplicate.py`. The inlined `_calculate_iou` in `dedup.py` is now exercised only through the higher-level `dedup_images` call in `test_drops_partial_overlap_above_threshold`. Boundary conditions like identical boxes, non-overlapping boxes, and zero-area degenerate cases are no longer explicitly covered — a regression in the IoU formula would not be caught at the unit level.
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!
20ee19a to
67edeb2
Compare
jperez999
left a comment
There was a problem hiding this comment.
This change still supports the case when no metadata information about the table type or the model exists correct? It still works as expected in that case?
| @@ -1,129 +0,0 @@ | |||
| { | |||
There was a problem hiding this comment.
If we are deleting this file, we need to add a new file that explains how to run this evaluation with the harness.
| yield (source_id, page_number), meta, {} | ||
|
|
||
|
|
||
| def lancedb_row_count(uri: str, table_name: str) -> int: |
There was a problem hiding this comment.
am I reading this right? this file only has this method? if so, is it possible to move this to one of the other files in this common/vdb/ sub folder that still exist?
| from typing import Any, Dict, Optional | ||
|
|
||
|
|
||
| def _fmt_time(seconds: float) -> str: |
There was a problem hiding this comment.
Same as above, there are other metric logic functions, we should group them altogether, or is there a reason these should stay here?
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
67edeb2 to
a282c35
Compare
…-legacy-ray-config-cleanup Signed-off-by: Jacob Ioffe <jioffe@nvidia.com> # Conflicts: # docs/docs/extraction/starter-kits.md # nemo_retriever/README.md
Summary
Remove the hidden
retriever pipelinecompatibility command now that supported CLI workflows are owned byretriever ingestandretriever query.This PR:
examples.graph_pipelinecompatibility shim;ServiceIngestorcoverage from the mixed pipeline-helper test file intest_ingest_service.py;Scope boundary
This PR does not change workflow YAML, Dockerfiles or Docker scripts, Hugging Face token handling, tokenizer pre-download behavior, documentation build CI, Ray configuration snapshots, or any of the other cleanup candidates from the earlier version of this PR. Those can be evaluated independently in later PRs.
Compatibility boundary
This is a deliberate removal of the unsupported compatibility adapter, not the underlying Retriever implementation.
retriever ingestandretriever queryremain the supported CLI workflows.GraphIngestor,ServiceIngestor, graph execution, LanceDB upload/query, BEIR, recall, QA evaluation, and the benchmark harness remain.Tests
test_graph_pipeline_cli.pyis removed because it exclusively tested the deleted CLI.test_pipeline_helpers.pyandtest_beir_evaluation.py.ServiceIngestortests are retained intest_ingest_service.py.pipelineis not callable, whileingestandqueryremain available.Validation
uvx pre-commit run --all-files: all hooks passed.retriever ingest --helpandretriever query --helpsucceed;retriever pipeline --helpis rejected as an unknown command.Reviewer decision requested
Please confirm that the hidden
retriever pipelinecompatibility command and its adapter-only support code are not compatibility surfaces we intend to maintain.Checklist