fix: fail to load ivf_flat(metric = cosine) with DeserializeFromFile #841
Conversation
|
@cqy123456 🔍 Important: PR Classification Needed! For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:
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!. |
cqy123456
force-pushed
the
cosine-mmap
branch
2 times, most recently
from
aa8e6db
to
96f7a06
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #841 +/- ##
=========================================
+ Coverage 0 75.40% +75.40%
=========================================
Files 0 86 +86
Lines 0 6318 +6318
=========================================
+ Hits 0 4764 +4764
- Misses 0 1554 +1554 |
|
/kind bugfix |
|
@foxspy: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/hold |
| } | ||
| } | ||
|
|
||
| void OnDiskInvertedLists::release_code_norms(size_t list_no, const float* codes) |
There was a problem hiding this comment.
why is it needed to override this method in the provided way?
There was a problem hiding this comment.
Just to clarify the question. It seems that the base class has the same implementation as the added one
There was a problem hiding this comment.
@alexanderguzhva I thought the behavior in release_code_norms is based on the storage method of InvertedList(e.g. ArrayInvertedLists, ConcurrentArrayInvertedLists), so this function is overrided, and this function do nothing like the basic class.
There was a problem hiding this comment.
Then I would eliminate it, because it does nothing.
Or I can lgtm the PR, just let me know. :)
|
@cqy123456 Please remove redundant |
mergify
bot
added
do-not-merge/description-tag-conflict
and removed
do-not-merge/missing-related-issue
labels
|
/unhold |
cqy123456
force-pushed
the
cosine-mmap
branch
from
96f7a06
to
b35d053
Compare
Signed-off-by: cqy123456 <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cqy123456, foxspy, 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 |
issue:milvus-io/milvus#36052
When we set IO_FLAG_WITH_NORM = true to read a faiss index, ArrayInvertedLists will read into a read-only OnDiskInvertedLists.
OnDiskInvertedLists will mmap all ids and vectors with the index file, and record the ids and vectors offset in memory. If we use cosine, l1-norms will save in index file and all offsets are wrong.