Enet4 / faiss-rs

Rust language bindings for Faiss
Apache License 2.0
200 stars 36 forks source link

Support static link as a feature #67

Closed SkyFan2002 closed 1 year ago

SkyFan2002 commented 1 year ago

After enabling the static feature, users do not need to download and build faiss, so it can be used out of the box.

SkyFan2002 commented 1 year ago

Hello! Thank you for this very interesting contribution, It may indeed help some users take advantage of the library.

I have raised some concerns in the comments below, but once they're sorted out we can continue with the review and testing.

Sorry, I pushed the development of generic io to this branch by mistake, I will solve it

SkyFan2002 commented 1 year ago

By the way, it seems that due to the update of bindgen, the gen_bindings.sh is no longer valid

$ ./gen_bindings.sh 
fatal: destination path 'faiss' already exists and is not an empty directory.
ls: cannot access 'faiss/c_api/impl/*_c.h': No such file or directory
ls: cannot access 'faiss/c_api/utils/*_c.h': No such file or directory
bindgen --rust-target 1.33 --size_t-is-usize --whitelist-function faiss_.* --whitelist-type idx_t|Faiss.* --opaque-type FILE c_api.h -o src/bindings.rs
error: unexpected argument '--size_t-is-usize' found

  tip: a similar argument exists: '--no-size_t-is-usize'

Usage: bindgen [FLAGS] [OPTIONS] [HEADER] -- [CLANG_ARGS]...

For more information, try '--help'.
bindgen --rust-target 1.33 --size_t-is-usize --whitelist-function faiss_.* --whitelist-type idx_t|Faiss.* --opaque-type FILE c_api.h -o src/bindings_gpu.rs -- -Ifaiss/c_api -I/opt/cuda/include
error: unexpected argument '--size_t-is-usize' found

  tip: a similar argument exists: '--no-size_t-is-usize'

Usage: bindgen [FLAGS] [OPTIONS] [HEADER] -- [CLANG_ARGS]...

For more information, try '--help'.
$ bindgen --version
bindgen 0.65.1
Enet4 commented 1 year ago

By the way, it seems that due to the update of bindgen, the gen_bindings.sh is no longer valid

Hmm, maybe I should have recorded which version of rust-bindgen I was using. It appears that the behavior _size_t is usize_ has become the default, so the flag --size_t-is-usize can be removed. The rust-bindgen changelog might help track down the latest changes and know what to update here.

The other errors may have to do with faiss now existing as a git submodule.

SkyFan2002 commented 1 year ago

In fact,--whitelist-function is invalid, eithier.

error: unexpected argument '--whitelist-type' found

  tip: a similar argument exists: '--allowlist-type'
Enet4 commented 1 year ago

It should be a matter of following the changelog and the recommendations from the tool itself until it works. For the record, I last used bindgen in version 0.59.2.

SkyFan2002 commented 1 year ago

What can I do to have this PR continued? I apologize for may have been impolite before.

SkyFan2002 commented 1 year ago

Hi, thanks for your help. I have fixed the repeated clone issue. As for the bindgen version, --size_t-is-usize can be deleted directly, but the --whitelist-function and --whitelist-type has been deprecated, and I don't know if there is an API with similar semantics. Do you have any idea about this?Or I am glad to try to solve this in anothr PR.

web3creator commented 1 year ago

@SkyFan2002 Why is this PR closed?I hope to add support for static libraries, but I don't know c language

Enet4 commented 1 year ago

It seems to have compiled seamlessly on my Linux machine. But I wonder, how can one specify more details about the compilation and linking of Faiss without changing the build script? In particular, it should be possible to define some of the options found here, including which BLAS implementation to use, where to find CUDA in case of GPU support, and whether to use the avx2 optimization level. If there is no way to define these options from outside (maybe specially written environment variables?), we should open a way to do this ourselves at faiss-sys, using environment variables and/or Cargo features.

jondot commented 1 year ago

https://github.com/rustformers/llm/blob/main/crates/ggml/sys/build.rs

@Enet4 this is a good reference, it shows a similar library, with factors such as CPU/GPU and advanced CPU instruction toggling, as well as logic by-platform.

it should have all kinds of example of toggling flags for upstream libraries.

jondot commented 1 year ago

@Enet4 I've been playing around (actually pulling hair!) with building an abstraction that depends on faiss-rs, and my impression is that this PR offers a better starting point for dynamic linking and a reasonable static linking experience (I'm on ARM macos so that's already a litmus test). Reasonable being that we can assume that the static linking build -- as it is now in this PR -- does not support advanced options.

All in all, I'm getting a smoother experience with the static build. I've locally updated to 1.7.x with no issues (faiss master), and build went smoothly with a small tweak.

Any thoughts on merging this PR for a starting point for static linking, having that it gives the existing dynamic linking experience an equivalent value?

SkyFan2002 commented 1 year ago

@Enet4 . Let's merge. I'm willing to help if advanced options are needed by someone in the future.