feature!: verify and set default VamanaBuildParameters #96

yuejiaointel · 2025-03-18T01:05:23Z

BREAKING CHANGE: Removed the deprecated num_threads argument and added a use_full_search_history argument in VamanaBuildParameters in Python bindings. This change ensures consistency between the Python API and the C++ implementation in include/svs/index/vamana/build_params.h.

Added default value setting and checking based on doc requirement, and added additional tests.

Pin CMake < 4 as a workaround until our dependencies require CMake >= 3.5.

ibhati · 2025-03-21T19:44:20Z

include/svs/index/vamana/index.h

+
+    // Check User set values
+    // Validate number parameters are positive
+    if (parameters.alpha < 0.0f) {


Suggested change

if (parameters.alpha < 0.0f) {

if (parameters.alpha <= 0.0f) {

Good catch, fixed!

ibhati · 2025-03-21T19:54:19Z

tests/svs/orchestrators/vamana.cpp

+        CATCH_REQUIRE(index.get_max_candidates() == 64);
+        CATCH_REQUIRE(index.get_full_search_history() == true);
+    }
+}


I think these tests should go to svs/index/vamana/index.cpp where the logic is implemented. Also, can we add similar tests for dynamic_index?

Good idea, moved and added similar tests in dynamic_index_2.cpp

mihaic

Good work, @yuejiaointel! I look forward to not having to care about the default value of alpha. :) I think we can make a couple of improvements. Please see my suggestions.

bindings/python/tests/test_vamana.py

bindings/python/src/vamana.cpp

mihaic · 2025-03-21T20:44:03Z

bindings/python/src/vamana.cpp

-            py::arg("prune_to") = std::numeric_limits<size_t>::max(),
-            py::arg("num_threads") = std::numeric_limits<size_t>::max(),
+            py::arg("alpha") = svs::FLOAT_MAX,
+            py::arg("graph_max_degree") = svs::UNSIGNED_INTEGER_MAX,


We are not setting graph_max_degree based on other parameters. The default is always 32. Let's leave it as 32 so users immediately see what the default is.

Suggested change

py::arg("graph_max_degree") = svs::UNSIGNED_INTEGER_MAX,

py::arg("graph_max_degree") = 32,

Thx, renamed it to be GRAPH_MAX_DEGREE_DEFAULT with value 32

mihaic · 2025-03-21T20:47:39Z

bindings/python/src/vamana.cpp

-            py::arg("num_threads") = std::numeric_limits<size_t>::max(),
+            py::arg("alpha") = svs::FLOAT_MAX,
+            py::arg("graph_max_degree") = svs::UNSIGNED_INTEGER_MAX,
+            py::arg("window_size") = svs::UNSIGNED_INTEGER_MAX,


Same as graph_max_degree.

Suggested change

py::arg("window_size") = svs::UNSIGNED_INTEGER_MAX,

py::arg("window_size") = 64,

Thx, renamed it to be WINDOW_SIZE_DEFAULT with value 64

bindings/python/src/vamana.cpp

mihaic · 2025-03-21T20:49:01Z

bindings/python/src/vamana.cpp

+            py::arg("graph_max_degree") = svs::UNSIGNED_INTEGER_MAX,
+            py::arg("window_size") = svs::UNSIGNED_INTEGER_MAX,
+            py::arg("max_candidate_pool_size") = svs::UNSIGNED_INTEGER_MAX,
+            py::arg("prune_to") = svs::UNSIGNED_INTEGER_MAX,


Suggested change

py::arg("prune_to") = svs::UNSIGNED_INTEGER_MAX,

py::arg("prune_to") = svs::UNSIGNED_INTEGER_PLACEHOLDER,

Thx, renamed!

mihaic

Thank you for accommodating my suggestions. I think the code looks good now. We should also update the docs.

mihaic · 2025-03-24T17:03:10Z

include/svs/lib/preprocessor.h

+// Maximum values used as default initializers
+inline constexpr size_t UNSIGNED_INTEGER_PLACEHOLDER = std::numeric_limits<size_t>::max();
+inline constexpr float FLOAT_PLACEHOLDER = std::numeric_limits<float>::max();
+inline constexpr float GRAPH_MAX_DEGREE_DEFAULT = 32;


Can you please look at the docs corresponding to the PR and confirm these *_DEFAULT values are visible? Users should be able to see the default value without reading the code. Ideally, there should be numbers visible in the VamanaBuildParameters doc as currently; alternatively, there should be separate doc entries for the *_DEFAULT consts.

bindings/python/src/vamana.cpp

mihaic · 2025-03-24T17:16:37Z

include/svs/index/vamana/index.h

+    if (parameters.alpha == svs::FLOAT_PLACEHOLDER) {
+        // Check if it's a supported distance type
+        if (is_L2) {
+            parameters.alpha = 1.2f;


Good idea with the *_DEFAULT values! (I was only suggesting numerical values). What do you think about adding two more for the two alpha values for maintainability, e.g., ALPHA_MAXIMIZE_DEFAULT and ALPHA_MINIMIZE_DEFAULT? Feel free to say no.

Good idea, added!

bindings/python/src/vamana.cpp

mihaic · 2025-03-25T01:51:06Z

/intelci

yuejiaointel · 2025-03-25T05:36:14Z

/intelci
set ref=fix_set_default_build_params

yuejiaointel · 2025-03-25T05:37:04Z

/intelci

yuejiaointel · 2025-03-25T05:38:42Z

/intelci
set ref=fix_set_default_build_params

yuejiaointel · 2025-03-25T05:46:54Z

/intelci
set ref=fix_set_default_build_params

yuejiaointel · 2025-03-25T06:00:47Z

/intelci
set ref=fix_set_default_build_params

yuejiaointel · 2025-03-25T06:01:54Z

/intelci
set ref=fix_set_default_build_params

dian-lun-lin · 2025-03-27T21:10:34Z

include/svs/lib/preprocessor.h

@@ -159,3 +162,14 @@ inline constexpr bool have_avx512_avx2 = true;
 #endif

 } // namespace svs::arch
+
+namespace svs {


Thank you for the good work.

Since some of these parameters are vamana-specific, could we move these parameters to svs::index::vamana namespace? Or change the name to Vamana specific.

For example, GRAPH_MAX_DEGREE_DEFAULT -> VAMANA_GRAPH_MAX_DEGREE_DEFAULT.
ALPHA_*, USE_FULL_SEARCH_HISTORY_DEFAULT, WINDOW_SIZE_DEFAULT should also be changed to Vamana specific

Thx for suggestion, fixed!

yuejiaointel · 2025-03-27T21:38:37Z

/intelci
set ref=fix_set_default_build_params_test_back

yuejiaointel · 2025-03-27T22:01:43Z

/intelci
set ref=fix_set_default_build_params_test_back

yuejiaointel · 2025-03-27T22:30:28Z

/intelci
set ref=fix_set_default_build_params_test_back

mihaic

The signature for VamanaBuildParameters looks good in the docs! That was my main concern. I am leaving suggestions for removing duplication like the repetition of defaults for the defaults that are shown in the signature and for reStructuredText (vs Markdown) formatting.

bindings/python/src/vamana.cpp

include/svs/index/vamana/dynamic_index.h

bindings/python/src/vamana.cpp

Co-authored-by: Mihai Capotă <[email protected]>

mihaic

Thanks a lot for your contribution (and for your patience), @yuejiaointel! SVS users will benefit greatly from the new defaults.

mihaic · 2025-03-28T16:44:57Z

@yuejiaointel, please try changing the CIBuildWheel workflow to use Ubuntu 22.04:
https://github.com/intel/ScalableVectorSearch/actions/runs/14119826155/workflow?pr=96#L33

mihaic · 2025-03-28T17:20:04Z

Ubuntu 24.04 didn't help...

yuejiaointel · 2025-03-28T18:33:11Z

/intelci
set ref=fix_set_default_build_params_test_back

yuejiaointel added 11 commits March 17, 2025 18:03

feature: add alpha default value set and check in build

9702506

fix: format

97a0128

fix: set and check default params in index.h

e5f0cf4

fix: default test work

4080533

fix: fix all c++ test

5d8cf0f

fix: fix index serach test failing by removing reverify functin in apply

ffa6bd0

fix: revert vamana config.toml

eb7be5f

fix: all test working

c3ec8ba

fix: remove unnecessary changes

749bce2

fix: remove unnecessary changes

8bd4161

fix:format

4e774c4

yuejiaointel marked this pull request as ready for review March 21, 2025 05:33

fix: format

cc4da27

ibhati requested review from mihaic and dian-lun-lin March 21, 2025 19:32

ibhati reviewed Mar 21, 2025

View reviewed changes

mihaic requested changes Mar 21, 2025

View reviewed changes

yuejiaointel added 2 commits March 23, 2025 21:53

fix: fix comments

100cfef

fix: format

ca56187

mihaic reviewed Mar 24, 2025

View reviewed changes

yuejiaointel added 2 commits March 24, 2025 12:49

fix: comments and alpha default values const

1afaee3

fix: doc update and use constant in tests

f65ca7a

fix: format

ed83622

dian-lun-lin reviewed Mar 27, 2025

View reviewed changes

fix: rename parameters constant to vamana specific constant

d9abbf3

yuejiaointel added 2 commits March 27, 2025 14:51

fix: combine lgoic in veryfiy function

9b68be4

fix: combine lgoic in veryfiy function

ef1d863

fix: update logic again for verify

76f61d3

dian-lun-lin approved these changes Mar 27, 2025

View reviewed changes

mihaic requested changes Mar 28, 2025

View reviewed changes

mihaic changed the title ~~feature: verify and set default VamanaBuildParameters~~ feature!: verify and set default VamanaBuildParameters Mar 28, 2025

yuejiaointel and others added 2 commits March 27, 2025 17:46

Apply suggestions from code review

1d35ac4

Co-authored-by: Mihai Capotă <[email protected]>

fix: update comment

382d252

mihaic approved these changes Mar 28, 2025

View reviewed changes

yuejiaointel added 8 commits March 27, 2025 18:10

fix: format

3c8085c

test: test ci failture with extra para

0793c85

fix: revert

47c3297

test: ci fail test

63ebadf

fix: revert

becd2cd

test: ci fail test

839d3a3

test: ci fail test

e146904

fix: revert

a9b6471

Use Ubuntu 24.04 in cibuildwheel.yml

5d7d2e0

mihaic and others added 3 commits March 28, 2025 10:31

Update pybind11 to 2.11.2

47d967f

Require cmake<4

dde61e8

fix: revert changes in cibuildwheel and cmakelist

d88d970

mihaic merged commit f2380c1 into intel:main Mar 28, 2025
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature!: verify and set default VamanaBuildParameters #96

feature!: verify and set default VamanaBuildParameters #96

yuejiaointel commented Mar 18, 2025 •

edited by mihaic

Loading

ibhati Mar 21, 2025

yuejiaointel Mar 24, 2025

ibhati Mar 21, 2025

yuejiaointel Mar 24, 2025

mihaic left a comment

mihaic Mar 21, 2025

yuejiaointel Mar 24, 2025

mihaic Mar 21, 2025

yuejiaointel Mar 24, 2025

mihaic Mar 21, 2025

yuejiaointel Mar 24, 2025

mihaic left a comment

mihaic Mar 24, 2025

mihaic Mar 24, 2025

yuejiaointel Mar 24, 2025

mihaic commented Mar 25, 2025

yuejiaointel commented Mar 25, 2025

yuejiaointel commented Mar 25, 2025

yuejiaointel commented Mar 25, 2025

yuejiaointel commented Mar 25, 2025

yuejiaointel commented Mar 25, 2025

yuejiaointel commented Mar 25, 2025

dian-lun-lin Mar 27, 2025

yuejiaointel Mar 27, 2025

yuejiaointel commented Mar 27, 2025

yuejiaointel commented Mar 27, 2025

yuejiaointel commented Mar 27, 2025

mihaic left a comment

mihaic left a comment

mihaic commented Mar 28, 2025

mihaic commented Mar 28, 2025

yuejiaointel commented Mar 28, 2025

	if (parameters.alpha < 0.0f) {
	if (parameters.alpha <= 0.0f) {

	py::arg("graph_max_degree") = svs::UNSIGNED_INTEGER_MAX,
	py::arg("graph_max_degree") = 32,

	py::arg("window_size") = svs::UNSIGNED_INTEGER_MAX,
	py::arg("window_size") = 64,

	py::arg("prune_to") = svs::UNSIGNED_INTEGER_MAX,
	py::arg("prune_to") = svs::UNSIGNED_INTEGER_PLACEHOLDER,

feature!: verify and set default VamanaBuildParameters #96

feature!: verify and set default VamanaBuildParameters #96

Conversation

yuejiaointel commented Mar 18, 2025 • edited by mihaic Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaic commented Mar 25, 2025

yuejiaointel commented Mar 25, 2025

yuejiaointel commented Mar 25, 2025

yuejiaointel commented Mar 25, 2025

yuejiaointel commented Mar 25, 2025

yuejiaointel commented Mar 25, 2025

yuejiaointel commented Mar 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuejiaointel commented Mar 27, 2025

yuejiaointel commented Mar 27, 2025

yuejiaointel commented Mar 27, 2025

mihaic left a comment

Choose a reason for hiding this comment

mihaic left a comment

Choose a reason for hiding this comment

mihaic commented Mar 28, 2025

mihaic commented Mar 28, 2025

yuejiaointel commented Mar 28, 2025

yuejiaointel commented Mar 18, 2025 •

edited by mihaic

Loading