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.

Sign up for GitHub

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

Jump to bottom

A candidate that uses Faiss 1.7.4 #90

Closed
wants to merge 1 commit into from
Closed

A candidate that uses Faiss 1.7.4 #90

wants to merge 1 commit into from

Conversation

alexanderguzhva
Copy link
Collaborator

@alexanderguzhva alexanderguzhva commented Sep 18, 2023 •
edited
Loading

This is a beta candidate of Milvus with upgraded Faiss 1.7.4 (based on commit c8d6f7bb2b09b8928570222131353f75acb4007b from Sep 6 2023, but with all the most recent merged commits).

kind/enhancement

@mergify
Copy link

mergify bot commented Sep 18, 2023

@alexanderguzhva 🔍 Important: PR Classification Needed!

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

  1. If you're fixing a bug, label it as kind/bug.
  2. For small tweaks (less than 20 lines without altering any functionality), please use kind/improvement.
  3. Significant changes that don't modify existing functionalities should be tagged as kind/enhancement.
  4. 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!.

@alexanderguzhva alexanderguzhva changed the title [do-not-merge] A candidate that uses Faiss 1.7.4 (early Sep 2023 version) [do-not-merge] A candidate that uses Faiss 1.7.4 (06 Sep 2023 version) Sep 18, 2023
@alexanderguzhva alexanderguzhva force-pushed the faiss_174_upgrade_candidate branch 2 times, most recently from f1b55de to 36097a2 Compare September 20, 2023 18:27
@mergify mergify bot added needs-dco and removed dco-passed labels Sep 20, 2023
@alexanderguzhva alexanderguzhva force-pushed the faiss_174_upgrade_candidate branch from 36097a2 to 5e42726 Compare September 20, 2023 18:33
@mergify mergify bot added dco-passed and removed needs-dco labels Sep 20, 2023
@alexanderguzhva alexanderguzhva force-pushed the faiss_174_upgrade_candidate branch from 5e42726 to 2e4a50c Compare September 20, 2023 18:44
@mergify mergify bot added needs-dco and removed dco-passed labels Sep 20, 2023
@alexanderguzhva alexanderguzhva force-pushed the faiss_174_upgrade_candidate branch from 2e4a50c to 8c6599f Compare September 20, 2023 19:51
@mergify mergify bot added dco-passed and removed needs-dco labels Sep 20, 2023
@alexanderguzhva alexanderguzhva force-pushed the faiss_174_upgrade_candidate branch from 8c6599f to 6549c29 Compare September 25, 2023 14:04
@mergify mergify bot added needs-dco and removed dco-passed labels Sep 25, 2023
@alexanderguzhva alexanderguzhva force-pushed the faiss_174_upgrade_candidate branch from 6549c29 to a2c7da0 Compare September 25, 2023 14:15
@mergify mergify bot added dco-passed and removed needs-dco labels Sep 25, 2023
@alexanderguzhva alexanderguzhva force-pushed the faiss_174_upgrade_candidate branch 4 times, most recently from bdd33e4 to 020925f Compare September 26, 2023 17:04
liliu-z
liliu-z reviewed Nov 1, 2023
View reviewed changes
src/simd/distances_avx.cc
@@ -38,80 +40,34 @@ masked_read(int d, const float* x) {
// cannot use AVX2 _mm_mask_set1_epi32
}

// trust the compiler to unroll this properly
FAISS_PRAGMA_IMPRECISE_FUNCTION_BEGIN
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my understanding, compiler will auto gen SIMD code for this code block, questions:

  1. Any version limit for compiler?
  2. How can compiler know AVX2 or AVX512 to use?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. auto-vectorizers exists in almost every compiler. The quality depends on the compiler version & family, but for a simple code like this one every compiler should work
  2. depending on the flags that are used for compiling a particular file (-mavx, -mavx2, -mavx512f, -mfma)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let me know which compiler versions are going to be used.

@alexanderguzhva alexanderguzhva force-pushed the faiss_174_upgrade_candidate branch 2 times, most recently from 609c978 to b5544f1 Compare November 1, 2023 16:26
@mergify mergify bot added the ci-passed label Nov 1, 2023
@alexanderguzhva alexanderguzhva force-pushed the faiss_174_upgrade_candidate branch from b5544f1 to ba2e70a Compare November 2, 2023 14:26
@mergify mergify bot added ci-passed and removed ci-passed labels Nov 2, 2023
@alexanderguzhva alexanderguzhva force-pushed the faiss_174_upgrade_candidate branch from ba2e70a to 03233c2 Compare November 2, 2023 17:24
@mergify mergify bot added ci-passed and removed ci-passed labels Nov 2, 2023
@liliu-z
Copy link
Collaborator

liliu-z commented Nov 4, 2023

/approve

@liliu-z
Copy link
Collaborator

liliu-z commented Nov 4, 2023

/lgtm

@liliu-z
Copy link
Collaborator

liliu-z commented Nov 6, 2023

/approve

@sre-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexanderguzhva, liliu-z

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

@alexanderguzhva alexanderguzhva force-pushed the faiss_174_upgrade_candidate branch from 03233c2 to 41e3245 Compare November 6, 2023 15:17
@sre-ci-robot sre-ci-robot removed the lgtm label Nov 6, 2023
@sre-ci-robot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@mergify mergify bot removed the ci-passed label Nov 6, 2023
@alexanderguzhva alexanderguzhva mentioned this pull request Nov 6, 2023
Merged
@alexanderguzhva
Copy link
Collaborator Author

Republished as #181

@chasingegg
Copy link
Collaborator

Close this pr as #181 be merged.

@chasingegg chasingegg closed this Nov 9, 2023
@mergify mergify bot added needs-dco and removed dco-passed labels Apr 24, 2024
@mergify Mergify
Copy link

mergify bot commented Apr 24, 2024

@alexanderguzhva 🔍 Important: PR Classification Needed!

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

  1. If you're fixing a bug, label it as kind/bug.
  2. For small tweaks (less than 20 lines without altering any functionality), please use kind/improvement.
  3. Significant changes that don't modify existing functionalities should be tagged as kind/enhancement.
  4. 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!.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liliu-z liliu-z liliu-z left review comments

@Presburger Presburger Presburger left review comments

@cqy123456 cqy123456 cqy123456 left review comments

@chasingegg chasingegg chasingegg left review comments

@foxspy foxspy Awaiting requested review from foxspy

Assignees

@liliu-z liliu-z

Labels
approved do-not-merge/missing-related-issue needs-dco size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alexanderguzhva @liliu-z @sre-ci-robot @chasingegg @cqy123456 @Presburger