fix: add support independent len calculation#840
Merged
Conversation
Codecov ReportBase: 81.08% // Head: 79.73% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #840 +/- ##
==========================================
- Coverage 81.08% 79.73% -1.35%
==========================================
Files 138 138
Lines 7067 7072 +5
==========================================
- Hits 5730 5639 -91
- Misses 1337 1433 +96
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
3681e03 to
8c0ccb3
Compare
AnneYang720
reviewed
Nov 24, 2022
AnneYang720
reviewed
Nov 24, 2022
Contributor
There was a problem hiding this comment.
You can change the previous test to
@pytest.fixture(scope='function')
def docs():
d1 = Document(id='0', embedding=np.array([10, 0]))
d2 = Document(id='1', embedding=np.array([0, 10]))
d3 = Document(id='2', embedding=np.array([-10, -10]))
yield d1, d2, d3
@pytest.mark.parametrize(
'storage,config',
[
('weaviate', {'n_dim': 2, 'list_like': False}),
('annlite', {'n_dim': 2, 'list_like': False}),
('qdrant', {'n_dim': 2, 'list_like': False}),
('elasticsearch', {'n_dim': 2, 'list_like': False}),
('redis', {'n_dim': 2, 'list_like': False}),
('milvus', {'n_dim': 2, 'list_like': False}),
],
)
def test_disable_offset2id(docs, storage, config, start_storage):
if config:
da = DocumentArray(storage=storage, config=config)
else:
da = DocumentArray(storage=storage)
assert len(da) == 0
da.extend(docs)
assert len(da) == 3
del da['0']
assert len(da) == 2
with pytest.raises(ValueError):
da[0]
AnneYang720
reviewed
Nov 24, 2022
Contributor
There was a problem hiding this comment.
There are three more things:
- Since you have changed the extend function of
Offset2ID, the_extendfunction for storage backends should also be changed, like for Redis,if self._list_like:should be removed. - The
_del_docand_del_docsfunction will callself._offset2ids.delete_by_id(_id)andself._del_doc_by_id(_id).self._del_doc_by_id(_id)won't delete the document in storage if the key doesn't exist, but theself.ids_count -= 1of offset2id is executed anyway. You can notice this issue if you change thedel da['0']todel da['any_id']in the script above. - The test for milvus fails with
AttributeError: 'DocumentArrayMilvus' object has no attribute '_offset2ids'
357b4e6 to
a40e17d
Compare
JoanFM
reviewed
Nov 25, 2022
JoanFM
requested changes
Nov 25, 2022
be3ddf3 to
579154a
Compare
AnneYang720
suggested changes
Nov 28, 2022
00ed22a to
c31e05c
Compare
c31e05c to
418c992
Compare
JoanFM
requested changes
Nov 28, 2022
JoanFM
requested changes
Nov 28, 2022
Signed-off-by: dong xiang <[email protected]>
418c992 to
52809ec
Compare
JoanFM
approved these changes
Nov 28, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: dong xiang [email protected]
Goals:
...
As reported by users, after disabling the list-like flag in Docarray backends, the len should still work, because otherwise the redis backend will fail. Therefore it is necessary to add this functionality.
...
check and update documentation, if required. See guide