criteo / autofaiss

Automatically create Faiss knn indices with the most optimal similarity search parameters.
https://criteo.github.io/autofaiss/
Apache License 2.0
793 stars 73 forks source link

[Feature Request:] Add new features to a previously built index #137

Open kushalkafle opened 1 year ago

kushalkafle commented 1 year ago

Right now there does not seem to be an easy way to take an already-built index and add more embeddings to it (from the same distribution). This is obviously already indirectly supported by / possible with autofaiss because distributed training already does it, and also it is something easily supported by FAISS backbone. But I wonder if we can expose an easy interface to take a built index and add more features from a new set of embeddings (Using all the bells and whistles provided by autofaiss/embedding-reader for reading embeddings from a numpy-parquet format). Perhaps a update_index interface?

Thanks!

hitchhicker commented 1 year ago

Hey,

Thanks for suggesting this feature.

Why not ! Working on it...

hitchhicker commented 1 year ago

I close the PR that I created because we will implement something similar on our side, once it is stable, we will consider adding here. Stay tuned.

kushalkafle commented 1 year ago

Thanks a lot for looking into this! The update_index branch seems usable already; I might start using it and will report any hiccups/successes here!

Cheers!

hitchhicker commented 1 year ago

It does have issues, it won't work if the new index_key is different than the one used in the already-built index. Essentially, we need to retrain the index in this case. That’s something I have missed in the branch. Thus, I don't recommend you use this branch right now.

rom1504 commented 1 year ago

if the index_key needs to be changed then it's not possible at all to add items to the index. so why is that method not suitable?

kushalkafle commented 1 year ago

Yes, I have the same thoughts as @rom1504; For many use cases, index_key would not be different and it should just work fine for those cases, right? I can report back about my specific use case soon anyway :) If it does not work, I'll report here, and await the revised PR later.

But even if it has issues that you'd like to solve before merging to the main, I think it is already useful in many cases. So thanks for working on this 👍

hitchhicker commented 1 year ago

I agree what both of you said. It is suitable if we don’t need to update the clustering. What I had in mind is that the new interface can handle two main use cases ideally:

rom1504 commented 1 year ago

Would you only retrain the IVF (clustering) or also the quantizer (OPQ and PQ) ? If it's both, isn't it equivalent to rebuilding the index from scratch?

On Sat, Nov 12, 2022, 02:53 hitchhicker @.***> wrote:

I agree what both of you said. It is suitable if we don’t need to update the clustering. What I had in mind is that the new interface can handle two main use cases ideally:

  • keep the clustering unchanged, add more embedding on it;
  • update the clustering with the existing embedding sand new embedding

@kushalkafle https://github.com/kushalkafle I would be happy to see your feedbacks after using it. Thanks :)

— Reply to this email directly, view it on GitHub https://github.com/criteo/autofaiss/issues/137#issuecomment-1312311651, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAR437QO6SHSGF4LC4HW6MLWH32DNANCNFSM6AAAAAAR36KI5Q . You are receiving this because you were mentioned.Message ID: @.***>

hitchhicker commented 1 year ago

@rom1504 Pardon me for replying late. We would retrain both, it is indeed equivalent to rebuilding the index from scratch. In my opinion, it is possible to provide an interface like update_index branch whose responsibility is only to add more features/embeddings on a built index while keeping the index unchanged. It would be useful for autofaiss's users.

@nateagr is working on incremental indexing right now for our internal use cases, we will revisit this topic soon.

kushalkafle commented 1 year ago

I think I want to chime in here as well. Overall, I think I agree with @rom1504.