Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a jaccard distance problem that was causing #192 #195

Merged

Conversation

alexanderguzhva
Copy link
Collaborator

This diff fixes #192 . The version of Knowhere with Faiss 1.7.4 was using a wrong piece for Jaccard distance in IndexFlat. Which lead to the following problem in Clustering.cpp in Faiss:

[----------] 1 test from BinaryVecIndex
[ RUN      ] BinaryVecIndex.All
I20231115 16:48:20.364634 18504 factory.cc:20] [KNOWHERE][Create][index_builder_t] create knowhere index BIN_IVF_FLAT with version 1
I20231115 16:48:20.364969 18504 index.cc:30] [KNOWHERE][LoadConfig][index_builder_t] Build config dump: {"dim":"16","index_engine_version":"1","index_type":"BIN_IVF_FLAT","metric_type":"JACCARD","nlist":16}
W20231115 16:48:20.365072 18504 ivf.cc:209] [KNOWHERE][MatchNlist][index_builder_t] nlist(16) is too large, adjust to a proper value
W20231115 16:48:20.365111 18504 ivf.cc:211] [KNOWHERE][MatchNlist][index_builder_t] Row num 10 match nlist 1
WARNING clustering 10 points to 1 centroids: please provide at least 39 training points
index_builder_test: /home/ubuntu/zilliz/milvus/milvus/cmake_build/thirdparty/knowhere/knowhere-src/thirdparty/faiss/faiss/Clustering.cpp:154: void faiss::{anonymous}::compute_centroids(size_t, size_t, size_t, size_t, const uint8_t*, const faiss::Index*, const int64_t*, const float*, float*, float*): Assertion `ci >= 0 && ci < k + k_frozen' failed.

Thread 1 "index_builder_t" received signal SIGABRT, Aborted.
__pthread_kill_implementation (no_tid=0, signo=6, threadid=140737278448064) at ./nptl/pthread_kill.c:44
44	./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=140737278448064) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=140737278448064) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=140737278448064, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007ffff0e42476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ffff0e287f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x00007ffff0e2871b in __assert_fail_base (fmt=0x7ffff0fdd150 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
    assertion=0x7ffff0588b1e "ci >= 0 && ci < k + k_frozen", 
    file=0x7ffff05883e0 "/home/ubuntu/zilliz/milvus/milvus/cmake_build/thirdparty/knowhere/knowhere-src/thirdparty/faiss/faiss/Clustering.cpp", line=154, function=<optimized out>) at ./assert/assert.c:92
#6  0x00007ffff0e39e96 in __GI___assert_fail (assertion=0x7ffff0588b1e "ci >= 0 && ci < k + k_frozen", 
    file=0x7ffff05883e0 "/home/ubuntu/zilliz/milvus/milvus/cmake_build/thirdparty/knowhere/knowhere-src/thirdparty/faiss/faiss/Clustering.cpp", line=154, 
    function=0x7ffff0588a80 "void faiss::{anonymous}::compute_centroids(size_t, size_t, size_t, size_t, const uint8_t*, const faiss::Index*, const int64_t*, const float*, float*, float*)") at ./assert/assert.c:101
#7  0x00007fffefbf0810 in _ZN5faiss12_GLOBAL__N_117compute_centroidsEmmmmPKhPKNS_5IndexEPKlPKfPfSA_._omp_fn.0(void) ()
    at /home/ubuntu/zilliz/milvus/milvus/cmake_build/thirdparty/knowhere/knowhere-src/thirdparty/faiss/faiss/Clustering.cpp:154
#8  0x00007ffff6e53a16 in GOMP_parallel () from /lib/x86_64-linux-gnu/libgomp.so.1
#9  0x00007fffefbecf5c in faiss::(anonymous namespace)::compute_centroids (d=16, k=1, n=10, k_frozen=0, 
    x=0x7fffeb8fc4e0 "f\217\271\367j\331q\310", <incomplete sequence \311>, codec=0x7fffffffd700, assign=0x7fffeb83f860, 
    weights=0x0, hassign=0x7fffeb84b1e8, centroids=0x7fffeb962800)
    at /home/ubuntu/zilliz/milvus/milvus/cmake_build/thirdparty/knowhere/knowhere-src/thirdparty/faiss/faiss/Clustering.cpp:142
#10 0x00007fffefbef208 in faiss::Clustering::train_encoded (this=0x7fffffffd510, nx=10, 
    x_in=0x7fffeb8fc4e0 "f\217\271\367j\331q\310", <incomplete sequence \311>, codec=0x7fffffffd700, index=..., weights=0x0)
    at /home/ubuntu/zilliz/milvus/milvus/cmake_build/thirdparty/knowhere/knowhere-src/thirdparty/faiss/faiss/Clustering.cpp:598
#11 0x00007fffefbfb1c8 in faiss::IndexBinaryIVF::train (this=0x7fffeb8ff000, n=10, 
    x=0x7fffeb8fc4e0 "f\217\271\367j\331q\310", <incomplete sequence \311>)
    at /home/ubuntu/zilliz/milvus/milvus/cmake_build/thirdparty/knowhere/knowhere-src/thirdparty/faiss/faiss/IndexBinaryIVF.cpp:317
#12 0x00007fffefbd239b in knowhere::IvfIndexNode<faiss::IndexBinaryIVF>::Train (this=0x7fffeb923dc0, dataset=..., cfg=...)
    at /home/ubuntu/zilliz/milvus/milvus/cmake_build/thirdparty/knowhere/knowhere-src/src/index/ivf/ivf.cc:417
#13 0x00007fffefb58519 in knowhere::IndexNode::Build (this=0x7fffeb923dc0, dataset=..., cfg=...)
    at /home/ubuntu/zilliz/milvus/milvus/cmake_build/thirdparty/knowhere/knowhere-src/include/knowhere/index_node.h:41
#14 0x00007fffefb4a7a6 in knowhere::Index<knowhere::IndexNode>::Build (this=0x7fffeb8e5de0, dataset=..., json=...)
    at /home/ubuntu/zilliz/milvus/milvus/cmake_build/thirdparty/knowhere/knowhere-src/src/common/index.cc:44
#15 0x00007ffff0c32978 in milvus::index::VectorMemIndex<unsigned char>::BuildWithDataset (this=0x7fffeb8e5d80, 
    dataset=std::shared_ptr<knowhere::DataSet> (use count 1, weak count 0) = {...}, config=...)
    at /home/ubuntu/zilliz/milvus/milvus/internal/core/src/index/VectorMemIndex.cpp:237
#16 0x00007ffff6f69ca9 in milvus::indexbuilder::VecIndexCreator::Build (this=0x7fffeb923d00, 
    dataset=std::shared_ptr<knowhere::DataSet> (use count 1, weak count 0) = {...})
    at /home/ubuntu/zilliz/milvus/milvus/internal/core/src/indexbuilder/VecIndexCreator.cpp:47
#17 0x00007ffff6f76500 in BuildBinaryVecIndex (index=0x7fffeb923d00, data_size=20, 
    vectors=0x7fffeb8fc4e0 "f\217\271\367j\331q\310", <incomplete sequence \311>)
    at /home/ubuntu/zilliz/milvus/milvus/internal/core/src/indexbuilder/index_c.cpp:196

I've added a unit test for this particular case.

/kind bug

@liliu-z
Copy link
Collaborator

liliu-z commented Nov 17, 2023

Should contribute back to FAISS?

@liliu-z
Copy link
Collaborator

liliu-z commented Nov 17, 2023

/lgtm

@cydrain
Copy link
Collaborator

cydrain commented Nov 17, 2023

/approve

@sre-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexanderguzhva, cydrain

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot sre-ci-robot merged commit 7d38b36 into zilliztech:main Nov 17, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After upgrade to faiss-1.7.4, Milvus ut-cpp coredump
4 participants