-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[WIP] f16 feature #4032
[WIP] f16 feature #4032
Conversation
💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe/Alipay. |
Hey @charles-r-earp, thanks for the PR! Also, I noticed that you decided to change But recently we introduced the generic support over just vector storage #3900 which might me a more desirable approach after all. P.S. We are hiring |
The primary change is adding the f16 feature which switches VectorElementType to f16. Many operations require converting to f32 or u8 which inspired additional traits to ease implementation. For example, many tests use float literals like
I interpreted #3333 to mean implement exactly that.
It seems possible to limit f16 to only segment::vector_storage instead of segment::data_types::vectors::DenseVector, via PrimitiveVectorElement. Runtime flexibility between storage types is nice. May require refactoring to test both f16 and f32, which is not as easy as a feature gate (though a feature gate might have a lot of redundant tests). In theory there is a cache advantage to f16, even if it requires conversion to f32 for numerical operations. As you mentioned, this has a large impact that may not be worth it, especially without an initial proof of concept. It looks like the storage type is hard coded as VectorElementType in several places, for example segment::vector_storage::simple_dense_vector_storage:
Initially it might be easier to feature gate the storage type. Then it could be exposed as a generic function or have an additional argument for selecting the type. There are still going to be conflicts, as DenseVector and SimpleDenseVectorStorage are defined with VectorElementType. So conversions would be required when inserting or accessing vectors. This should be handled via PrimitiveVectorElement, but doesn't appear to be fully utilized yet. |
my intention was to not expose internal storage type into the interface. So we only do the conversion from f32 into f16 once, when we put the vector into the storage. The main challenge here is to be able to compute distances over f16 vectors directly, avoiding back conversion to f32 |
closing in favor of #4122 |
/claim #3333
Tasks
Test Failures
cargo test -p segment --features f16