[Bugfix] Add patch for bf16 distance computation

zilliztech / knowhere

Knowhere is an open-source vector search engine, integrating FAISS, HNSW, etc.

Apache License 2.0

135 stars 57 forks source link

[Bugfix] Add patch for bf16 distance computation #521

Closed foxspy closed 3 weeks ago

foxspy commented 3 weeks ago

issue: #519 /kind enhancement

sre-ci-robot commented 3 weeks ago

@foxspy: The label(s) kind/bugfix cannot be applied, because the repository doesn't have them.

In response to [this](https://github.com/zilliztech/knowhere/pull/521): >issue: #519 >/kind bugfix Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

sre-ci-robot commented 3 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: foxspy

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: - ~~[OWNERS](https://github.com/zilliztech/knowhere/blob/2.2/OWNERS)~~ [foxspy] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

mergify[bot] commented 3 weeks ago

@foxspy 🔍 Important: PR Classification Needed!

For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:

If you're fixing a bug, label it as kind/bug.
For small tweaks (less than 20 lines without altering any functionality), please use kind/improvement.
Significant changes that don't modify existing functionalities should be tagged as kind/enhancement.
Adjusting APIs or changing functionality? Go with kind/feature.

For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”.

Thanks for your efforts and contribution to the community!.

foxspy commented 3 weeks ago

/kind enhancement

PwzXxm commented 3 weeks ago

Waiting for @hhy3 double-check.

alexanderguzhva commented 3 weeks ago

This looks Horrible, because it is a really dirty hack.

It is almost impossible to identify the reason of why such a patch is needed without comments in the code (which are missing).
Why the patch is applied to y, but not in x in distance(const float* x, const float* y, const size_t dim).
Why is it needed to apply the patch every time instead of prepatching y somewhere else
Why it is not possible to patch a query vector on the level of milvus-to-knowhere parameters passing instead of patching all the distance functions.
What is going to happen in PPC64, which could have a big-endian format.

Too many questions and too few answers.

alexanderguzhva commented 3 weeks ago

or, at least, one could introduce a different set of distance computation functions (which include this patch) and assign different distance pointers in simdhook. This won't hurt the performance and the code becomes a bit more clear.

foxspy commented 3 weeks ago

/hold