Skip to content

refactor: reduce configuration sprawl (dead run-mode/chunking config + unify LanceDB table name)#2297

Draft
jdye64 wants to merge 4 commits into
mainfrom
cursor/reduce-config-sprawl-6355
Draft

refactor: reduce configuration sprawl (dead run-mode/chunking config + unify LanceDB table name)#2297
jdye64 wants to merge 4 commits into
mainfrom
cursor/reduce-config-sprawl-6355

Conversation

@jdye64

@jdye64 jdye64 commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Description

Incremental reduction of configuration sprawl in the repo. Grouped into logical commits; each addresses a specific finding from the configuration audit.

1. Remove dead ModelRunMode run-mode vocabulary

ModelRunMode = "local" | "NIM" | "build-endpoint" was dead metadata: every BaseModel subclass hard-coded model_runmode → "local", and .model_runmode was only ever read in tests. Real local/NIM/hosted selection happens per-stage in operators/operator_archetype.py via *_invoke_url presence + GPU detection. Removed the type alias, the abstract property on BaseModel, and its 10 concrete overrides; tests updated. This removes one of the four competing "run mode" vocabularies. (Report suggestion #2)

2. Remove broken common/params/chunk_bundle.py

Imported TextChunkParamsByType (a symbol that does not exist), so it raised ImportError if ever loaded; nothing referenced it. Leftover of an incomplete refactor and a redundant third way to express chunking. (Report suggestion #24)

3. Unify the default LanceDB table name behind one constant

The default table name was spelled three different ways: nv-ingest (params, graph retriever, vdb operators/stage/bulk, harness config), nemo-retriever (ingest plan, query options, CLIs, harness resolution), and nemo_retriever (service vectordb + Helm). A default ingest and a default query could therefore silently target different tables — a latent bug.

Introduced common.vdb.DEFAULT_LANCEDB_TABLE_NAME = "nemo-retriever" (matching the user-facing ingest/query CLI defaults, which are unchanged) and routed every default through it: LanceDbParams, IngestStorageOptions, QueryStorageOptions, the graph Retriever fallback, the LanceDB operator/stage/bulk writers, the ingest & query CLIs, recall tooling, service VectorDbConfig/vectordb_app/Helm values, harness config/resolution, and benchmark_registry.DEFAULT_TABLE_NAME. (Report suggestion #6)

⚠️ Migration note: this changes the service vectordb default table name from nemo_retriever to nemo-retriever. Existing Helm/service deployments that relied on the old default should set serviceConfig.vectordb.tableName explicitly to keep reading pre-existing data. Evaluation tools that deliberately default to nv-ingest (pre-ingested benchmark tables) are intentionally left unchanged.

Intentionally NOT changed

  • local_ingest_embed_backend default (vllm for ingest vs hf for query) — investigated and left as-is. This divergence is intentional (vLLM for batch-ingest throughput vs HF for low-latency single-query embedding); forcing them equal would likely regress one path.

Validation

  • Base package installs; all changed modules import with no cycles; all defaults resolve to nemo-retriever.
  • Passing: test_root_cli_workflow, test_root_query_cli, test_query_workflow_options, test_ingest_empty_validation, test_type_aliases, test_beir_evaluation, test_evaluation_retrievers, test_nv_ingest_vdb_operator, test_service_vectordb_app, test_service_vectordb_evidence_integration, test_helm_vectordb_embed_required, test_harness_agentic_eval, test_split_config* (170+ tests). GPU/[local]-extra tests were compile-checked only (torch not available in CI env).

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
Open in Web Open in Cursor 

cursoragent and others added 3 commits July 6, 2026 14:02
chunk_bundle.py imported TextChunkParamsByType from common.params.models,
a symbol that does not exist, so the module raised ImportError if ever
loaded. Nothing referenced coalesce_text_chunk_bundle. Removing it drops a
broken, redundant chunking-config path (report suggestion: consolidate the
three ways chunking is currently expressed).

Co-authored-by: Jeremy Dyer <jdye64@gmail.com>
ModelRunMode ("local" | "NIM" | "build-endpoint") and the abstract
model_runmode property were dead metadata: every BaseModel subclass hard
-coded "local" and the property was only ever read in tests. Real
local/NIM/hosted selection happens in operators/operator_archetype.py via
*_invoke_url presence and GPU detection, not on the model classes.

Removing it eliminates one of the four competing "run mode" vocabularies
in the repo (report suggestion #2). Updated tests to assert the alias is
gone instead of checking its members.

Co-authored-by: Jeremy Dyer <jdye64@gmail.com>
The default LanceDB table name was spelled three different ways across the
codebase: "nv-ingest" (params, graph retriever, vdb operators/stage/bulk,
harness config), "nemo-retriever" (ingest plan, query options, CLIs,
harness resolution), and "nemo_retriever" (service vectordb + Helm). A
default ingest and a default query could therefore silently target
different tables.

Introduce a single canonical constant
common.vdb.DEFAULT_LANCEDB_TABLE_NAME = "nemo-retriever" (matching the
user-facing ingest/query CLI defaults, which are unchanged) and route every
default through it: LanceDbParams, IngestStorageOptions, QueryStorageOptions,
graph Retriever fallback, LanceDB operator/stage/bulk, ingest & query CLIs,
recall tooling, service VectorDbConfig + vectordb_app + Helm values,
harness config/resolution, and benchmark_registry.DEFAULT_TABLE_NAME.

Implements report suggestion #6. Note: this changes the service vectordb
default table name from 'nemo_retriever' to 'nemo-retriever'; existing
Helm/service deployments that relied on the old default will point at a new
table and should set serviceConfig.vectordb.tableName explicitly to keep
reading pre-existing data. Evaluation tools that deliberately default to
'nv-ingest' pre-ingested benchmark tables are intentionally left unchanged.

Co-authored-by: Jeremy Dyer <jdye64@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@cursor cursor Bot changed the title refactor: reduce configuration sprawl (remove dead run-mode + chunking config paths) refactor: reduce configuration sprawl (dead run-mode/chunking config + unify LanceDB table name) Jul 6, 2026
Removing the test_model_runmode methods left test_model_type directly
adjacent to test_input_batch_size, which black flags. Re-add the
PEP8/black-required blank line between the methods.

Co-authored-by: Jeremy Dyer <jdye64@gmail.com>
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.

2 participants