Conversation
Codecov Report
@@ Coverage Diff @@
## main #622 +/- ##
==========================================
+ Coverage 86.48% 86.64% +0.15%
==========================================
Files 133 133
Lines 6771 6528 -243
==========================================
- Hits 5856 5656 -200
+ Misses 915 872 -43
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. |
JoanFM
left a comment
There was a problem hiding this comment.
Do not merge this. This should be fixed by Annlite or closed when ctxt manager leaves
| return self._annlite.index_size | ||
|
|
||
| def close(self): | ||
| self._annlite.close() |
There was a problem hiding this comment.
I do not think there is a close interface in DocArray
There was a problem hiding this comment.
Yes, this is only for annlite backend. We need this because the file lock needed to be closed before access it again.
|
|
||
| assert len(da) == 0 | ||
| if da_cls == DocumentArrayAnnlite: | ||
| da.close() |
There was a problem hiding this comment.
remove this, this is not what we are testing, we cannot test custom behavior like this. WE AIM FOR SAME interface
There was a problem hiding this comment.
Do you think we need to refactor the annlite-based test? We need to manually close it before another test starts. Otherwise, the file lock will block us.
There was a problem hiding this comment.
I do not like imposing the close API, this needs to be fix on ANNLite backend IMO
tests/unit/array/mixins/test_io.py
Outdated
| [d.embedding for d in da], | ||
| [d.content for d in da], | ||
| ] | ||
| da.close() |
|
Maybe instead of having a with da:
...context manager |
This is a reasonable approach |
bf2c582 to
8ca4027
Compare
There was a problem hiding this comment.
Please reenable the test that was disabled by @alaeddine-13 PR #649
Done |
| da = cls() | ||
| assert getattr(da, content_attr) is None | ||
|
|
||
| if cls == DocumentArrayAnnlite: |
There was a problem hiding this comment.
for example here, DA will create an annlite instance with temp file name. I don't think anything would access the same temp file concurrently, why do we need to release the lock ?
There was a problem hiding this comment.
This is because we need to test with different parameters, it's like a for loop, need to close it before another iteration.
|
I do not understand this PR. Now u are saying 0.4.0 is already released and all these tests were not failing even without |
|
No, we haven't released annlite 0.4.0 yet. But when we release it, the docarray test will fail without these changes. So this is what this pr is doing: make sure the ci won't fail after 0.4.0 is released. |
|
@jemmyshin I prefer then to have a PR fixing |
Goals:
Recently we are going to release
Annlitev0.4.0. We replacelmdbwithrocksdbto reduce memory footprint.More details are here, green lines in these charts (rocksdb and meta table on disk) are our implementation now.
This pr fixes the errors in
Annlite-related unit tests introduced by file lock inrocksdb.We will make a formal pr once we confirm that
Annlitewon't break thedocarraytest.Annlitein unit test in order to solve the file lock issue.