-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Refactor Faiss-based vector format for easier backport #14934
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
base: main
Are you sure you want to change the base?
Conversation
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
Also ran benchmarks to ensure these changes don't adversely affect performance..
This PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @kaivalnp -- this looks clean to me -- but @uschindler probably has suggestions still ;)
interface FaissLibrary { | ||
FaissLibrary INSTANCE = lookup(); | ||
|
||
// TODO: Use vectorized version where available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm what does this TODO
mean again? Is "vectorized" meaning "SIMD instructions"? This term (vector) is overloaded! https://youtu.be/fVq4_HhBK8Y
Maybe clarify to // TODO: use SIMD Faiss API versions where available
or so (if that's what it really means)? I think this is necessary because the Faiss build process will produce specific dynamic library for specific SIMD targets (AVX-512 vs AVX-128 etc.)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sorry, I meant using SIMD instructions wherever available..
Today, the shared library of Faiss' C API (libfaiss_c.so
) is linked to the non-SIMD version of the base library (libfaiss.so
) by default, but a user can still "point" to the correct SIMD version by changing its dependencies using:
# patchelf --replace-needed OLD_DEPENDENCY NEW_DEPENDENCY SHARED_LIBRARY
patchelf --replace-needed libfaiss.so libfaiss_{avx2,avx512,sve}.so libfaiss_c.so
However, we'd ideally want to do this automatically (either propose a change to upstream Faiss, or something else from Lucene) -- but I wasn't sure how to do it right now..
I'll update the comment soon!
Give me some time to check this. In general I agree with those refactorings, it is much cleaner and exception handling is correct. Also everything which touches native code is encapsulated. This is all strongly needed and should be done without any backporting in mind. It is rwquired to have good code. I don't think we should backport this to our stable 10.x branch as this won't work with Java 21 and still theres complex work to be used to make this compile with Java 21 using APIJARs duplication and so on. My proposal is to release Lucene 11 this autumn with Java 25 (see mailing list thread) where we have no limitations with preview APIs anymore. |
Description
Refactor classes of the Faiss-based vector format to simplify backport to 10.x
FaissLibrary
interfaceFaissNativeWrapper
(and useinvokeExact
for faster calls!)FaissLibraryNativeImpl
class implementingFaissLibrary
withFaissNativeWrapper
under the hoodFaissLibraryNativeImpl
fromFaissLibrary
at runtimeFaissNativeWrapper
andFaissLibraryNativeImpl
(as marked by TODOs in build: enable more of javac 24's lints, fix some issues #14907), which can independently be moved into thejava21/
directory for an easier backport (see comments in Backport Faiss-based vector format to 10.x #14843), while theFaissKnnVectors{Format/Reader/Writer}
can stay as-is!