fix(pipelines): align incremental store schema guard with the full run

[abhishekKokadwar]

Why

store_milvus (full pipeline) and store_milvus_incremental both write into the same kubeflow_docs collection, but only the full one honoured embedding_dim or verified an existing collection's schema.

store_milvus_incremental took no embedding_dim parameter at all and hardcoded dim=768. Point the stack at a non-768 TEI model and the two components disagree: whichever runs first fixes the dimension for the other, and the incremental run inserts vectors of the wrong width into a collection it silently created at 768 — with no guard to catch it, unlike its sibling.

This is a port, not a new design. The guard already exists in store_milvus and is copied field-for-field, so both components now accept and reject exactly the same schemas.

What

File	Change
pipelines/incremental-pipeline.py	embedding_dim on the component and the pipeline (default DEFAULT_EMBEDDING_DIM); created vector field uses it; versioned schema-compatibility check ported from store_milvus
pipelines/kubeflow-pipeline.py, pipelines/code-pipeline.py	nlist floored at 32
pipelines/README.md	embedding_dim row in the incremental parameter table
.github/workflows/oke-cicd.yaml	KFP-compile incremental-pipeline.py
tests/test_incremental_pipeline.py	New — first test coverage for this pipeline

Why the nlist change

Milvus recommends nlist in [32, 4096]. min(1024, len(records)) builds a near-degenerate index on a small first run. The incremental and issues pipelines already carry max(100, ...) and max(16, ...) floors, so this only brings the two remaining copies in line.

To be precise about severity: this is latent, not a live crash. create_index sits inside if records:, so nlist=0 is unreachable, and a real docs run indexes thousands of chunks and gets 1024 anyway.

Verification

Written test-first. Against main the new tests fail with:

TypeError: store_milvus_incremental() got an unexpected keyword argument 'embedding_dim'

After the change all three pass. I also mutation-checked that they actually bite rather than passing either way:

Mutation	Result
dim=embedding_dim reverted to dim=768	fails assert 768 == 1024
vector_dim != embedding_dim dropped from the guard	mismatch test fails

• pytest: 176 passed on this branch vs 173 on main, same environment with the KFP SDK installed — 3 new tests, 0 regressions
• All four pipelines KFP-compile: kubeflow-pipeline.py, issues-pipeline.py, code-pipeline.py, incremental-pipeline.py
• python -m compileall over all six paths the Python compile check job uses: clean
• Compiled .yaml artifacts are gitignored; none committed
• DCO: signed off

One caveat, stated rather than glossed over: ruff could not execute in my environment (Windows Application Control policy blocks the binary — module, executable, and shell invocation all refused). I verified with pyflakes instead, which covers ruff's selected F rules: zero findings on the new test file. The only hits anywhere are pre-existing F403/F405 star imports in pipelines/*.py, which pyproject.toml already ignores for that path. pycodestyle --select=E4,E7,E9 is also clean and my longest added line is 114 characters against the 120 limit. CI runs the real thing and will confirm.

Relationship to #100

#100 proposed this same idea and its description lists store_milvus_incremental among its changes. Worth flagging clearly so this is not read as duplicate work:

• Its diff contains only kubeflow-pipeline.py — the incremental change it describes was never included
• The store_milvus half it does contain has already landed upstream via fix: stop A2A answers truncating at 30s, and ground/cite agent replies #234, which is where the embedding_dim parameter this PR matches came from
• It targets base 6b4caba (2026-03-02), now 15 commits behind, and edits pipelines/..., a directory that no longer exists since the move to docs-agent-mcp/pipelines/

So the remaining gap was the incremental component, which is what this PR closes. I kept upstream's embedding_dim spelling rather than #100's embedding_dimension for consistency with store_milvus. Happy to close this if the author would prefer to refresh #100 instead.

Notes for reviewers

• Where the new tests run. They need the KFP SDK, so PR Safety skips them (it installs only requirements-test.txt), exactly as it already skips the existing component tests. Build, Test, and Deploy to OKE installs kfp kfp-kubernetes and runs pytest on every pull request, so they do execute there. Happy to add kfp to requirements-test.txt if you would rather both jobs cover it.
• Why the CI compile step is included. The compile step covered three of the four pipelines and skipped incremental-pipeline.py. Since this PR modifies that file, CI would otherwise have gone green without ever compiling the change. fix(pipelines): cite published docs URLs and align the incremental cleaner with the full run #244 flagged the same gap independently.
• Scope. store_milvus_incremental was explicitly left out of scope by fix(pipelines): cite published docs URLs and align the incremental cleaner with the full run #244 ("still hardcodes dim=768 and has no schema check, unlike store_milvus"). This closes that and deliberately touches nothing else in the file. The ZeroDivisionError in the same component's chunk-count print is left to bug(pipelines): ZeroDivisionError in chunk_and_embed_incremental when text splitter returns empty chunks #163/fix(pipelines): guard against ZeroDivisionError in chunk_and_embed_incremental when chunk list is empty #164.
• Likely conflict. fix(pipelines): cite published docs URLs and align the incremental cleaner with the full run #244 also adds a line to the oke-cicd.yaml compile step. One-line overlap, trivial in either order — happy to drop mine if fix(pipelines): cite published docs URLs and align the incremental cleaner with the full run #244 lands first.
• Rollout. No migration needed. Behaviour is unchanged at the default embedding_dim=768: the guard accepts any collection the full pipeline created, and the new default matches what was previously hardcoded. The dimension only becomes configurable for operators who switch embedding models.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign chasecadet for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[abhishekKokadwar]

Rebased onto dedbc84 (#249) as offered. Two things changed materially, and I found a separate breakage on main while verifying.

main is currently red — three test modules abort at collection

ImportError: cannot import name 'clean_content' from 'utils' (unknown location)
ERROR tests/test_code_utils.py
ERROR tests/test_issues_pipeline.py
ERROR tests/test_pipeline_utils.py
!!!!!! Interrupted: 3 errors during collection !!!!!!

pytest exits 2 on pristine dedbc84, so the suite stops before running anything and both workflows that call it fail.

Same root cause as the import problem in #249: utils.py moved into pipelines/utils/, and these modules put only pipelines/ on sys.path, where the sibling directory of the same name shadows the module as a namespace package:

>>> sys.path.insert(0, ".../docs-agent-mcp/pipelines"); import utils
>>> utils.__file__
None
>>> utils.__path__
_NamespacePath(['.../docs-agent-mcp/pipelines/utils'])

So no attribute resolves. The pipelines already solve this with their _UTILS_DIR entry; efad4a4 applies the same one-line addition to the three test modules. That recovers 72 tests across the four affected files.

Worth saying plainly: 7 failures and 63 errors remain, and they are pre-existing on dedbc84 — I verified them on a clean checkout and deliberately left them alone. They look like fallout from the same refactor:

• test_mcp_server.py (63 errors) — AttributeError: module 'docs_agent_mcp_server' has no attribute 'client'; client went away when feat(rag): hybrid docs retrieval and readable ingest path #249 split the MCP server into router/search/citations
• test_docs_pipeline.py (4 failures) — exercises the store_milvus component feat(rag): hybrid docs retrieval and readable ingest path #249 replaced with utils/milvus_store.py
• test_widget_markdown.py (3 failures) — widget changes in the same PR

Happy to fix those too, in a separate PR, if that's wanted — it's a bigger change and belongs with whoever owns the refactor.

What I dropped from this PR

The nlist floor is gone. #249 switched both pipelines to index_type: "FLAT", where nlist has no meaning, so the fix is obsolete. (Both files do still pass a now-inert nlist param — cosmetic, not worth a commit here.)

The kubeflow-pipeline.py and README.md changes are also gone: store_milvus and its schema guard moved into utils/milvus_store.py, so there was nothing left to align there.

What remains, and why it is now a stronger case

Only store_milvus_incremental — and after #249 the divergence is wider than a dimension mismatch. The full pipeline writes the v4 hybrid schema; the incremental component still writes the v1 one, into the same kubeflow_docs collection:

v4 requires, incremental never supplies	document_id, sparse_vector, title, section_path, doc_type, version, release_date
incremental inserts, v4 has no such field	file_unique_id, repo_name, file_name, last_updated

The schemas are now disjoint rather than merely mismatched, and sparse_vector is the field milvus_search.py needs for BM25 — collection_has_bm25() gates hybrid search on its presence. So if the incremental pipeline ever created the collection, hybrid retrieval would silently degrade to dense-only.

With the guard in this PR, running it against the live v4 collection fails fast instead:

Schema version mismatch for kubeflow_docs. Expected compatible v=1;
description='RAG lean hybrid collection for documentation (v=4, hybrid=bm25+dense)' ...

That is the right outcome — it must not write v1 rows into a v4 collection — but it does mean this PR makes explicit that the incremental pipeline no longer has a valid target. Two reasonable directions, and I would rather have a maintainer pick than guess:

1. Land this as a guard. The component fails loudly instead of corrupting the collection, and the v4 rewrite is a follow-up.
2. Retire it. legacy/pipelines/incremental-pipeline.py already exists as a copy; if the live one is not meant to be v4-aware, deleting it is simpler than guarding it.

I have no stake in which — happy to convert this to the deletion if that is the call.

Verification

• pytest tests/test_incremental_pipeline.py — 3 passed; still fails on main without the fix
• The three repaired modules plus mine — 72 passed
• incremental-pipeline.py KFP-compiles, along with issues- and code-. kubeflow-pipeline.py needs pymilvus locally, which I do not have installed; it is unmodified by this PR
• DCO signed on both commits

Still blocked on CI

Workflows are still awaiting maintainer approval, so none of this has been confirmed by CI — including ruff, which I still cannot run locally (Application Control policy blocks the binary). Given main is red at the moment, approving the workflows here would also show whether efad4a4 turns the suite green.

[abhishekKokadwar]

Closing this, with a correction to my previous comment.

The v1/v4 schema mismatch is already documented, and I overstated it. #249 records it in two places:

• docs/RAG_V4_ARCHITECTURE.md: incremental pipeline "Still legacy dense schema; full rebuild via kubeflow-pipeline.py is the v4 path"
• pipelines/README.md: "It predates v4 hybrid schema and is kept for reference only; use kubeflow-pipeline.py for production docs indexing."

So it's a known, intentional limitation, not a new finding. Two claims above were also wrong:

• "Hybrid retrieval would silently degrade" is only true if the incremental pipeline creates kubeflow_docs from scratch. Against the existing v4 collection, an insert fails on the missing fields, so the failure is loud, not silent.
• "No valid target" was presented as a discovery when the README already says so.

Given that, a schema guard for a pipeline you've designated reference-only isn't worth review time, so I'm withdrawing it.

The one part that did matter is split out as #258: since #249, pytest on main aborts at collection (exit 2) because utils/ shadows utils.py as a namespace package. That PR is test-only, +9 lines, and is explicit that the suite stays red afterwards, from the pre-existing test_mcp_server.py / test_docs_pipeline.py / test_widget_markdown.py failures. Also a small correction: I said 72 recovered tests above. That count included this PR's own 3 tests. The accurate figure for the fix alone is 69.

One small docs/tree inconsistency, in case it's useful: pipelines/README.md says the incremental pipeline lives under legacy/pipelines/, but b92f87c restored a copy at docs-agent-mcp/pipelines/incremental-pipeline.py, which still defaults to collection_name=kubeflow_docs. Either the README or the live copy is out of date. I've left it for whoever owns that call.