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

Upgrade faiss to 1.8.0 #453

Merged
merged 1 commit into from
Apr 18, 2024
Merged

Upgrade faiss to 1.8.0 #453

merged 1 commit into from
Apr 18, 2024

Conversation

alexanderguzhva
Copy link
Collaborator

@alexanderguzhva alexanderguzhva commented Mar 11, 2024 •
edited
Loading

Upgrade Faiss to 1.8.0
/kind improvement

@sre-ci-robot sre-ci-robot requested review from foxspy and liliu-z March 11, 2024 18:09
@mergify Mergify
Copy link

mergify bot commented Mar 11, 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!.

@alexanderguzhva alexanderguzhva changed the title Merge commits from Faiss master branch (upgrading Faiss to 1.8.0) Merge commits from Faiss master branch (upgrading Faiss to 1.8.0, part 1) Mar 12, 2024
@alexanderguzhva alexanderguzhva changed the title Merge commits from Faiss master branch (upgrading Faiss to 1.8.0, part 1) [in progress] Merge commits from Faiss master branch (upgrading Faiss to 1.8.0 ...) Mar 14, 2024
@mergify mergify bot added the ci-passed label Mar 14, 2024
@alexanderguzhva alexanderguzhva force-pushed the merge_faiss_changes_from_baseline_v2a branch from a850048 to 708a405 Compare March 14, 2024 13:35
@mergify mergify bot removed the ci-passed label Mar 14, 2024
@alexanderguzhva alexanderguzhva changed the title [in progress] Merge commits from Faiss master branch (upgrading Faiss to 1.8.0 ...) Upgrade faiss to 1.8.0 Mar 14, 2024
@alexanderguzhva alexanderguzhva force-pushed the merge_faiss_changes_from_baseline_v2a branch 2 times, most recently from 3918ae3 to e641053 Compare March 14, 2024 16:25
@mergify mergify bot added the ci-passed label Mar 14, 2024
@alexanderguzhva alexanderguzhva force-pushed the merge_faiss_changes_from_baseline_v2a branch from e641053 to 6ba1024 Compare March 21, 2024 16:01
@mergify mergify bot removed the ci-passed label Mar 21, 2024
@alexanderguzhva alexanderguzhva force-pushed the merge_faiss_changes_from_baseline_v2a branch from 6ba1024 to 404c25f Compare March 21, 2024 17:00
@alexanderguzhva alexanderguzhva changed the title Upgrade faiss to 1.8.0 [in progress] Upgrade faiss to 1.8.0 Mar 21, 2024
@alexanderguzhva alexanderguzhva force-pushed the merge_faiss_changes_from_baseline_v2a branch 3 times, most recently from 15be1bf to 4808643 Compare April 9, 2024 20:36
@mergify mergify bot added the ci-passed label Apr 9, 2024
cydrain
cydrain reviewed Apr 11, 2024
View reviewed changes
thirdparty/faiss/faiss/impl/ScalarQuantizerCodec_avx.h
@@ -284,8 +284,8 @@ struct SimilarityL2_avx<8> {
accu8 = _mm256_fmadd_ps(tmp, tmp, accu8);
}

FAISS_ALWAYS_INLINE void add_8_components_2(__m256 x, __m256 y) {
__m256 tmp = _mm256_sub_ps(y, x);
FAISS_ALWAYS_INLINE void add_8_components_2(__m256 x, __m256 y_2) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why need rename to "y_2" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these were semi-automatic changes across the whole Faiss codebase. Let me cite it:

Shadowed variables are variables in an inner scope with the same name as another variable in an outer scope. Having the same name for both variables might be semantically correct, but it can make the code confusing to read! It can also hide subtle bugs.

@cydrain
Copy link
Collaborator

cydrain commented Apr 11, 2024

/lgtm

@cydrain
Copy link
Collaborator

cydrain commented Apr 11, 2024

/hold
will merge to main branch after Milvus 2.4 released

@alexanderguzhva alexanderguzhva mentioned this pull request Apr 17, 2024
Merged
@alexanderguzhva alexanderguzhva force-pushed the merge_faiss_changes_from_baseline_v2a branch from 4808643 to 96df3b4 Compare April 17, 2024 21:05
@sre-ci-robot sre-ci-robot removed the lgtm label Apr 17, 2024
@mergify mergify bot removed the ci-passed label Apr 17, 2024
@alexanderguzhva
Copy link
Collaborator Author

rebase on top of master

@codecov Codecov
Copy link

codecov bot commented Apr 17, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.36%. Comparing base (3c46f4c) to head (a67ef6c).
Report is 9 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           main     #453       +/-   ##
=========================================
+ Coverage      0   72.36%   +72.36%     
=========================================
  Files         0       63       +63     
  Lines         0     4291     +4291     
=========================================
+ Hits          0     3105     +3105     
- Misses        0     1186     +1186

see 63 files with indirect coverage changes

@mergify mergify bot added the ci-passed label Apr 17, 2024
@alexanderguzhva
Upgrade Faiss to 1.8.0
a67ef6c 
Signed-off-by: Alexandr Guzhva <[email protected]>
@alexanderguzhva alexanderguzhva force-pushed the merge_faiss_changes_from_baseline_v2a branch from 96df3b4 to a67ef6c Compare April 18, 2024 13:20
@alexanderguzhva alexanderguzhva changed the title [in progress] Upgrade faiss to 1.8.0 Upgrade faiss to 1.8.0 Apr 18, 2024
@alexanderguzhva
Copy link
Collaborator Author

rebased to master

@mergify mergify bot removed the ci-passed label Apr 18, 2024
@liliu-z
Copy link
Collaborator

liliu-z commented Apr 18, 2024

/lgtm

@liliu-z
Copy link
Collaborator

liliu-z commented Apr 18, 2024

/unhold

@liliu-z
Copy link
Collaborator

liliu-z commented Apr 18, 2024

/kind improvement

@liliu-z
Copy link
Collaborator

liliu-z commented Apr 18, 2024

/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

@sre-ci-robot sre-ci-robot merged commit ad6f71f into zilliztech:main Apr 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cydrain cydrain cydrain left review comments

@foxspy foxspy Awaiting requested review from foxspy

@liliu-z liliu-z Awaiting requested review from liliu-z

Assignees

@cydrain cydrain

@liliu-z liliu-z

Labels
approved ci-passed dco-passed kind/improvement lgtm size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexanderguzhva @cydrain @liliu-z @sre-ci-robot