[C API] Refactor C API to be ready for the main branch - #363
Conversation
- Updated ThreadPoolBuilder to ensure custom threadpool pointers are validated and initialized correctly. - Enhanced error handling in parallel_for method to catch exceptions and rethrow them appropriately. - Modified IDFilterAdapter to check for null operations and validate filter rates during initialization. - Adjusted test cases to reflect changes in function signatures and ensure proper error handling. - Introduced new utility functions for initializing search results and memory breakdown structures. - Updated sequential threadpool implementation to return a boolean indicating success.
|
Tick the box to add this pull request to the merge queue (same as
|
|
@copilot, write a comment with key changes in details and instructions how to convert client code for new API |
Key Changes & Migration Guide for the New C APIThis PR introduces breaking changes to the C API. Below is a summary of every significant change and how to update your client code. 1. Header Path:
|
…ce structure to let user release it.
ethanglaser
left a comment
There was a problem hiding this comment.
Overall a lot of good changes. Not CMake and versioning are looking good.
| // SVS C API requires a C99 or later compiler, or a C++20 or later compiler. If the | ||
| // compiler does not meet these requirements, a compilation error will be generated with a | ||
| // clear message indicating the required standard version. | ||
| #if defined(__cplusplus) | ||
| #if __cplusplus < 202002L | ||
| #error \ | ||
| "svs_c.h requires C++20 or later (designated initializers in SVS_INIT_* / SVS_MAKE_INTERFACE)." | ||
| #endif | ||
| #elif defined(__STDC_VERSION__) | ||
| #if __STDC_VERSION__ < 199901L | ||
| #error \ | ||
| "svs_c.h requires C99 or later (designated initializers in SVS_INIT_* / SVS_MAKE_INTERFACE)." | ||
| #endif | ||
| #else | ||
| #error "svs_c.h requires C99 or later, or C++20 or later." |
There was a problem hiding this comment.
I get that this helps ensure compiler version use aligns with requirements, but we don't have these types of checks elsewhere afaik and are we really expecting people to be trying to build this with compilers outside of these scopes? Maybe worth a short note in docs instead
Edit: looks like we also have this enforce in CMakeLists.txt as well, so maybe we stick with that
| ) | ||
|
|
||
| # Include directories | ||
| target_include_directories(${TARGET_NAME} PRIVATE |
There was a problem hiding this comment.
Should this same block be removed from samples CMakeLists.txt?
| selectivity; if the observed hit rate ends up lower than the reported estimate the | ||
| function returns an empty result set for that query. | ||
|
|
||
| ## API Reference |
There was a problem hiding this comment.
Is it worth having 400+ lines of API reference here along with the verbosity of comments in svs_c.h? Maybe better to have one reference the other to avoid having to update both moving forward
There was a problem hiding this comment.
This is a different setup than the runtime bindings, though seems like the right approach. Follow-up work can include a similar update to the runtime bindings.
|
|
||
| /// @brief Structure to hold memory breakdown for an index. | ||
| /// | ||
| /// Forward-compatibility contract: On any write to this OUT struct, the library |
There was a problem hiding this comment.
Same comment block as line 262
| # C++20 is required to build this library, but not to consume it: the public | ||
| # surface is a C ABI. Keep the requirement PRIVATE so that pure-C consumers are | ||
| # not forced to compile as C++20. | ||
| target_compile_features(${TARGET_NAME} PRIVATE cxx_std_20) |
There was a problem hiding this comment.
Worth adding target_compile_features(${TARGET_NAME} INTERFACE c_std_99) as well?
| Configure and build from the top of the ScalableVectorSearch tree; the C API is | ||
| picked up as a subdirectory under `bindings/c`: | ||
|
|
||
| ```bash | ||
| cmake -S . -B build -DCMAKE_BUILD_TYPE=Release |
There was a problem hiding this comment.
Not sure it can be built from top of tree like comment suggests - there is no target in root CMakeLists.txt. Something like cmake -S bindings/c -B build -DCMAKE_BUILD_TYPE=Release would work
This pull request makes significant improvements to the C API bindings for Scalable Vector Search, focusing on consistent header layout, improved CMake integration, and modernization of sample code. The changes standardize header locations, introduce versioned headers, update the CMake installation structure, and refactor usage of result and threadpool interfaces in the C API samples.
Important note:
Header and Installation Layout Improvements:
include/svs/c/instead ofinclude/svs/c_api/, and installation paths have been updated accordingly for consistency and clarity. [1] [2]svs_c_version.his now included, providing version macros for consumers. This header is configured via CMake and installed as part of the public interface. [1] [2]GNUInstallDirsfor standard install locations and sets up public/private include directories correctly for consumers and downstream projects. [1] [2] [3]Sample Code Modernization and API Updates:
svs/c/path and uses the updated API for search results and threadpool interfaces, improving clarity and correctness. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]svs_search_results_tstruct and its associated functions, and threadpool usage is updated for the new interface. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]Documentation Additions:
README.mdis added for the C API, detailing usage, build instructions, API design, and quickstart examples for end users.Header Content Updates:
Summary of the most important changes:
Header and Installation Structure:
include/svs/c/, and installation paths updated to match, replacing the oldc_apidirectory. [1] [2]GNUInstallDirs, sets up correct include paths for build/install, and properly exports the versioned header. [1] [2] [3]svs_c_version.hwith version macros, configured and installed via CMake.Sample and API Modernization:
svs_search_results_tstruct and API, improving code clarity and correctness. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]Documentation:
README.mdfor the C API, including build instructions, usage, and API overview.Header Content: