Add missing HNSW bindings - #2307
Conversation
julianmi
added
feature request
| */ | ||
| @Override | ||
| public SearchResults search(HnswQuery query) throws Throwable { | ||
| synchronized (lifecycleLock) { |
There was a problem hiding this comment.
If I understand the intent correctly, this synchronized block was added to ensure a clean shutdown. However, it also appears to serialize all searches, preventing concurrent execution. If we want to enforce shutdown semantics here, a read-write lock may be a better fit. That said, at least in Lucene this concern is usually delegated to the caller. The caller typically tracks in-flight requests via reference counting (or a similar mechanism) and ensures that shutdown does not proceed until all outstanding requests have completed. So adding it here might duplicate an effort in some systems.
There was a problem hiding this comment.
Thank you and sorry that I missed this comment. I ran into an issues with a test that I've added and added the synchronization. I've reverted this change.
# Conflicts: # python/cuvs/cuvs/tests/test_hnsw.py # rust/cuvs-sys/src/bindings.rs
dantegd
left a comment
dantegd
left a comment
There was a problem hiding this comment.
lgtm, but it'd be a good idea to also update fern/pages/cuvs_bench/param_tuning.md?. The cuvs_hnsw section currently describes builds as always using ACE, but it would be good to reflect that the C++ HNSW build path automatically uses an in-memory CAGRA build when sufficient memory is available and falls back to ACE otherwise. This PR exposes that existing behavior through the bindings, so it would be good for the tuning guide to reflect it. What do you think?
Good catch. I updated the |
4 participants
Add the missing HNSW bindings and cleans up the changes in #1719.
Implements feature #2156. More work might be needed to better fit the recall curve provided by HNSW. See #2031. CC @achirkin