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

Improve RefineFlatIndex #60

Merged
merged 6 commits into from
Dec 14, 2022
Merged

Improve RefineFlatIndex #60

merged 6 commits into from
Dec 14, 2022

Conversation

ava57r
Copy link
Collaborator

@ava57r ava57r commented Oct 18, 2022

No description provided.

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

ava57r commented Nov 14, 2022

I revert your suggestion commits.
there required generic impl trait.

@@ -89,7 +89,7 @@ impl<I> NativeIndex for PreTransformIndexImpl<I> {
}
}

impl<I> FromInnerPtr for PreTransformIndexImpl<I> {
impl<IndexImpl> FromInnerPtr for PreTransformIndexImpl<IndexImpl> {
Copy link
Owner

@Enet4 Enet4 Oct 19, 2022

Choose a reason for hiding this comment

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

If making this impl generic is absolutely necessary, please use type parameter names which does not clash with existing types.

Suggested change
impl<IndexImpl> FromInnerPtr for PreTransformIndexImpl<IndexImpl> {
impl FromInnerPtr for PreTransformIndexImpl<IndexImpl> {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's existing type IndexImpl.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm. In that case it should be this, no? (note the lack of type parameters)

impl FromInnerPtr for PreTransformIndexImpl<IndexImpl> {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does not work. It needs type parameter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We tried it a07ca9b

Copy link
Owner

@Enet4 Enet4 Dec 10, 2022

Choose a reason for hiding this comment

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

OK, so that error was:

error[E0277]: the trait bound `RefineFlatIndexImpl<BI>: index::FromInnerPtr` is not satisfied
   --> src/index/refine_flat.rs:225:20
    |
225 | impl<BI: TryClone> TryClone for RefineFlatIndexImpl<BI> {}
    |                    ^^^^^^^^ the trait `index::FromInnerPtr` is not implemented for `RefineFlatIndexImpl<BI>`
    |
note: required by a bound in `index::TryClone`
   --> src/index/mod.rs:339:21
    |
339 | pub trait TryClone: FromInnerPtr {
    |                     ^^^^^^^^^^^^ required by this bound in `index::TryClone`
help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
    |
[22](https://github.com/Enet4/faiss-rs/actions/runs/3280362393/jobs/5401039258#step:6:23)5 | impl<BI: TryClone> TryClone for RefineFlatIndexImpl<BI> where RefineFlatIndexImpl<BI>: index::FromInnerPtr {}
    |                                                         ++++++++++++++++++++++++++++++++++++++++++++++++++

The error message already suggests a way to fix the problem, but the way I see it, the TryClone trait is overly restrictive right now due to its default implementation of try_clone. So we can either do what the error suggests or move the default implementation onto those that actually depend on it. The steps would be:

  1. Create a helper function try_clone_from_inner_ptr with the default implementation in TryClone::try_clone.
  2. Remove default implementation body of method TryClone::try_clone.
  3. Remove FromInnerPtr as a supertrait of TryClone
  4. In each TryClone implementation with a missing try_clone, add the implementation to make them call try_clone_from_inner_ptr.

Adding a type parameter might make the error go away, but it's not what we want.

@@ -71,7 +71,7 @@ impl<BI> NativeIndex for RefineFlatIndexImpl<BI> {
}
}

impl<BI> FromInnerPtr for RefineFlatIndexImpl<BI> {
impl<IndexImpl> FromInnerPtr for RefineFlatIndexImpl<IndexImpl> {
Copy link
Owner

@Enet4 Enet4 Nov 29, 2022

Choose a reason for hiding this comment

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

Suggested change
impl<IndexImpl> FromInnerPtr for RefineFlatIndexImpl<IndexImpl> {
impl FromInnerPtr for RefineFlatIndexImpl<IndexImpl> {

@Enet4 Enet4 self-requested a review December 10, 2022 11:28
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.

I updated the suggested changes above.

@ava57r
Copy link
Collaborator Author

ava57r commented Dec 10, 2022

I checked in local PC

error[E0277]: the trait bound `PreTransformIndexImpl<I>: index::FromInnerPtr` is not satisfied
   --> src/index/pretransform.rs:221:19
    |
221 | impl<I: TryClone> TryClone for PreTransformIndexImpl<I> {}
    |                   ^^^^^^^^ the trait `index::FromInnerPtr` is not implemented for `PreTransformIndexImpl<I>`
    |
note: required by a bound in `index::TryClone`
   --> src/index/mod.rs:339:21
    |
339 | pub trait TryClone: FromInnerPtr {
    |                     ^^^^^^^^^^^^ required by this bound in `index::TryClone`
help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
    |
221 | impl<I: TryClone> TryClone for PreTransformIndexImpl<I> where PreTransformIndexImpl<I>: index::FromInnerPtr {}
    |                                                         +++++++++++++++++++++++++++++++++++++++++++++++++++

error[E0277]: the trait bound `RefineFlatIndexImpl<BI>: index::FromInnerPtr` is not satisfied
   --> src/index/refine_flat.rs:225:20
    |
225 | impl<BI: TryClone> TryClone for RefineFlatIndexImpl<BI> {}
    |                    ^^^^^^^^ the trait `index::FromInnerPtr` is not implemented for `RefineFlatIndexImpl<BI>`
    |
note: required by a bound in `index::TryClone`
   --> src/index/mod.rs:339:21
    |
339 | pub trait TryClone: FromInnerPtr {
    |                     ^^^^^^^^^^^^ required by this bound in `index::TryClone`
help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
    |
225 | impl<BI: TryClone> TryClone for RefineFlatIndexImpl<BI> where RefineFlatIndexImpl<BI>: index::FromInnerPtr {}
    |                                                         ++++++++++++++++++++++++++++++++++++++++++++++++++

For more information about this error, try `rustc --explain E0277`.
error: could not compile `faiss` due to 2 previous errors

@ava57r
Copy link
Collaborator Author

ava57r commented Dec 10, 2022

I think trait TryClone needs to change

C++ faiss it is function of Index https://github.com/facebookresearch/faiss/blob/main/faiss/clone_index.h

@Enet4
Copy link
Owner

Enet4 commented Dec 10, 2022

Yes, we need to change the TryClone trait so that the trait itself does not depend on FromInnerPtr (and only the implementations do). Following the instructions in my other comment, it should be a smooth change.

remove FromInnerPtr as super trait
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.

All right, it is good to enter now! Thanks! 👍

@Enet4 Enet4 merged commit 333d801 into Enet4:master Dec 14, 2022
@ava57r ava57r deleted the index-downcast-1 branch December 15, 2022 05:41
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