feature: Per index logger and new logger unit test for each index #87

yuejiaointel · 2025-02-21T23:04:40Z

This PR intends to add per index logging feature to help the redis integration progress

ibhati · 2025-03-04T18:10:09Z

include/svs/core/logging.h

    Level::Critical,
-    Level::Off};
+    Level::Off
+};


Can you please run these commands from the repository's root to ensure all the formatting is consistent?
sudo apt install clang-format-15
bash tools/clang-format.sh clang-format-15

Fixed, thx!

ibhati · 2025-03-04T18:40:28Z

include/svs/core/logging.h

+inline void set_global_log_callback(log_callback_function callback) {
+    global_log_callback = callback;
+    std::cout << "[DEBUG] set_global_log_callback(" << reinterpret_cast<void*>(callback)
+              << ")\n";


I think we should remove the std::cout print statements.

Removed, thx!

include/svs/core/logging.h

ibhati · 2025-03-04T23:55:46Z

include/svs/index/vamana/index.h

 ///     search. This may be included to perform operations like reranking for quantized
 ///     datasets.
+/// @tparam log_callback_ctx A pointer to a user-defined context for per-index logging
+/// customization.


We can remove the logger from here as it is not the template parameter

Removed, thx!

rfsaliev

Seems like this change breaks existing SVS logging design based on spdlog.
IMHO, instead of log_callback_ctx we should use logger_ptr.
In such case user will follow known spdlog approach for custom loggers (including contexts) rather than than learn one more SVS-specific approach (with the risk of typos like "DЕBYG" instead of "DEBUG").
And there is still global logger used, e.g. in MutableVamanaIndex::consolidate()

rfsaliev · 2025-03-06T14:22:38Z

include/svs/index/flat/flat.h

+        Data data,
+        Dist distance,
+        ThreadPoolProto threadpool_proto,
+        void* log_callback_ctx = nullptr


IMHO, svs::logger_ptr would be better.

Thx, that make sense, changed!

yuejiaointel · 2025-03-07T01:18:32Z

consolidate

That's a good idea! I changed the code to use sink component and make shared function in spdlog. But not too sure about functions like MutableVamanaIndex::consolidate(). It looks like these loggers are just used for debugging, do we want them to have per index logging as well?
Best,
Yue

dian-lun-lin · 2025-03-07T06:19:30Z

Now that we have per-index-instance logger, each member function in the index should use that logger. For example,
svs::logging::debug(logger, "Replacing entry point."); in consolidate should use per-index-instance logger. The unittest should check if this logging information is captured by per-index-instance logger.

Note that we have logging information in construct as well. We should also delegate the per-index-instance logger to that construct for logging rather than the global logger.

yuejiaointel · 2025-03-12T05:10:36Z

Now that we have per-index-instance logger, each member function in the index should use that logger. For example, svs::logging::debug(logger, "Replacing entry point."); in consolidate should use per-index-instance logger. The unittest should check if this logging information is captured by per-index-instance logger.

Note that we have logging information in construct as well. We should also delegate the per-index-instance logger to that construct for logging rather than the global logger.

Thx for the help! Now these files are using per index logging :D

dian-lun-lin · 2025-03-04T00:08:35Z

tests/svs/index/vamana/dynamic_index.cpp

+// const double DELETE_PERCENT = 0.05;
+// #endif
+
+// CATCH_TEST_CASE("MutableVamanaIndex", "[graph_index]") {


Why do we comment out this test?

these are removed already, dynamic_index.cpp is not changed, only dynamic_index_2.cpp is added a new test

dian-lun-lin · 2025-03-04T00:28:04Z

tests/svs/index/vamana/index.cpp

+    std::string prefix;
+};
+
+static void test_log_impl(void* ctx, const char* level, const char* message) {


I'm not really sure the purpose of static in every test_log_impl. I think putting this function and the struct into anonymous namespace is enough?

I don't see it anymore, seems to be older old?

dian-lun-lin · 2025-03-04T00:29:58Z

tests/svs/index/vamana/dynamic_index.cpp

-                  << post_add_time << " seconds." << std::endl;
-    }
-}
+CATCH_TEST_CASE("Dynamic MutableVamanaIndex Per-Index Logging Test", "[logging]") {


Could you also add a test for not setting the callback (i.e., not calling set_global_log_callback)? Adding this test in dynamic index should be enough.

Thx! Added in dynamic_index_2.cpp

dian-lun-lin · 2025-03-04T00:39:57Z

include/svs/index/vamana/dynamic_index.h

    /// @brief Add the points with the given external IDs to the dataset.
    //
    /// When `delete_entries` is called, a soft deletion is performed, marking the entries
-    /// as `deleted`. When `consolidate` is called, the state of these deleted entries becomes `empty`.


Just a side note that I notice clang-format keeps changing the format of this paragraph. We might want to consider disabling clang-format for this specific section.

Fixed, this part will not change

dian-lun-lin · 2025-03-13T15:34:48Z

include/svs/index/vamana/index.h

    }

+    /// @brief Getter method for logger
+    svs::logging::logger_ptr get_logger() const { return logger_; }


Should we have this get_logger for each index?

thx, added logger getter for each index

dian-lun-lin · 2025-03-13T15:37:12Z

tests/integration/vamana/index_build.cpp

+
+        // Verify the custom logger captured the log messages
+        CATCH_REQUIRE(captured_logs[0].find("Number of syncs:") != std::string::npos);
+        CATCH_REQUIRE(captured_logs[1].find("Batch Size:") != std::string::npos);


Please also test here to check if the global logger is not used.

thx, added!

ibhati · 2025-03-13T19:03:31Z

include/svs/index/vamana/index.h

+        I{},
+        std::move(distance),
+        std::move(threadpool),
+        logger};


Suggested change

logger};

std::move(logger)};

should we move instead of copy?

Good catch, changed!

yuejiaointel added 8 commits February 21, 2025 14:57

feature: logger in vamana index and unit test

b72fc00

feature: customized global logger and per index logger supoort

ff105b5

fix: test work for static index

4dae087

fix: comment out inverted for now

99be5df

feature: add logger to auto build and auto assemble functions

6882d87

fix: inverted index work

3b3136c

fix: dynamic index test working

9917d9c

fix: add created test files

3810870

yuejiaointel changed the title ~~feature: logger in vamana index and unit test~~ feature: Per index logger and new logger unit test for each index Feb 28, 2025

yuejiaointel marked this pull request as ready for review February 28, 2025 06:33

dian-lun-lin requested review from dian-lun-lin and ibhati March 3, 2025 23:57

ibhati reviewed Mar 4, 2025

View reviewed changes

yuejiaointel added 4 commits March 4, 2025 15:36

fix: format and comments

766004b

fix: everthing works locally

c5402ad

fix: format

32e05e4

fix: format

ce8d27d

ibhati reviewed Mar 4, 2025

View reviewed changes

yuejiaointel added 9 commits March 4, 2025 16:23

fix: dybamic index 2 working

e616eae

fix: format

6b05251

fix: fix flat test

ac988db

fix: remove empty space and make idnex test same as other tests

ee22f45

fix: remove changes on dynamic_index.cpp

88d866a

fix: remove changes on dynamic_index.cpp

2a092ae

fix: format

8321bc4

fix: don't de;ete ref from dynamic index 2

d40db30

fix: don't de;ete ref from dynamic index 2

64209f5

ibhati approved these changes Mar 5, 2025

View reviewed changes

rfsaliev requested changes Mar 6, 2025

View reviewed changes

rfsaliev reviewed Mar 6, 2025

View reviewed changes

yuejiaointel added 2 commits March 6, 2025 17:05

fix: use sdp log callback instead creating new wheels

3e62f8d

fix: format

5ee991d

yuejiaointel added 4 commits March 11, 2025 16:13

fix: vamana index internal logging change to per index logging works

3e9fcb8

fix: dynamic index works

e5b02c0

fix: clustering logging changed to per idnex logging

c3c3142

fix: format

6e379fe

yuejiaointel added 2 commits March 13, 2025 11:11

fix: add logger and extra check in integraiton logging test

ad80dbf

fix: format

e096841

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

View reviewed changes

ibhati reviewed Mar 13, 2025

View reviewed changes

yuejiaointel added 4 commits March 13, 2025 12:16

fix: resolveing comemnts

3429e9a

fix: use move instead of cp for logger in some places

270dd11

fix: don't remove exra lines

cb3f58e

fix: no extra spece added

f440064

yuejiaointel closed this Mar 13, 2025

yuejiaointel reopened this Mar 13, 2025

Merge branch 'main' into dev/yue/fix_per_index_logging

69b9da9

ibhati merged commit 1878951 into intel:main Mar 14, 2025
7 checks passed

feature: Per index logger and new logger unit test for each index #87

feature: Per index logger and new logger unit test for each index #87

Uh oh!

Conversation

yuejiaointel commented Feb 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rfsaliev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuejiaointel Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuejiaointel commented Mar 7, 2025

Uh oh!

dian-lun-lin commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuejiaointel commented Mar 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuejiaointel Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuejiaointel Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibhati Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yuejiaointel Mar 7, 2025 •

edited

Loading

dian-lun-lin commented Mar 7, 2025 •

edited

Loading

yuejiaointel Mar 13, 2025 •

edited

Loading

yuejiaointel Mar 13, 2025 •

edited

Loading

ibhati Mar 13, 2025 •

edited

Loading