fix: del with ids by samsja · Pull Request #79 · docarray/docarray

samsja · 2022-01-26T10:56:20Z

Current version of docarray has a wrong behavior while deleting item via ids ==>

@JohannesMessner and I added some test to cover all of the del case from the documentation + a fix when deleting by ids

codecov · 2022-01-26T11:15:39Z

Codecov Report

Merging #79 (3052494) into main (95d9f5a) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
+ Coverage   82.85%   82.98%   +0.13%     
==========================================
  Files          87       87              
  Lines        3815     3827      +12     
==========================================
+ Hits         3161     3176      +15     
+ Misses        654      651       -3

Flag	Coverage Δ
docarray	`82.98% <100.00%> (+0.13%)`	⬆️

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

Impacted Files	Coverage Δ
docarray/array/storage/memory/backend.py	`97.61% <100.00%> (+0.74%)`	⬆️
docarray/array/storage/memory/getsetdel.py	`97.87% <100.00%> (-0.05%)`	⬇️
docarray/array/storage/memory/seqlike.py	`100.00% <100.00%> (+3.22%)`	⬆️
docarray/array/mixins/getitem.py	`87.93% <0.00%> (+1.72%)`	⬆️
docarray/array/mixins/delitem.py	`86.66% <0.00%> (+2.22%)`	⬆️

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 95d9f5a...3052494. Read the comment docs.

docarray/array/storage/memory/getsetdel.py

numb3r3 · 2022-01-27T04:56:14Z

docarray/array/storage/memory/getsetdel.py

    def _del_docs_by_slice(self, _slice: slice):
        del self._data[_slice]
-        self._rebuild_id2offset()
+        self._needs_id2offset_rebuild = True


change the _needs_id2offset_rebuild to a decorator?

I changed it in the last commit, could you review ?

hanxiao · 2022-01-27T07:36:10Z

docarray/array/storage/memory/backend.py

+
    def _init_storage(
-        self, _docs: Optional['DocumentArraySourceType'] = None, copy: bool = False
+        self, _docs: Optional["DocumentArraySourceType"] = None, copy: bool = False


single quote

hanxiao · 2022-01-27T07:40:22Z

tests/unit/array/mixins/test_del.py

+def docs():
+    return DocumentArray([Document(id=f"{i}") for i in range(1, 10)])
+
+
+@pytest.mark.parametrize(
+    "to_delete",
+    [
+        0,
+        1,
+        4,
+        -1,
+        list(range(1, 4)),
+        [2, 4, 7, 1, 1],
+        slice(0, 2),
+        slice(2, 4),
+        slice(4, -1),
+        [True, True, False],
+        ...,
+    ],
+)
+def test_del_all(docs, to_delete):
+    doc_to_delete = docs[to_delete]
+    del docs[to_delete]
+    assert doc_to_delete not in docs
+
+
+@pytest.mark.parametrize(
+    ["deleted_ids", "expected_ids"],
+    [
+        (["1", "2", "3", "4"], ["5", "6", "7", "8", "9"]),
+        (["2", "4", "7", "1"], ["3", "5", "6", "8", "9"]),
+    ],
+)


use black -s

or a better way is to install pre-commit hook via

# at docarray root pre-commit install

you will see this option auto-checked on every commit

I already installed the pre-commit hook, this is already formatted by black

alaeddine-13

In general, I think that the operations that require rebuilding the id2offset are the delete and insert. Extend and append really don't require doing so. If we append/extend and the id2offset, is ready, then we update it efficiently without rebuilding, otherwise we don't update and wait for the next rebuilding

docarray/array/storage/memory/getsetdel.py

docarray/array/storage/memory/seqlike.py

docarray/array/storage/memory/backend.py

docarray/array/storage/memory/seqlike.py

Co-authored-by: AlaeddineAbdessalem <[email protected]>

alaeddine-13

LGTM

hanxiao · 2022-01-27T16:23:08Z

docarray/array/storage/memory/backend.py

            elif isinstance(_docs, DocumentArray):
                self._data = _docs._data
                self._id_to_index = _docs._id2offset


i think this part is wrong, not all DocumentArray has _id_to_index or _id2offset

I agree, that was here before so I might be an other bug ? we should delete line 77 as the index _id_to_index will be automatically built on first call.

fix in last commit

hanxiao · 2022-01-27T16:23:30Z

docarray/array/storage/memory/backend.py

            if copy:
                self._data = [Document(d, copy=True) for d in _docs]
-                self._rebuild_id2offset()
            elif isinstance(_docs, DocumentArray):


Suggested change

elif isinstance(_docs, DocumentArray):

elif isinstance(_docs, DocumentArrayinMemory):

fix in last commit

hanxiao

wrong logic in copy constructor

docarray/array/storage/memory/seqlike.py

changes applied already

github-actions bot added size/s area/core area/testing component/array labels Jan 26, 2022

JohannesMessner mentioned this pull request Jan 26, 2022

refactor: migration to Jina 3 jina-ai/executor-simpleindexer#39

Merged

samsja force-pushed the fix-del-with-ids branch 4 times, most recently from 9eb4b38 to 12f962f Compare January 26, 2022 11:10

hanxiao reviewed Jan 26, 2022

View reviewed changes

docarray/array/storage/memory/getsetdel.py Show resolved Hide resolved

samsja force-pushed the fix-del-with-ids branch 2 times, most recently from d81bb73 to 5e09e7e Compare January 26, 2022 14:20

samsja marked this pull request as ready for review January 26, 2022 16:06

samsja requested review from alaeddine-13, davidbp and hanxiao January 26, 2022 16:46

numb3r3 reviewed Jan 27, 2022

View reviewed changes

hanxiao reviewed Jan 27, 2022

View reviewed changes

samsja force-pushed the fix-del-with-ids branch from 18af361 to 46a3633 Compare January 27, 2022 08:54

Sami Jaghouar added 8 commits January 27, 2022 10:42

test(del): add test to cover deleting doc

ee3338a

fix(del): deleting by ids in DA no longer bugs

34807b4

test(seq): add tests for acces by id after insert

6aab5fc

fix(seq): rebuild index after insert

2728244

fix(del): del by id call del by offset

6bd5542

fix(del): improve efficiency by rebuilding id2offset lazily when needed

40242f2

fix(del): double quotes ==> simple quote

fd744f4

feat(getdelset): use a decorator for needs id2offset rebuild

6621bc4

samsja force-pushed the fix-del-with-ids branch from 46a3633 to 6621bc4 Compare January 27, 2022 09:42

alaeddine-13 reviewed Jan 27, 2022

View reviewed changes

docarray/array/storage/memory/getsetdel.py Outdated Show resolved Hide resolved

docarray/array/storage/memory/seqlike.py Outdated Show resolved Hide resolved

fix(extend): extend is faster no rebuild id2offset index

b32ac41

samsja requested a review from alaeddine-13 January 27, 2022 10:31

fix(set): no id2offset rebuild for set_do_value_pairs

485d99d

alaeddine-13 reviewed Jan 27, 2022

View reviewed changes

docarray/array/storage/memory/backend.py Show resolved Hide resolved

alaeddine-13 reviewed Jan 27, 2022

View reviewed changes

docarray/array/storage/memory/seqlike.py Outdated Show resolved Hide resolved

fix(seqlike): insert use id2offset now

94b2388

Co-authored-by: AlaeddineAbdessalem <[email protected]>

samsja force-pushed the fix-del-with-ids branch from bd98fbe to 4426b61 Compare January 27, 2022 13:28

fix(newdoc): avoid rebuilding index when copying da

ddfa8a9

Co-authored-by: AlaeddineAbdessalem <[email protected]>

samsja force-pushed the fix-del-with-ids branch from 4426b61 to ddfa8a9 Compare January 27, 2022 13:36

samsja changed the title ~~Fix del with ids~~ fix: del with ids Jan 27, 2022

samsja requested a review from alaeddine-13 January 27, 2022 14:28

fix(extend): values iterable is only consume once in extend

697cba0

samsja force-pushed the fix-del-with-ids branch from 7620636 to 697cba0 Compare January 27, 2022 15:28

alaeddine-13 approved these changes Jan 27, 2022

View reviewed changes

hanxiao reviewed Jan 27, 2022

View reviewed changes

hanxiao previously requested changes Jan 27, 2022

View reviewed changes

hanxiao reviewed Jan 27, 2022

View reviewed changes

docarray/array/storage/memory/seqlike.py Outdated Show resolved Hide resolved

fix(initstorage): only daInMemory has an _id2offset

b40ef88

github-actions bot added size/m and removed size/s labels Jan 27, 2022

perf(extend): cast iterator to a list to use extend

3052494

github-actions bot added size/s and removed size/m labels Jan 27, 2022

samsja requested review from alaeddine-13 and hanxiao January 27, 2022 17:27

hanxiao merged commit 2d5e93b into main Jan 28, 2022

hanxiao deleted the fix-del-with-ids branch January 28, 2022 09:43

	elif isinstance(_docs, DocumentArray):
	elif isinstance(_docs, DocumentArrayinMemory):

Conversation

samsja commented Jan 26, 2022

Uh oh!

codecov bot commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alaeddine-13 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alaeddine-13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanxiao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jan 26, 2022 •

edited

Loading