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.

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

[faiss v1.7.1] Impl PreTransformIndex #24

Merged
merged 7 commits into from
Jun 21, 2021
Merged

Conversation

ava57r
Copy link
Collaborator

@ava57r ava57r commented May 13, 2021

No description provided.

@ava57r
Copy link
Collaborator Author

ava57r commented May 31, 2021

facebookresearch/faiss#1816

and
facebookresearch/faiss#1869

merged

@ava57r ava57r changed the title [faiss unreleased] Impl PreTransformIndex [faiss v1.7.1] Impl PreTransformIndex May 31, 2021
@Enet4 Enet4 self-requested a review June 1, 2021 08:47
Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

While most of the constructs were well thought out, this PreTransformIndexImpl can be exploited to perform memory unsafety.

  • Values can outlive its native objects if they are not owned.
  • ConcurrentIndex is implemented even if the native index does not.

More details and guidance in the comments below.

src/index/pretransform.rs Outdated Show resolved Hide resolved
src/index/pretransform.rs Outdated Show resolved Hide resolved
@ava57r
Copy link
Collaborator Author

ava57r commented Jun 12, 2021

Maybe we should not use the option when own_fields = = false?

@Enet4
Copy link
Owner

Enet4 commented Jun 13, 2021

Maybe we should not use the option when own_fields = = false?

I mean, I suppose part of the problem would no longer apply if we require all indexes to be owned (and so own_fields would never be false), but adding a few type/lifetime parameters should do the trick here.

@ava57r
Copy link
Collaborator Author

ava57r commented Jun 13, 2021

I won't complicate the impl. I like the transfer of ownership.

@ava57r
Copy link
Collaborator Author

ava57r commented Jun 13, 2021

I suggest starting with the ownership of indexes, quantizers. In the future, it will become clear whether to use own_fields = = false

Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Thank you.

@Enet4 Enet4 merged commit 82843d9 into Enet4:master Jun 21, 2021
@ava57r ava57r deleted the vec-transform branch November 28, 2022 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants