[C API] Add OpenMP support validation in ThreadPoolBuilder by rfsaliev · Pull Request #412 · intel/ScalableVectorSearch

rfsaliev · 2026-10-06T14:04:46Z

This pull request improves the handling of OpenMP thread pool support in the C bindings, making OpenMP an optional feature that can be toggled at build time. It ensures that the API and documentation clearly indicate when OpenMP is available, returns appropriate errors if OpenMP is requested but not built in, and updates tests to handle both configurations. The main changes are grouped below:

Build System and Feature Flagging:

Added a new SVS_ENABLE_OMP CMake option (enabled by default) to control whether OpenMP thread pool support is built into the library. The build system now only enables OpenMP-related code and definitions if this option is set and OpenMP is found. Default symbol visibility is also set to hidden for shared libraries. [1] [2] [3]

API Behavior and Error Handling:

Updated the thread pool API (svs_index_builder_set_threadpool) and its documentation to clarify that selecting the OpenMP thread pool kind will fail with SVS_ERROR_NOT_IMPLEMENTED if the library was not built with OpenMP support. The implementation now checks for OpenMP support at runtime and throws an appropriate error if unavailable. [1] [2] [3] [4]

Documentation Updates:

Improved documentation in C_API_Design.md and header comments to accurately reflect the conditional availability of OpenMP thread pools and the associated error handling. [1] [2]

Testing Improvements:

Updated C API tests to check for OpenMP support at runtime using a new check_openmp_support helper, allowing tests to pass whether or not OpenMP is enabled in the build. Test code now conditionally executes OpenMP-specific assertions and logic. [1] [2] [3] [4]

Copilot

🟡 Changes recommended

The tests accept either capability state and therefore cannot detect broken OpenMP build configuration.

2 open findings

Test does not detect incorrect OpenMP capability configuration · New
Test fails to verify OpenMP detection compile definition · New

What changed in this PR

Adds optional OpenMP support handling to the C API.

Changes:

Conditionally enables OpenMP and returns SVS_ERROR_NOT_IMPLEMENTED when unavailable.
Updates public documentation for OpenMP thread pools.
Adapts thread-pool tests for OpenMP-disabled builds.

File	Description
`bindings/c/CMakeLists.txt`	Makes OpenMP optional.
`bindings/c/src/threadpool.hpp`	Validates OpenMP availability.
`bindings/c/include/svs/c/svs_c.h`	Documents availability and errors.
`bindings/c/docs/C_API_Design.md`	Updates thread-pool guidance.
`bindings/c/tests/c_api_index.cpp`	Adjusts OMP index tests.
`bindings/c/tests/c_api_index_builder.cpp`	Adjusts OMP builder tests.

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

6 open findings

This code assumes SVS_OMP is always defined (and boolean-like). In this PR, CMake conditionally… · New
SVS_VALIDATE_BOOL_ENV(SVS_OMP) is invoked without a trailing semicolon; if the macro expands to a… · New
Basing SVS_TEST_EXPECT_OPENMP on OpenMP_CXX_FOUND can desynchronize test expectations from the… · New
Test fails to verify OpenMP detection compile definition
Test does not detect incorrect OpenMP capability configuration
The comment says 'OMP is not implemented', but the behavior being tested is 'library was built… · New

🧠 Review effort: Lite

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

7 open findings

The implementation is keyed off SVS_OMP, but the build-system change in this PR defines… · New
The implementation is keyed off SVS_OMP, but the build-system change in this PR defines… · New
Adding -fvisibility=hidden unconditionally can break builds on toolchains that don't support… · New
This is internally inconsistent: if num_threads is ignored for… · New
OpenMP_CXX_FOUND is only guaranteed to be set when find_package(OpenMP ...) has been run in a… · New
The OpenMP unsupported path is handled twice (in validate() and again in build()). This… · New
The OpenMP unsupported path is handled twice (in validate() and again in build()). This… · New

6 resolved since last review

🧠 Review effort: Lite

ethanglaser

Overall LGTM

ethanglaser · 2026-10-13T23:54:31Z

+find_package(OpenMP)
+if (OpenMP_CXX_FOUND)
+    # PRIVATE: OpenMP is an implementation detail linked into the shared library.
+    # Exporting it would force consumers of the C ABI to resolve a C++ OpenMP
+    # target they never asked for.
+    target_link_libraries(${TARGET_NAME} PRIVATE OpenMP::OpenMP_CXX)
+    target_compile_definitions(${TARGET_NAME} PRIVATE SVS_ENABLE_OMP=1)
+endif() # OpenMP_CXX_FOUND


Only concern here is that OpenMP could be silently not found. What if instead the findpackage was moved inside a conditional related to the flag, something like:

Suggested change

find_package(OpenMP)

if (OpenMP_CXX_FOUND)

# PRIVATE: OpenMP is an implementation detail linked into the shared library.

# Exporting it would force consumers of the C ABI to resolve a C++ OpenMP

# target they never asked for.

target_link_libraries(${TARGET_NAME} PRIVATE OpenMP::OpenMP_CXX)

target_compile_definitions(${TARGET_NAME} PRIVATE SVS_ENABLE_OMP=1)

endif() # OpenMP_CXX_FOUND

if (SVS_C_API_ENABLE_OPENMP)

find_package(OpenMP REQUIRED)

# PRIVATE: OpenMP is an implementation detail linked into the shared library.

# Exporting it would force consumers of the C ABI to resolve a C++ OpenMP

# target they never asked for.

target_link_libraries(${TARGET_NAME} PRIVATE OpenMP::OpenMP_CXX)

target_compile_definitions(${TARGET_NAME} PRIVATE SVS_ENABLE_OMP=1)

endif() # OpenMP_CXX_FOUND

Addressed by adding the option SVS_ENABLE_OMP which is ON by default

mergify · 2026-10-13T23:55:39Z

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

Queue this pull request

izaakk · 2026-10-15T19:14:01Z

LGTM, verified from the consumer side. valkey-search compiles the C API sources itself without OpenMP and has been force-including a stub for the unguarded OMPThreadPool reference (eshenayo/valkey-search#19). With these commits on our pin 8285340 and the stub deleted, the build is clean in Release and Debug, the link gate passes with no libgomp, and all tests pass. This is exactly the guard we needed.

rfsaliev requested review from ahuber21 and ethanglaser as code owners October 6, 2026 14:04

rfsaliev linked an issue Oct 6, 2026 that may be closed by this pull request

[C API] Guard SVS_THREADPOOL_KIND_OMP case in threadpool.hpp behind #if SVS_OMP #399

Closed

rfsaliev requested a balanced review from Copilot October 8, 2026 14:08

Copilot started reviewing on behalf of rfsaliev October 8, 2026 14:09

View session

Copilot AI reviewed Oct 8, 2026

View reviewed changes

Comment thread bindings/c/tests/c_api_index.cpp Outdated

Comment thread bindings/c/tests/c_api_index_builder.cpp Outdated

rfsaliev force-pushed the rfsaliev/c-api-fix-omp branch from 546debd to 6bc4d2a Compare October 12, 2026 10:27

rfsaliev requested a balanced review from Copilot October 12, 2026 10:28

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Comment thread bindings/c/src/threadpool.hpp Outdated

Comment thread bindings/c/src/threadpool.hpp

Comment thread bindings/c/tests/CMakeLists.txt Outdated

Comment thread bindings/c/tests/c_api_test_utils.h

Copilot stopped reviewing on behalf of rfsaliev due to an error October 12, 2026 11:49
An unexpected error occurred. For more details, see the detailed logs in GitHub Actions.

rfsaliev force-pushed the rfsaliev/c-api-fix-omp branch from 6bc4d2a to 935a534 Compare October 13, 2026 09:18

rfsaliev requested a balanced review from Copilot October 13, 2026 13:33

Copilot AI reviewed Oct 13, 2026

View reviewed changes

Copilot stopped reviewing on behalf of rfsaliev due to an error October 13, 2026 14:13
The job was not acquired by a runner even after multiple attempts.

ethanglaser approved these changes Oct 13, 2026

View reviewed changes

rfsaliev added 3 commits October 15, 2026 10:13

[C API] Add OpenMP support validation in ThreadPoolBuilder

c35c1d7

Improve OMP support detection in C API tests

388d4da

Address code review

1674200

rfsaliev force-pushed the rfsaliev/c-api-fix-omp branch from 935a534 to 1674200 Compare October 15, 2026 17:19

rfsaliev requested a review from mihaic as a code owner October 15, 2026 17:19

ethanglaser reviewed Oct 15, 2026

View reviewed changes

Comment thread bindings/c/tests/CMakeLists.txt Outdated

base OpenMP test expectation on SVS_ENABLE_OMP

019b585

rfsaliev merged commit fd23725 into main Oct 16, 2026
19 of 21 checks passed

rfsaliev deleted the rfsaliev/c-api-fix-omp branch October 16, 2026 09:17

eshenayo mentioned this pull request Oct 16, 2026

build(svs): bump SVS to main cde3771 and drop the OpenMP shim eshenayo/valkey-search#35

Merged

Conversation

rfsaliev commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ethanglaser left a comment

Choose a reason for hiding this comment

Uh oh!

ethanglaser Oct 13, 2026 • 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!

rfsaliev Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Oct 13, 2026

Uh oh!

izaakk commented Oct 15, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rfsaliev commented Oct 6, 2026 •

edited

Loading

ethanglaser Oct 13, 2026 •

edited

Loading