fix: sharding parallel one #1657

florian-hoenicke · 2021-01-12T00:20:32Z

The following case leads to pea_id == -1:

 with Flow().add(
      uses=yaml_file,
      shards=1
      separated_workspace=True,
  ) as f:
       f.index(input_fn=docs)

#yaml_file
!BinaryPbIndexer
with:
  index_filename: doc.gzip
metas:
  name: docIndexer
  workspace: $JINA_TOPK_DIR

requests:
  on:
    IndexRequest:
      - !KVIndexDriver
        with:
          executor: docIndexer
          traversal_paths: ['r']

codecov · 2021-01-12T00:24:01Z

Codecov Report

Merging #1657 (23c6e32) into master (55ab38c) will decrease coverage by 15.77%.
The diff coverage is 40.00%.

@@             Coverage Diff             @@
##           master    #1657       +/-   ##
===========================================
- Coverage   84.95%   69.18%   -15.78%     
===========================================
  Files         133      132        -1     
  Lines        6948     6831      -117     
===========================================
- Hits         5903     4726     -1177     
- Misses       1045     2105     +1060

Flag	Coverage Δ
cd	`?`
ci	`?`
core	`?`
integration	`?`
jinad	`?`
unit	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
jina/clients/helper.py	`100.00% <ø> (+6.12%)`	⬆️
jina/enums.py	`96.50% <ø> (ø)`
jina/executors/indexers/cache.py	`44.28% <0.00%> (-52.86%)`	⬇️
jina/flow/base.py	`84.53% <ø> (-2.82%)`	⬇️
jina/flow/builder.py	`73.33% <ø> (-0.30%)`	⬇️
jina/helloworld/__init__.py	`100.00% <ø> (ø)`
jina/parsers/flow.py	`100.00% <ø> (ø)`
jina/parsers/helloworld.py	`100.00% <ø> (ø)`
jina/proto/jina_pb2.py	`100.00% <ø> (ø)`
jina/proto/jina_pb2_grpc.py	`78.94% <ø> (+8.57%)`	⬆️
... and 74 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e00ac9...038b486. Read the comment docs.

github-actions · 2021-01-12T00:32:09Z

Latency summary

Current PR yields:

😶 index QPS at 1571, delta to last 3 avg.: -3%
😶 query QPS at 27, delta to last 3 avg.: -1%

Breakdown

Version	Index QPS	Query QPS
current	1571	27
`0.9.12`	1621	27

Backed by latency-tracking. Further commits will update this comment.

jina/peapods/pods/helper.py

JoanFM · 2021-01-12T06:22:17Z

I think this can be expected, pea_id is only used to define separate workspaces, and I think in executors the pea_id is checked against -1

jina/peapods/pods/__init__.py

JoanFM · 2021-01-12T07:25:28Z

jina/peapods/pods/__init__.py

@@ -114,10 +114,6 @@ def _parse_args(self, args: Namespace) -> Dict[str, Optional[Union[List[Namespac
                self.is_tail_router = True
                peas_args['tail'] = _copy_to_tail_args(args)
            peas_args['peas'] = _set_peas_args(args, peas_args.get('head', None), peas_args.get('tail', None))
-        else:
-            self.is_head_router = False


I think this logic needs to remain there, are you sure it needs removal?

thanks, I don't think it is used, but now I keep it there because it is out of scope for this pr

JoanFM · 2021-01-12T07:28:21Z

@florian-hoenicke it would be good to describe what is the expected value of pea_id in this case?

would it be 1 or 0?

JoanFM · 2021-01-12T07:28:34Z

Does this pea_id value has any further impact?

florian-hoenicke · 2021-01-12T11:58:19Z

Does this pea_id value has any further impact?

Yes, the pea_id is used to generate the path the index is stored at. If it is -1, then the path becomes .../docIndexer--1/doc.gzip.

If we have 2 shards, the indexes are stored at .../docIndexer-1 and .../docIndexer-2.
In case of 1 shard it is messed up. It stores at .../docIndexer--1 and searches at .../docIndexer-0.
From my understanding, it should store and search at .../docIndexer-1` instead

JoanFM · 2021-01-12T12:09:34Z

Does this pea_id value has any further impact?

Yes, the pea_id is used to generate the path the index is stored at. If it is -1, then the path becomes .../docIndexer--1/doc.gzip.

If we have 2 shards, the indexes are stored at .../docIndexer-1 and .../docIndexer-2.
In case of 1 shard it is messed up. It stores at .../docIndexer--1 and searches at .../docIndexer-0.
From my understanding, it should store and search at .../docIndexer-1` instead

I think the correct way is to only consider the pea_id to name the file only if separated_workspace is True, and not to change the pea_id

JoanFM

add unittest with to check current_workspace does not include any reference to pea_id?

florian-hoenicke · 2021-01-12T23:39:53Z

add unittest with to check current_workspace does not include any reference to pea_id?

I don't update the pea_id anymore, but change the way the workspace path is created. The test validates that the index file is created in the right folder:

    with BaseExecutor.load((random_workspace / 'doc_idx.bin')) as doc_indexer:
        assert isinstance(doc_indexer, BinaryPbIndexer)
        assert doc_indexer.size == num_uniq_docs

There exists another test validating that the workspace is set up correctly in case we have more than one shard.tests/unit/test_index.py:test_index

fix: sharding parallel one

6b32ebe

jina-bot added size/S area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality component/peapod labels Jan 12, 2021

JoanFM requested changes Jan 12, 2021

View reviewed changes

jina/peapods/pods/helper.py Outdated Show resolved Hide resolved

JoanFM reviewed Jan 12, 2021

View reviewed changes

fix: pea workspace

88a955b

jina-bot added area/testing This issue/PR affects testing component/executor component/jaml executor/meta labels Jan 12, 2021

JoanFM reviewed Jan 12, 2021

View reviewed changes

refactor: format executor init

ba25545

jina-bot added size/XS and removed size/S labels Jan 12, 2021

fix: sharding file name

4d6e591

jina-bot added size/S and removed size/XS labels Jan 12, 2021

refactor: remove unused code

038b486

florian-hoenicke marked this pull request as ready for review January 13, 2021 00:08

florian-hoenicke requested a review from a team as a code owner January 13, 2021 00:08

florian-hoenicke requested review from imsergiy and deepankarm January 13, 2021 00:08

JoanFM approved these changes Jan 13, 2021

View reviewed changes

florian-hoenicke merged commit cd7d4ad into master Jan 13, 2021

florian-hoenicke deleted the fix-sharding-parallel-one branch January 13, 2021 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: sharding parallel one #1657

fix: sharding parallel one #1657

florian-hoenicke commented Jan 12, 2021

codecov bot commented Jan 12, 2021 •

edited

github-actions bot commented Jan 12, 2021 •

edited

JoanFM commented Jan 12, 2021

JoanFM Jan 12, 2021

florian-hoenicke Jan 12, 2021

JoanFM commented Jan 12, 2021

JoanFM commented Jan 12, 2021

florian-hoenicke commented Jan 12, 2021

JoanFM commented Jan 12, 2021 •

edited

JoanFM left a comment

florian-hoenicke commented Jan 12, 2021

fix: sharding parallel one #1657

fix: sharding parallel one #1657

Conversation

florian-hoenicke commented Jan 12, 2021

codecov bot commented Jan 12, 2021 • edited

Codecov Report

github-actions bot commented Jan 12, 2021 • edited

Latency summary

Breakdown

JoanFM commented Jan 12, 2021

JoanFM Jan 12, 2021

Choose a reason for hiding this comment

florian-hoenicke Jan 12, 2021

Choose a reason for hiding this comment

JoanFM commented Jan 12, 2021

JoanFM commented Jan 12, 2021

florian-hoenicke commented Jan 12, 2021

JoanFM commented Jan 12, 2021 • edited

JoanFM left a comment

Choose a reason for hiding this comment

florian-hoenicke commented Jan 12, 2021

codecov bot commented Jan 12, 2021 •

edited

github-actions bot commented Jan 12, 2021 •

edited

JoanFM commented Jan 12, 2021 •

edited