Promote filtering_rate to base search_params; honor it in brute_force#2120
Promote filtering_rate to base search_params; honor it in brute_force#2120maxwbuckley wants to merge 2 commits into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces an optional ChangesFiltering Rate Hint Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/tests/neighbors/brute_force_prefiltered.cu (1)
1084-1130: ⚡ Quick winAdd one high-sparsity hint case to cover the lazy-
nnzCSR branch.Current inputs top out at
sparsity = 0.5, soRunWithFilteringRateHint()never exercises the newsparsity >= 0.9path where exactnnzis computed lazily. Adding at least one> 0.9case for bitmap and bitset would close this gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/neighbors/brute_force_prefiltered.cu` around lines 1084 - 1130, Add at least one test entry to the selectk_inputs vector that triggers the lazy-nnz CSR branch by using a sparsity >= 0.9; update the PrefilteredBruteForceInputs initializer list (selectk_inputs) to include a case with high sparsity (e.g. 0.95) so RunWithFilteringRateHint() exercises the sparsity >= 0.9 path (add one for bitmap/bitset coverage if your test matrix type selection is driven by the distance/type field such as cuvs::distance::DistanceType::L2Expanded or InnerProduct).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/neighbors/detail/knn_brute_force.cuh`:
- Around line 623-627: The code currently enables hint mode for any non-negative
params.filtering_rate; change this to only enable hint mode when filtering_rate
is in [0.0, 1.0) by replacing the use_hint initialization with an explicit range
check (e.g., const bool use_hint = (params.filtering_rate >= 0.0f &&
params.filtering_rate < 1.0f)); additionally add an explicit validation branch
near that declaration (in the knn_brute_force.cuh scope where use_hint and
params.filtering_rate are used) that treats values >= 1.0f as invalid—either
disable the hint and emit a clear error/exception (e.g., throw
std::invalid_argument or return an error) or log a warning before proceeding—so
out-of-contract inputs do not silently alter path selection.
---
Nitpick comments:
In `@cpp/tests/neighbors/brute_force_prefiltered.cu`:
- Around line 1084-1130: Add at least one test entry to the selectk_inputs
vector that triggers the lazy-nnz CSR branch by using a sparsity >= 0.9; update
the PrefilteredBruteForceInputs initializer list (selectk_inputs) to include a
case with high sparsity (e.g. 0.95) so RunWithFilteringRateHint() exercises the
sparsity >= 0.9 path (add one for bitmap/bitset coverage if your test matrix
type selection is driven by the distance/type field such as
cuvs::distance::DistanceType::L2Expanded or InnerProduct).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 73fd1e69-bb28-41e6-8152-51e18206fd9f
📒 Files selected for processing (5)
cpp/include/cuvs/neighbors/cagra.hppcpp/include/cuvs/neighbors/common.hppcpp/src/neighbors/brute_force.cucpp/src/neighbors/detail/knn_brute_force.cuhcpp/tests/neighbors/brute_force_prefiltered.cu
| IdxT nnz_h = 0; | ||
| float sparsity = 0.0f; | ||
| bool nnz_h_is_set = false; | ||
| const bool use_hint = params.filtering_rate >= 0.0f; | ||
|
|
There was a problem hiding this comment.
Validate filtering_rate before treating it as a hint.
Line 626 currently accepts any non-negative value as valid. That allows out-of-contract inputs (e.g., >= 1.0) to silently affect path selection. Add an explicit range check and only enable hint mode for [0.0, 1.0).
🔧 Proposed fix
- IdxT nnz_h = 0;
- float sparsity = 0.0f;
- bool nnz_h_is_set = false;
- const bool use_hint = params.filtering_rate >= 0.0f;
+ const bool is_auto = params.filtering_rate < 0.0f;
+ const bool is_hint = params.filtering_rate >= 0.0f && params.filtering_rate < 1.0f;
+ RAFT_EXPECTS(is_auto || is_hint,
+ "brute_force::search_params::filtering_rate must be negative (auto) or in [0, 1).");
+
+ IdxT nnz_h = 0;
+ float sparsity = 0.0f;
+ bool nnz_h_is_set = false;
+ const bool use_hint = is_hint;As per coding guidelines “Input validation must check for negative or invalid dimensions, null pointers, and invalid parameter combinations before GPU operations”.
Also applies to: 634-650
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/neighbors/detail/knn_brute_force.cuh` around lines 623 - 627, The
code currently enables hint mode for any non-negative params.filtering_rate;
change this to only enable hint mode when filtering_rate is in [0.0, 1.0) by
replacing the use_hint initialization with an explicit range check (e.g., const
bool use_hint = (params.filtering_rate >= 0.0f && params.filtering_rate <
1.0f)); additionally add an explicit validation branch near that declaration (in
the knn_brute_force.cuh scope where use_hint and params.filtering_rate are used)
that treats values >= 1.0f as invalid—either disable the hint and emit a clear
error/exception (e.g., throw std::invalid_argument or return an error) or log a
warning before proceeding—so out-of-contract inputs do not silently alter path
selection.
There was a problem hiding this comment.
Good catch — agreed, fixed in 693f216.
The contract was already documented on the base search_params ("Negative (default): auto-detect... In [0.0, 1.0): the algorithm trusts the supplied value"), but the code only enforced the >= 0.0f half of it. The concrete failure mode is worse than "silently affects path selection": a caller who passes a percentage (50.0) instead of a fraction, or otherwise overshoots, gets sparsity >= 0.9 → the sparse CSR path → plus the lazy popcount. That's strictly slower than not setting the hint at all, and it happens with no diagnostic. Exactly the overhead this PR exists to remove, so failing loudly is right.
I took your fix with three small deviations:
- Placement. Moved the check up next to the other
RAFT_EXPECTSinput validation at the top ofbrute_force_search_filtered, rather than leaving it inline at theuse_hintdeclaration. That matches the "validate before GPU operations" guideline you cited and keeps the declaration block below purely about state. - NaN. Kept your
>= 0.0f && < 1.0fformulation deliberately, and called it out in a comment — NaN compares false against both bounds, so it falls through to the error rather than being silently treated as auto-detect. Worth pinning down since it's easy to "simplify" back into a bug. - Message. Dropped the
brute_force::qualifier, since the field now lives on the basecuvs::neighbors::search_params, and included the offending value.
// Negative means "auto-detect from the filter"; [0.0, 1.0) is trusted as-is. Anything else
// (including NaN, which compares false against both bounds) is out of contract.
const bool auto_filtering_rate = params.filtering_rate < 0.0f;
const bool use_hint = params.filtering_rate >= 0.0f && params.filtering_rate < 1.0f;
RAFT_EXPECTS(auto_filtering_rate || use_hint,
"search_params::filtering_rate must be negative (auto-detect) or in [0.0, 1.0), "
"got %f",
params.filtering_rate);Added an InvalidFilteringRateThrows test case on the bitmap fixture covering 1.0, 1.5, and 50.0.
Verified, since NVIDIA CI hasn't been able to run on this PR yet: both brute_force.cu and brute_force_prefiltered.cu compile clean with the project's -Wall -Werror -Werror=all-warnings flags, and I drove the real detail::brute_force_search_filtered directly to observe the guard at runtime — -1.0, -0.5, 0.0, 0.5, 0.999 all search normally; 1.0, 1.5, 50.0, and NaN all throw raft::logic_error with the message above.
One thing I deliberately did not do: extend this validation to CAGRA, which reads the same inherited field. CAGRA currently tolerates out-of-range values by guarding at its use site (search_plan.cuh checks 0.0 < filtering_rate && filtering_rate < 1.0) instead of throwing. Making that path throw is a behavior change for existing CAGRA callers and belongs in its own PR, not this one.
aamijar
added
non-breaking
`filtering_rate` (a hint of the fraction of items filtered out by the sample filter) previously lived on `cagra::search_params` only. CAGRA used it to size `itopk_size` and, when the user left it at its default of `-1.0`, called `bitset_view::count(res)` on every search — a GPU popcount reduction + host sync that adds measurable latency. `brute_force` filtered search also called `bitset_view::count(res)` on every search to compute `sparsity` (used to choose between the dense tiled GEMM path and the sparse CSR path), but had no user-facing knob to skip the auto-detection. This change: - Moves `float filtering_rate = -1.0` from `cagra::search_params` to the base `cuvs::neighbors::search_params`. `cagra::search_params` inherits it; existing code that accesses `params.filtering_rate` is unaffected. - Plumbs `brute_force::search_params` through `detail::search` and `brute_force_search_filtered`. When `params.filtering_rate >= 0`, the hint is used directly as `sparsity` and the per-search popcount is skipped on the dense path. The CSR path still needs an exact non-zero count to size the matrix, so popcount runs lazily there. - Algorithms that don't use the hint (`ivf_flat`, `ivf_pq`, `hnsw`) ignore the new base field; their `search_params` inherit it but nothing reads it. Tests: extends `brute_force_prefiltered.cu` with `ResultWithFilteringRateHint` cases for both bitmap and bitset fixtures (float and half), reusing the existing parameter matrix and asserting that results match the auto-detect path. Closes NVIDIA#1960. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
71decfc to
d99e6a0
Compare
The base search_params docstring already defines the contract: negative means "auto-detect from the filter", and [0.0, 1.0) is trusted as a hint. The code only checked `>= 0.0f`, so an out-of-contract value (e.g. a caller passing a percentage like 50.0, or 1.5) was silently accepted as `sparsity`, forcing the sparse CSR path plus its popcount -- slower than not setting the hint at all, with no diagnostic. Enforce the documented range with RAFT_EXPECTS. NaN is rejected too, since it compares false against both bounds. Behavior for in-contract values, including the negative default, is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@lowener this is number 2 in my series after 1) the dot product normalization constant optimization and before 3) the scatter gather and threshold fixing |
3 participants
Summary
filtering_ratepreviously lived oncagra::search_paramsonly. Moving it to the basecuvs::neighbors::search_paramsmakes it inheritable by every algorithm'ssearch_paramsand removes the duplicate-per-algorithm pattern that would follow if other prefiltered paths wanted the same knob.brute_force::search_paramsthroughdetail::searchandbrute_force_search_filtered. Whenparams.filtering_rate >= 0, the hint is used directly assparsityand the per-searchbitset_view::count(res)popcount + host sync is skipped on the dense path. The sparse CSR path still needs an exact non-zero count to size the matrix, so popcount runs lazily there.ivf_flat,ivf_pq,hnsw) ignore it; theirsearch_paramsinherit the field but nothing reads it.Closes #1960.
cc @cjnolet — this is the cleanup for the popcount-per-search overhead from #1960.
Why
Both
cagra::search(with defaultfiltering_rate=-1.0) andbrute_force::search(always) callbitset_view::count(res)on every search to derive selectivity. That's a GPU reduction kernel + host stream sync per call. Documented measured impact on RTX 5090 at 1M/d=128/50% pass rate is ~18% latency overhead for CAGRA; brute_force had no escape hatch at all because itssearch_paramswas an empty derivation of the base.brute_force::search_paramswas already empty (struct search_params : cuvs::neighbors::search_params {};), so the field arrives via inheritance — no algorithm-specific addition needed.What changed
cpp/include/cuvs/neighbors/common.hppfloat filtering_rate = -1.0to basesearch_paramswith docstringcpp/include/cuvs/neighbors/cagra.hppcpp/src/neighbors/brute_force.cuparamsthrough the 4 macrosearch()entry pointscpp/src/neighbors/detail/knn_brute_force.cuhparamsarg todetail::searchandbrute_force_search_filtered; honor hint when>= 0, lazy popcount on CSR pathcpp/tests/neighbors/brute_force_prefiltered.cuResultWithFilteringRateHinttest cases for bitmap and bitset fixtures (float and half) reusing the existing parameter matrixTest plan
cpp/src/neighbors/brute_force.cucompiles cleanlycpp/tests/neighbors/brute_force_prefiltered.cucompiles cleanlyNEIGHBORS_TESTto confirmResultWithFilteringRateHintmatchesResultacross all parameter casesNotes
search_paramswassizeof == 1(empty); nowsizeof == 4(onefloat). All algorithm-specificsearch_paramsinherit, so their layout grows by 4 bytes. Appropriate for a release-cycle change.filtering_rateis currently only referenced from C++.filtering_rate < 0(default): same popcount, same downstream logic.🤖 Generated with Claude Code