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

feat: add doccache #21

Merged
merged 10 commits into from
Jun 21, 2021
Merged

feat: add doccache #21

merged 10 commits into from
Jun 21, 2021

Conversation

cristianmtr
Copy link
Contributor

@cristianmtr cristianmtr commented Jun 15, 2021

@cristianmtr cristianmtr force-pushed the feat-doccahce branch 3 times, most recently from 8377d17 to d4e9c1f Compare June 16, 2021 16:01
@cristianmtr cristianmtr marked this pull request as ready for review June 16, 2021 16:03
Copy link
Member

@maximilianwerk maximilianwerk left a comment

Choose a reason for hiding this comment

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

Some minor requests

# for testing the image
RUN pip install pytest && pytest -s -v tests

ENTRYPOINT ["jina", "pod", "--uses", "config.yml"]
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should become ..."jina", "pea", ..., but it might anyhow change completely when JinaD 2.0 is finished. Opinions @deepankarm ?

Copy link
Member

Choose a reason for hiding this comment

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

it should be jina pea I agree, but the sugar of using executor would make sense

Choose a reason for hiding this comment

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

This needs to be decided in core & JinaD would adapt to it. With JinaD 2.0, we still support containerized pods & handle them by just modifying host and mapping ports wherever required.

Comment on lines 10 to 13
RUN pip install -r requirements.txt

# for testing the image
RUN pip install pytest && pytest -s -v tests
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to establish the base from syntax.

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, sure

)

@requests(on='/index')
def index(self, docs: 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.

Name index_or_remove_from_request or something like that? Not sure anyhow.

def update(self, docs: DocumentArray, **kwargs):
"""Update the documents in the cache with the new content

WARNING: If the Document doesn't exist in the cache
Copy link
Member

Choose a reason for hiding this comment

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

Feels like this sentence is not complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -0,0 +1,4 @@
jtype: FileQueryIndexer
Copy link
Member

Choose a reason for hiding this comment

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

C&P error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 1 to 12
manifest_version: 1
name: FileQueryIndexer
kind: indexer
description: A basic file based query indexer
author: Jina AI Dev-Team (dev-team@jina.ai)
url: https://jina.ai
vendor: Jina AI Limited
documentation: https://github.com/jina-ai/jina-hub
version: 0.0.1
license: apache-2.0
keywords: [database, FileQueryIndexer, indexer]
type: pod
Copy link
Member

Choose a reason for hiding this comment

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

Please update

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

@@ -0,0 +1,2 @@
jina==2.0.0rc5.dev54
Copy link
Member

Choose a reason for hiding this comment

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

I believe we are at RC6 already? or even beyond. not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, using master

@cristianmtr
Copy link
Contributor Author

@maximilianwerk recheck, please

@@ -12,4 +12,4 @@ RUN pip install -r requirements.txt
# for testing the image
RUN pip install pytest && pytest -s -v tests

ENTRYPOINT ["jina", "pod", "--uses", "config.yml"]
ENTRYPOINT ["jina", "pea", "--uses", "config.yml"]
Copy link
Member

Choose a reason for hiding this comment

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

I think the FROM base is still missing, right @jakob1111996 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

Copy link
Member

@maximilianwerk maximilianwerk left a comment

Choose a reason for hiding this comment

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

LGTM. Actually, you can ignore my comments about the Dockerfile BASE, FROM syntax. We can do this normalization in a follow-up step.

assert len(self.cache_handler.id_to_hash) == len(
self.cache_handler.hash_to_id
), 'Cache state invalid. Length of mappings does not align. Try rebuilding the index'
def ids(self):
Copy link
Member

Choose a reason for hiding this comment

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

The naming seems to be confusing. Perhaps ids_count if size is no option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed them

jinahub/indexers/indexer/LMDBIndexer/Dockerfile Outdated Show resolved Hide resolved
Copy link
Member

@maximilianwerk maximilianwerk left a comment

Choose a reason for hiding this comment

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

LGTM

@cristianmtr cristianmtr merged commit 37f5bc1 into main Jun 21, 2021
@cristianmtr cristianmtr deleted the feat-doccahce branch June 21, 2021 15:53
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.

4 participants