feat: redis supports multiple DocumentArrays by AnneYang720 · Pull Request #540 · docarray/docarray

AnneYang720 · 2022-09-15T05:57:22Z

Goals:

This PR will add support for storing multiple DocumentArrays in the one Redis server instance according to #507

add code for redis multiple DocumentArrays
modify related tests
update documentation

codecov · 2022-09-15T06:02:58Z

Codecov Report

Merging #540 (37198cb) into main (b2ca490) will decrease coverage by 2.37%.
The diff coverage is 100.00%.

❗ Current head 37198cb differs from pull request most recent head 9de62bd. Consider uploading reports for the commit 9de62bd to get more accurate results

@@            Coverage Diff             @@
##             main     #540      +/-   ##
==========================================
- Coverage   86.41%   84.04%   -2.38%     
==========================================
  Files         142      142              
  Lines        7141     7139       -2     
==========================================
- Hits         6171     6000     -171     
- Misses        970     1139     +169

Flag	Coverage Δ
docarray	`84.04% <100.00%> (-2.38%)`	⬇️

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

Impacted Files	Coverage Δ
docarray/array/storage/redis/backend.py	`92.30% <100.00%> (-4.31%)`	⬇️
docarray/array/storage/redis/getsetdel.py	`95.45% <100.00%> (-1.57%)`	⬇️
docarray/array/mixins/io/pushpull.py	`32.77% <0.00%> (-59.67%)`	⬇️
docarray/array/mixins/io/dataframe.py	`40.00% <0.00%> (-46.67%)`	⬇️
docarray/array/mixins/io/common.py	`28.57% <0.00%> (-38.10%)`	⬇️
docarray/array/mixins/io/json.py	`67.64% <0.00%> (-23.53%)`	⬇️
docarray/array/mixins/content.py	`83.58% <0.00%> (-13.44%)`	⬇️
docarray/array/storage/annlite/backend.py	`82.66% <0.00%> (-13.34%)`	⬇️
docarray/array/storage/sqlite/backend.py	`84.81% <0.00%> (-8.87%)`	⬇️
docarray/document/generators.py	`78.63% <0.00%> (-7.70%)`	⬇️
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

JoanFM · 2022-09-15T08:13:09Z

docarray/array/storage/redis/backend.py

    host: str = field(default='localhost')
    port: int = field(default=6379)
-    index_name: str = field(default='idx')
-    flush: bool = field(default=False)


make sure this is not a breaking change

bwanglzu

havn't start review, but please take a look of the question

bwanglzu · 2022-09-15T09:38:20Z

docarray/array/storage/redis/backend.py


+        if config.index_name is None:
+            id = uuid.uuid4().hex
+            config.index_name = 'index_name__' + id


can we create a CONSTENT rather than hard code a string?

docarray/array/storage/redis/backend.py

docs/advanced/document-store/redis.md

docarray/array/storage/redis/backend.py

bwanglzu · 2022-09-16T09:55:30Z

docarray/array/storage/redis/backend.py

+            id = uuid.uuid4().hex
+            config.index_name = 'index_name__' + id
+
        self._offset2id_key = config.index_name + '__offset2id'


can we create some constant values?

docarray/array/storage/redis/getsetdel.py

docs/advanced/document-store/redis.md

bwanglzu

great work! left some comments in the PR

JoanFM

I agree with @bwanglzu suggestions

docarray/array/storage/redis/backend.py

docs/advanced/document-store/redis.md

docarray/array/storage/redis/backend.py

docarray/array/storage/redis/getsetdel.py

bwanglzu

LGTM!

docarray/array/storage/redis/backend.py

tests/unit/array/mixins/test_content.py

… into feat-redis-multidas

bwanglzu · 2022-09-20T11:07:23Z

@JoanFM @alaeddine-13 please review again

JoanFM · 2022-09-20T11:14:26Z

@alaeddine-13 why u think tests are failing?

docarray/array/storage/redis/getsetdel.py

bwanglzu · 2022-09-20T15:19:37Z

@JoanFM @alaeddine-13 can you review again?

JoanFM · 2022-09-20T15:33:41Z

docarray/array/storage/redis/getsetdel.py

-            self._client.delete(*keys)
-            if cursor == 0:
-                break
+        pipe = self._client.pipeline()


is this pipeline object not a context manager?

seems no as they didn't document

alaeddine-13 · 2022-09-21T06:41:31Z

@alaeddine-13 why u think tests are failing?

I'm inspecting push/pull failure but still no idea about failure in prepare environment

alaeddine-13

LGTM, let's try to pass CI

JoanFM · 2022-09-21T15:57:06Z

Hey @AnneYang720 ,

can u try rebasing your code against master?

I am not sure why the tests fail. But another PR has them passing #548

alaeddine-13 · 2022-09-21T16:23:58Z

Hey @AnneYang720 ,

can u try rebasing your code against master?

I am not sure why the tests fail. But another PR has them passing #548

actually this PR is already up-to-date

alaeddine-13 · 2022-09-21T22:33:44Z

Hey @AnneYang720 ,

can u try rebasing your code against master?

I am not sure why the tests fail. But another PR has them passing #548

Okay now I get why it was not passing. The github secret is not set on external contributor CI.
tests passed on this PR.
I mistakenly force pushed to this branch so waiting for @AnneYang720 to push back her work so we can merge the PR

feat: redis supports storing multiple DAs

b2ccb52

docs: modify redis doc for mutiple DAs

f40ef7c

JoanFM requested changes Sep 15, 2022

View reviewed changes

bwanglzu suggested changes Sep 15, 2022

View reviewed changes

JohannesMessner reviewed Sep 15, 2022

View reviewed changes

docs/advanced/document-store/redis.md Outdated Show resolved Hide resolved

bwanglzu reviewed Sep 16, 2022

View reviewed changes

docs/advanced/document-store/redis.md Show resolved Hide resolved

bwanglzu reviewed Sep 16, 2022

View reviewed changes

docs/advanced/document-store/redis.md Outdated Show resolved Hide resolved

bwanglzu reviewed Sep 16, 2022

View reviewed changes

docarray/array/storage/redis/backend.py Outdated Show resolved Hide resolved

bwanglzu reviewed Sep 16, 2022

View reviewed changes

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

bwanglzu reviewed Sep 16, 2022

View reviewed changes

docs/advanced/document-store/redis.md Outdated Show resolved Hide resolved

bwanglzu reviewed Sep 16, 2022

View reviewed changes

JoanFM requested changes Sep 16, 2022

View reviewed changes

docarray/array/storage/redis/backend.py Outdated Show resolved Hide resolved

docs/advanced/document-store/redis.md Outdated Show resolved Hide resolved

Merge branch 'main' into feat-redis-multidas

344c273

alaeddine-13 reviewed Sep 16, 2022

View reviewed changes

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

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

AnneYang720 added 2 commits September 17, 2022 00:56

refactor: use packaged function

45c95d5

docs: clarify expression

3d04d50

bwanglzu approved these changes Sep 19, 2022

View reviewed changes

AnneYang720 and others added 2 commits September 19, 2022 16:16

perf: batch redis _clear_storage

606297e

Merge branch 'main' into feat-redis-multidas

17d0f43

alaeddine-13 reviewed Sep 19, 2022

View reviewed changes

docarray/array/storage/redis/backend.py Outdated Show resolved Hide resolved

tests/unit/array/mixins/test_content.py Outdated Show resolved Hide resolved

AnneYang720 added 2 commits September 20, 2022 08:14

fix: minor error fix

0cc4902

Merge branch 'feat-redis-multidas' of github.com:AnneYang720/docarray…

173b00e

… into feat-redis-multidas

JoanFM requested changes Sep 20, 2022

View reviewed changes

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

AnneYang720 added 3 commits September 20, 2022 22:57

refactor: use scan_iter in redis clear

88301c5

fix: remove useless imports

695795e

fix: redis delete empty batch

be798d3

JoanFM approved these changes Sep 20, 2022

View reviewed changes

refactor: use drop_index in redis clear

37198cb

alaeddine-13 approved these changes Sep 21, 2022

View reviewed changes

alaeddine-13 closed this Sep 21, 2022

alaeddine-13 force-pushed the feat-redis-multidas branch from 9de62bd to b2ca490 Compare September 21, 2022 22:13

JoanFM reopened this Sep 22, 2022

JoanFM merged commit 4764cc7 into docarray:main Sep 22, 2022

hanxiao mentioned this pull request Sep 22, 2022

chore: release note draft v0.16.0...HEAD #553

Closed

JoanFM mentioned this pull request Oct 3, 2022

Support multiple DocumentArrays in the same redis database #507

Closed

Conversation

AnneYang720 commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

JoanFM Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

bwanglzu left a comment

Choose a reason for hiding this comment

Uh oh!

bwanglzu Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwanglzu Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bwanglzu left a comment

Choose a reason for hiding this comment

Uh oh!

JoanFM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwanglzu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bwanglzu commented Sep 20, 2022

Uh oh!

JoanFM commented Sep 20, 2022

Uh oh!

Uh oh!

bwanglzu commented Sep 20, 2022

Uh oh!

JoanFM Sep 20, 2022

Choose a reason for hiding this comment

Uh oh!

bwanglzu Sep 20, 2022

Choose a reason for hiding this comment

Uh oh!

alaeddine-13 commented Sep 21, 2022

Uh oh!

alaeddine-13 left a comment

Choose a reason for hiding this comment

Uh oh!

JoanFM commented Sep 21, 2022

Uh oh!

alaeddine-13 commented Sep 21, 2022

Uh oh!

alaeddine-13 commented Sep 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AnneYang720 commented Sep 15, 2022 •

edited

Loading

codecov bot commented Sep 15, 2022 •

edited

Loading