Lucene: define and validate CAGRA parameter boundaries - #2516
Lucene: define and validate CAGRA parameter boundaries#2516shaunakkapur wants to merge 10 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test efb837a |
cjnolet
added
improvement
shaunakkapur
changed the title
|
/ok to test a405e39 |
|
/ok to test 9bdd04b |
1 similar comment
|
/ok to test 9bdd04b |
|
/ok to test 558635f |
dantegd
left a comment
dantegd
left a comment
There was a problem hiding this comment.
Great to see this PR! Just had a few comments
| /** Smallest supported CAGRA intermediate-result count. */ | ||
| public static final int MIN_ITOPK = 1; | ||
|
|
||
| /** Largest intermediate-result count representable by the public Java API. */ | ||
| public static final int MAX_ITOPK = Integer.MAX_VALUE; | ||
|
|
||
| /** Largest intermediate-result count supported by CAGRA's SINGLE_CTA search algorithm. */ | ||
| public static final int MAX_SINGLE_CTA_ITOPK = 512; | ||
|
|
||
| /** Smallest supported number of CAGRA search entry points. */ | ||
| public static final int MIN_SEARCH_WIDTH = 1; | ||
|
|
||
| /** | ||
| * Largest search width that keeps CAGRA's result buffer within its unsigned 32-bit indexing | ||
| * limit at the maximum graph degree and aligned {@link #MAX_ITOPK}. | ||
| */ | ||
| public static final int MAX_SEARCH_WIDTH = 4_194_303; |
There was a problem hiding this comment.
I wonder if we should reconsider how these upper bounds are derived? MAX_SEARCH_WIDTH appears to protect the unsigned result-buffer calculation, but that isn’t the limiting calculation for the MULTI_CTA path that a normal one-query AUTO search uses.
MULTI_CTA sizes its hash from max(searchWidth, ceil(iTopK / 32)) * max(32, maxIterations). With MAX_SEARCH_WIDTH, that requires at least a 28-bit hash table, while native CAGRA hard-limits it to 25. MAX_ITOPK is even larger and would require a 32-bit table, beyond the range of the native 1U << bitlen sizing.
Since the current test only constructs the Java query, it doesn’t exercise these native constraints. What do you think about validating after the algorithm is resolved and adding a test that actually creates the native search plan?
There was a problem hiding this comment.
Agreed, these were presented as more universal than they are. The oversized searchWidth test now goes through a normal one-query AUTO search and asserts the native failure names the hash_bitlen 25-bit limit, and the README/javadoc now say only the minimum of 1 and the SINGLE_CTA max of 512 are real native limits.
One open item: I couldn't add equivalent coverage for MAX_ITOPK. With iTopK = Integer.MAX_VALUE the native call doesn't return an error, it leaks and stays stuck past the test's own timeout, so asserting on it isn't CI-safe. I documented that in the test. Since the real limit depends on the resolved algorithm and other settings, would you rather we enforce an algorithm-specific limit in the Lucene API, or fix the native calculation to reject it safely first?
- Scope graphDegree <= intermediateGraphDegree validation to CUSTOM strategy in AcceleratedHNSWParams/GPUSearchParams; HEURISTIC derives both degrees and ignores the configured pair, so previously a valid HEURISTIC config could fail to build over values that were never used. Remove the resulting workaround setter from TestSegmentMaxConnConsistency and add regression tests for both the CUSTOM enforcement and the HEURISTIC no-op case. - Re-validate the SINGLE_CTA effective iTopK limit against the value actually sent to native CAGRA in CuVS2510GPUVectorsReader, not just the value checked at GPUKnnFloatVectorQuery construction time: the filtered per-segment fallback path can raise topK further based on filter cardinality after construction, so a config valid at construction could still send an out-of-range value to native CAGRA. Add a regression test reproducing this exact scenario. - Stop presenting GPUKnnFloatVectorQuery's MAX_ITOPK/MAX_SEARCH_WIDTH as universally valid native ranges in the README and tests. Only the lower bound of 1 and the SINGLE_CTA iTopK maximum of 512 are genuine native limits; other upper bounds depend on the resolved algorithm, max_iterations, graph degree, and dataset size, none of which are known at query-construction time. Document that native CAGRA rejects unsupported combinations itself (e.g. MULTI_CTA's 25-bit traversal hash-table limit), and add a test that builds a real native search plan to confirm this for an oversized MULTI_CTA searchWidth. Also correct the README's HEURISTIC description: AcceleratedHNSWParams derives both degrees from maxConn/beamWidth, while GPUSearchParams passes the configured graphDegree into the dataset heuristic and derives the rest from it. Signed-off-by: Shaunak Kapur <shaunakk@nvidia.com>
- TestNativeSearchPlanBoundaryRejection: exercise AUTO (not an explicit MULTI_CTA) for the oversized searchWidth case, matching the reviewer's exact "normal one-query AUTO search resolves to MULTI_CTA" scenario, and assert the native exception message names the hash_bitlen/25 limit so an unrelated native/CUDA failure can't make the test pass. Also document why this class deliberately does not add an equivalent MAX_ITOPK case: empirically, iTopK = Integer.MAX_VALUE does not fail fast like an oversized searchWidth -- it hangs indefinitely inside the native call instead, which would make a test asserting on it unsafe to run in CI. - TestFilteredSingleCtaITopKValidation: assert the rejection message names SINGLE_CTA and the specific 512/513 values, confirming the post-filter re-validation fired rather than some unrelated argument check. - README: clarify that most parameter checks run at construction time but the SINGLE_CTA iTopK limit is additionally re-checked at search time, and that only intermediateGraphDegree is ignored under HEURISTIC for GPUSearchParams (graphDegree is passed into the dataset-size heuristic, not ignored). Verified locally: full compile, 34/34 relevant tests pass (including both native GPU-backed integration tests), spotless clean. Signed-off-by: Shaunak Kapur <shaunakk@nvidia.com>
The javadoc and README claimed native CAGRA rejects any oversized itopk_size/searchWidth combination with a clear exception. Measurement shows that only holds below a threshold: above roughly iTopK 1e9 the native hash-table sizing loop fails to terminate and the search hangs instead of erroring. Correct that claim, stop implying MAX_ITOPK is a usable maximum, and point at NVIDIA#2523 which tracks the native defect. Documentation only; no behaviour change. Signed-off-by: Shaunak Kapur <shaunakk@nvidia.com>
3 participants
Summary
Define and document boundaries for all five Lucene CAGRA parameter families: writer threads, intermediate graph degree, graph degree, iTopK, and search width.
Validate individual build parameter ranges when public builder setters are called. Validate at build time that graphDegree does not exceed intermediateGraphDegree, independent of setter order.
Validate search parameters when GPUKnnFloatVectorQuery is constructed, including the native SINGLE_CTA iTopK limit and the numeric safety limit for search width.
Add focused minimum, maximum, invalid range, cross field relationship, and search algorithm coverage.
Testing
Closes #2450