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

Adapt to v2.0.0 core #19

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Adapt to v2.0.0 core #19

wants to merge 9 commits into from

Conversation

alanthssss
Copy link

@alanthssss alanthssss changed the title Adapt to v2.0.0 jina Adapt to v2.0.0 core Jun 11, 2021
@alanthssss alanthssss marked this pull request as ready for review June 13, 2021 09:33
Copy link
Contributor

@jacobowitz jacobowitz left a comment

Choose a reason for hiding this comment

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

Thank you for porting this to 2.0 :)

Main comment is a question about sharding etc, would this be removed by the PR as it is now?

Maybe another question is if it makes sense to generalize the Executors implemented here and provide them as standalone Executors?

with f:
f.index(inputs=DocumentArray([d1, d2, d3]))

with Flow.load_config(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this file in the PR? Looks like some local test to me which will not be used in the actual stress test? If you think its handy to have as some local test entry point, maybe generalize this and otherwise remove?

Copy link
Member

@bwanglzu bwanglzu Jun 14, 2021

Choose a reason for hiding this comment

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

@jacobowitz this will be removed after flow runs successfully, only for test purpose, shouldn't commit to the PR, good catch!

Copy link
Author

Choose a reason for hiding this comment

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

thx for your review.
This PR is still in local progress.

pods:
- name: segmenter
polling: any
shards: {{ JINA_SEGMENTER_SHARDS }}
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we removing the sharding? Testing this as part of the stress test is one of the added values here I think?

Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Author

Choose a reason for hiding this comment

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

Maybe I should convert this PR to a draft one.
Let me fix the status of this PR.

@bwanglzu
Copy link
Member

bwanglzu commented Jun 14, 2021

@niuzs-alan things needs to be done (from the flow/executor level):

  1. Remove the Segmenter from search flow. Otherwise the query document will be splitted into n chunks as well.
  2. Finish the ranker, score is the average of all chunks given the same parent it (aggregating).

@alanthssss alanthssss marked this pull request as draft June 15, 2021 04:28
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.

3 participants