Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Migrate faiss search migrate to jina 0.4.1 #130

Merged
merged 19 commits into from
Aug 6, 2020

Conversation

bwanglzu
Copy link
Member

@bwanglzu bwanglzu commented Jul 31, 2020

This PR was aligned with issue #116 , migrate faiss search example for Jina 0.4.1 release, includes:

  1. Code migration based on migration guide.
  2. Minor changes on Readme.

To run this example, you should build faiss image locally using jina 0.4.1 with Dockerfile:

FROM conda/miniconda3-centos7:latest

WORKDIR /

RUN conda update -n base conda && \
    conda install -y -c pytorch faiss-cpu

COPY faiss.yml .

RUN conda install pip
RUN pip install jina==0.4.1

ENTRYPOINT ["jina", "pod", "--uses", "faiss.yml"]

And run command:

docker build -f Dockerfile -t jinaai/hub.executors.indexers.vector.faiss:latest .

@bwanglzu bwanglzu added the enhancement New feature or request label Jul 31, 2020
@bwanglzu bwanglzu self-assigned this Jul 31, 2020
@JoanFM
Copy link
Member

JoanFM commented Aug 5, 2020

@nan-wang should we merge this example?

@bwanglzu
Copy link
Member Author

bwanglzu commented Aug 5, 2020

@JoanFM one moment, let me test again

@JoanFM
Copy link
Member

JoanFM commented Aug 5, 2020

I proposed change in #138.

When I created this example, I wanted to have a dummy encoder and crafter to showcase a much more common pattern than simply index. It may be able to work directly against the Index.

@JoanFM
Copy link
Member

JoanFM commented Aug 5, 2020

With the new recursive document, this example should be refactored to use only 0-depth-level. We can do it in the future

@YueLiu1415926
Copy link
Contributor

YueLiu1415926 commented Aug 5, 2020

Made some small changes in #139. I included the dockerfile folder into the example, otherwise COPY faiss.yml . on line 8 will show an error.

@bwanglzu
Copy link
Member Author

bwanglzu commented Aug 5, 2020

@YueLiu-jina @JoanFM I saw your PRs, thanks for that.How we are going to proceed?

@JoanFM
Copy link
Member

JoanFM commented Aug 5, 2020

@YueLiu-jina @JoanFM I saw your PRs, thanks for that.How we are going to proceed?

You can merge both into your branch!

CatStark and others added 4 commits August 5, 2020 11:38
This reverts commit fc91fd3, reversing
changes made to 6f263a9.
@JoanFM
Copy link
Member

JoanFM commented Aug 5, 2020

See comment #138 (comment)
Tested it locally, it worked. 2 points to be noted:

  1. Use jina 0.4.1 (or github master branch) to build docker image.
  2. Clean up workspace before you index and query.

lgtm!

@JoanFM
Copy link
Member

JoanFM commented Aug 5, 2020

LGTM!

@JoanFM JoanFM requested a review from nan-wang August 5, 2020 11:15
@bwanglzu
Copy link
Member Author

bwanglzu commented Aug 5, 2020

@YueLiu-jina can you resolve the conflict? Then I'll test again

@YueLiu1415926
Copy link
Contributor

@bwanglzu Merged my branch, I'll give it a test again

@YueLiu1415926 YueLiu1415926 mentioned this pull request Aug 6, 2020
@JoanFM
Copy link
Member

JoanFM commented Aug 6, 2020

Still works?

faiss-search/flow-query.yml Outdated Show resolved Hide resolved
faiss-search/README.md Show resolved Hide resolved
@YueLiu1415926
Copy link
Contributor

it works well, I think this PR is ready to merge

@JoanFM
Copy link
Member

JoanFM commented Aug 6, 2020

@YueLiu-jina, it prints results? can u post a screenshot of what u get in query output? just double checking

@YueLiu1415926
Copy link
Contributor

截屏2020-08-06 下午2 22 30

Is this the effect we want?

@JoanFM
Copy link
Member

JoanFM commented Aug 6, 2020

Good to merge then!

@nan-wang nan-wang merged commit 2b4180d into master Aug 6, 2020
@nan-wang nan-wang deleted the chore-faiss-search-migrate-166 branch August 6, 2020 06:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants