fix: put siblings back #2234
fix: put siblings back #2234
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2234 +/- ##
==========================================
- Coverage 90.59% 87.40% -3.19%
==========================================
Files 209 209
Lines 11352 11372 +20
==========================================
- Hits 10284 9940 -344
- Misses 1068 1432 +364
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Latency summaryCurrent PR yields:
Breakdown
Backed by latency-tracking. Further commits will update this comment. |
Co-authored-by: Nan Wang <nan.wang@jina.ai>
76e84da to
1cb3d52
Compare
@Yongxuanzhang thanks, I added the test |
jina/drivers/segment.py
Outdated
| @@ -54,8 +54,9 @@ def _apply_all(self, docs: 'DocumentSet', *args, **kwargs): | |||
|
|
|||
| @staticmethod | |||
| def _add_chunks(doc, chunks): | |||
| num_siblings = len(chunks) | |||
There was a problem hiding this comment.
| num_siblings = len(chunks) | |
| num_siblings = len(chunks) + len(doc.chunks) |
Co-authored-by: Nan Wang <nan.wang@jina.ai>
0d39275 to
3cba88b
Compare
|
Rebased to fix broken test |
| @@ -54,8 +54,9 @@ def _apply_all(self, docs: 'DocumentSet', *args, **kwargs): | |||
|
|
|||
| @staticmethod | |||
There was a problem hiding this comment.
can we add an assertion in the segment driver test to see if the resulting chunks have the right amount of siblings?
There was a problem hiding this comment.
Now I added a test.
| @@ -48,17 +56,41 @@ def append(self, document: 'Document', **kwargs) -> 'Document': | |||
| chunk.update_content_hash() | |||
| return chunk | |||
|
|
|||
| def extend(self, iterable: Iterable['Document']) -> None: | |||
There was a problem hiding this comment.
I added the sibling update functionality to the extend method. It is a bit inconsistent because append does not update the siblings. But if we would update this attribute for append as well it could slow down the system when having many chunks. We could argue that the system is slowed down anyways if we have a lot of chunks and have the doc.siblings = len(self) also in the append.
Anyways, I think there is the goal to remove siblings in the long term. So
A3E2
thinking about all the cases might not be necessary
@cristianmtr I've added an integration test |
Removing the length attribute broke the BiMatchRanker.
This pr restores the length attribute and renames it to siblings.