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

feat: add faiss indexer #19

Merged
merged 1 commit into from
Jun 16, 2021
Merged

feat: add faiss indexer #19

merged 1 commit into from
Jun 16, 2021

Conversation

cristianmtr
Copy link
Contributor

@cristianmtr cristianmtr commented Jun 14, 2021

@cristianmtr cristianmtr force-pushed the feat-faiss-indexer branch 4 times, most recently from bd8ab70 to 9f7477e Compare June 15, 2021 13:42
@cristianmtr cristianmtr marked this pull request as ready for review June 15, 2021 13:42
Copy link
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

Also, let's add the capacity to use integration tests

jinahub/indexers/query/vector/FaissIndexer/__init__.py Outdated Show resolved Hide resolved
@cristianmtr cristianmtr force-pushed the feat-faiss-indexer branch 3 times, most recently from 211f288 to 9727a47 Compare June 15, 2021 14:23
@cristianmtr cristianmtr requested a review from JoanFM June 15, 2021 14:25
return

@requests(on='/search')
def query(self, docs: DocumentArray, parameters: Dict = None, *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if it matches the signature but maybe add Optional to parameters type hint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

jinahub/indexers/query/vector/FaissIndexer/__init__.py Outdated Show resolved Hide resolved
normalize: bool = False,
nprobe: int = 1,
dump_path: Optional[str] = None,
traverse_path: str = 'r',
Copy link
Member

Choose a reason for hiding this comment

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

we have to agree if this is default_traversal_path or what, but we should be consistent. Also this should be a Union[str, List[str]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why we should actually have another class, Indexer to do this consistently.

I'll name it default_ for this

return 0

@requests(on='/fill_embedding')
def fill_embedding(self, query_da: DocumentArray, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

this should be called docs as per any function of any executor that has an endpoint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

@cristianmtr
Copy link
Contributor Author

@JoanFM recheck

@cristianmtr cristianmtr requested a review from JoanFM June 16, 2021 07:31
normalize: bool = False,
nprobe: int = 1,
dump_path: Optional[str] = None,
default_traversal_path: str = 'r',
Copy link
Member

Choose a reason for hiding this comment

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

it can be a List also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, missed it

@cristianmtr cristianmtr requested a review from JoanFM June 16, 2021 07:34
@cristianmtr
Copy link
Contributor Author

Also, let's add the capacity to use integration tests

What do you mean? There are integration tests and they are always run in the CI

@cristianmtr cristianmtr merged commit 23731c7 into main Jun 16, 2021
@cristianmtr cristianmtr deleted the feat-faiss-indexer branch June 16, 2021 08:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants