Skip to content

fix(search): apply cosine radius across iterative recall paths - #451

Open
knqiufan wants to merge 2 commits into
EverMind-AI:mainfrom
knqiufan:codex/fix-search-radius
Open

knqiufan wants to merge 2 commits into
EverMind-AI:mainfrom
knqiufan:codex/fix-search-radius

Conversation

@knqiufan

@knqiufan knqiufan commented Sep 16, 2026

Copy link
Copy Markdown

Summary

Fixes #394.

agentic and llm_multiround accepted radius but bypassed the manager's dense-recall filter. A request with dense hits at cosine 0.9 and 0.2 and radius=0.5 now removes the 0.2 hit before fusion on every iterative query. Existing vector and hybrid filtering remains in place.

  • Share request-default resolution and dense filtering across search orchestration. Resolve the default before capping top_k: explicit values win (including zero), unlimited searches default to 0.5, and positive top_k has no implicit floor.
  • Pass the resolved floor through user/agent AGENTIC and LLM_MULTIROUND. Filter both atomic-fact and episode-subject dense hits before user AGENTIC MaxSim pooling, and filter each multi-round dense pool before RRF.
  • Remove the three inert RankInput.radius arguments. Never map cosine thresholds to ahybrid_retrieve(min_score=...), which would compare them with a different score scale.
  • Document the dense-only contract and the episode HYBRID-only scope of min_score. Cover dispatch/defaults, zero and boundary scores, sparse preservation, empty dense pools, both agent kinds, and repeated queries with real RRF/MaxSim operators.

Area

  • Architecture method
  • Benchmark
  • Use case
  • Documentation
  • Developer experience
  • CI, build, or release

Acceptance coverage

  • Query-specific embeddings and dense pools cover user AGENTIC (fact and subject children), agent case/skill AGENTIC, and LLM_MULTIROUND. A shared candidate crosses the cosine floor across queries (0.9, 0.2, 0.8).
  • Inspect the child pools entering real MaxSim and the inputs to real RRF on every query. Include a low subject child sharing a parent with a stronger fact, and a low dense hit sharing an ID with a valid sparse hit; the latter must retain only its sparse RRF contribution for that query.
  • Preserve explicit zero through JSON serialization and the HTTP/service boundary. Check unlimited/positive top_k dispatch separately. API docs and the generated OpenAPI description explain omitted/null versus explicit zero, inclusive boundaries, and recall-pool limits.
  • In-memory fault injection confirms the new checks fail for all four tested regressions: cached first-query pools, first-query-only filtering, skipped subject filtering, and post-fusion filtering. These mutations were confined to verification processes and are not part of the source tree.

Verification

Validation for PR head 33d7e23:

  • Linux CI: make lint and make package passed; Python 3.12 and 3.13 unit suites each passed (2623 passed, 4 skipped), and both integration suites passed (183 passed, 5 skipped, 7 deselected each). The Milvus 2.6.22 repository and /get checks passed; its tiered API suite and final index-contract check are still running/pending as of this update. The linked run is the live status source.
  • Local search, request-validation, and runtime OpenAPI suites: 344 passed, including 79 radius-related cases. Windows used an import-only fcntl stand-in that raises on POSIX operations; Linux CI uses the real platform.
  • Local Ruff, import contracts, repository gates, documentation links, Conventional Commit checks, and OpenAPI drift checks passed.

The Linux validation branch has identical source, tests, and documentation to this PR. Its only differences are CI configuration: enabling the validation-branch push trigger and using the same MinIO version tag from quay.io/minio/minio for Milvus setup. The image-source workaround is not part of this PR; the unchanged upstream workflow may still encounter the previously observed Docker Hub pull failure.

Checklist

  • I kept the change scoped to the relevant area.
  • I am opening this from a separate branch, not pushing directly to main.
  • I updated docs, examples, or setup notes when behavior changed.
  • I added or updated tests when the change affects behavior.
  • I did not commit secrets, .env files, dependency folders, or generated output.
  • Active relative links in Markdown files resolve.

Notes for Reviewers

This implements option (a) from the issue: a pre-fusion dense cosine floor. Keyword matches, cluster expansions, and case-to-skill linkage remain independent recall routes; radius is not a final-score floor or a promise of monotonically decreasing result counts. The existing min_score behavior is unchanged.

The intended behavior change includes default unlimited iterative searches: they now honor the documented 0.5 floor. Internal helper callers that omit the new optional argument retain unfiltered behavior. VECTOR and HYBRID already filtered dense hits before this change; the unused ranker argument was not evidence that those paths ignored radius.

By submitting this pull request, I agree that my contribution is licensed under the Apache License 2.0.

Share radius resolution and pre-fusion dense filtering across search lanes. Preserve sparse and linkage routes, remove inert RankInput arguments, and cover defaults, boundaries, iterative queries, and RRF score separation.

Fixes EverMind-AI#394
@BrierAinz

Copy link
Copy Markdown

This looks like the right shape for radius: resolve once as a dense cosine floor, apply it before fusion, and keep sparse / linkage routes out of that promise.

One acceptance check I would keep explicit before merge: a repeated-query iterative request should prove that each dense pool is filtered independently before RRF/MaxSim, not only that the final result set happens to exclude a low-score item. The easy regression trap is a helper path that filters the first query's pool but lets a later round contribute sub-radius dense hits through fusion.

It may also be worth spelling out in docs that radius=0 means "accept every dense hit" while omitted unlimited iterative searches default to 0.5. That distinction is subtle, but it is exactly the kind of subtlety client SDKs tend to flatten accidentally.

This branch has not been deployed

No deployments
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.

radius is documented and validated but has no effect on any search lane

2 participants