Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[storage][vineyard] Implement storage lib of vineyard backend #1952

Merged
merged 17 commits into from
Feb 4, 2021
Merged

[storage][vineyard] Implement storage lib of vineyard backend #1952

merged 17 commits into from
Feb 4, 2021

Conversation

acezen
Copy link
Contributor

@acezen acezen commented Jan 30, 2021 •
edited
Loading

What do these changes do?

implement vineyard backend storage lib.

  • vineyard backend
  • unit test case

this pr should be merged after #1904

Related issue number

#1905

@qinxuye qinxuye added this to the v0.7.0a6 milestone Jan 30, 2021
@acezen acezen changed the title Implement storage lib of vineyard banckend Implement storage lib of vineyard backend Jan 31, 2021
@hekaisheng
Copy link
Contributor

#1904 has been merged, there's some small changes in storage lib API.

@acezen acezen force-pushed the feature/vineyard-storage-lib branch from 947b1f1 to 5cb3267 Compare February 2, 2021 08:03
qinxuye
qinxuye reviewed Feb 3, 2021
View reviewed changes
Copy link
Collaborator

@qinxuye qinxuye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, I left some comments.

mars/storage/core.py Outdated Show resolved Hide resolved
mars/storage/tests/test_libs.py Outdated Show resolved Hide resolved
mars/storage/tests/test_libs.py Show resolved Hide resolved
mars/storage/vineyard.py Show resolved Hide resolved
@acezen acezen force-pushed the feature/vineyard-storage-lib branch 2 times, most recently from af59269 to 4b37f28 Compare February 3, 2021 06:19
@acezen acezen force-pushed the feature/vineyard-storage-lib branch 2 times, most recently from 549f26f to 395271b Compare February 3, 2021 06:34
@acezen acezen force-pushed the feature/vineyard-storage-lib branch from 395271b to a4dcdb2 Compare February 3, 2021 06:42
qinxuye
qinxuye reviewed Feb 3, 2021
View reviewed changes
Copy link
Collaborator

@qinxuye qinxuye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering why etcd is required for core testing?

.github/workflows/core-ci.yml Outdated Show resolved Hide resolved
@qinxuye
Copy link
Collaborator

qinxuye commented Feb 3, 2021

I think vineyard storage can be tested in vineyard platform testing only.

@acezen acezen marked this pull request as ready for review February 3, 2021 09:32
@acezen acezen requested a review from hekaisheng as a code owner February 3, 2021 09:32
@sighingnow sighingnow self-requested a review February 3, 2021 09:34
sighingnow
sighingnow previously approved these changes Feb 3, 2021
View reviewed changes
Copy link
Contributor

@sighingnow sighingnow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. Just one concern is should be impose another storage (maybe VINEYARD) and use it for vineyard?

Or MEMORY is fine enough here?

I would like to know comments from @qinxuye.

@acezen acezen force-pushed the feature/vineyard-storage-lib branch from 2e0adf8 to b618e19 Compare February 3, 2021 09:41
sighingnow
sighingnow reviewed Feb 3, 2021
View reviewed changes
Copy link
Contributor

@sighingnow sighingnow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add vineyard (maybe a fixed version) to the requirements.txt of mars?

@qinxuye
Copy link
Collaborator

qinxuye commented Feb 3, 2021

Overall LGTM. Just one concern is should be impose another storage (maybe VINEYARD) and use it for vineyard?

Or MEMORY is fine enough here?

I would like to know comments from @qinxuye.

You mean storage level? I think MEMORY is enough.

@qinxuye
Copy link
Collaborator

qinxuye commented Feb 3, 2021

Could we add vineyard (maybe a fixed version) to the requirements.txt of mars?

As for requirements, it's about necessary libraries, how about add a pip install mars[vineyard] support?

@sighingnow
Copy link
Contributor

You mean storage level? I think MEMORY is enough.

Then this pull request looks to me.

As for requirements, it's about necessary libraries, how about add a pip install mars[vineyard] support?

Add a vvineyard profile is also ok. I'm wondering if we could put vineyard at the same dependency level with plasma.

@qinxuye
Copy link
Collaborator

qinxuye commented Feb 3, 2021

You mean storage level? I think MEMORY is enough.

Then this pull request looks to me.

As for requirements, it's about necessary libraries, how about add a pip install mars[vineyard] support?

Add a vvineyard profile is also ok. I'm wondering if we could put vineyard at the same dependency level with plasma.

Yeah, but for now, Mars does not rely on plasma either, but in the near future, multiprocessing will be the default option, till then, we can see if vineyard could be the default library.

@qinxuye
Copy link
Collaborator

qinxuye commented Feb 3, 2021

@wjsi Could you please help to check why vineyard storage is not covered?

@sighingnow
Copy link
Contributor

@wjsi Could you please help to check why vineyard storage is not covered?

I'm not very sure, I think it is becase there are three pytest call and the result for the first one is overrided by the later run. Maybe we need to combine like https://github.com/mars-project/mars/blob/master/.github/workflows/run-tests.sh#L16

@qinxuye
Copy link
Collaborator

qinxuye commented Feb 4, 2021

@wjsi Could you please help to check why vineyard storage is not covered?

I'm not very sure, I think it is becase there are three pytest call and the result for the first one is overrided by the later run. Maybe we need to combine like https://github.com/mars-project/mars/blob/master/.github/workflows/run-tests.sh#L16

Perhaps, is it possible to combine them into one? @acezen

@qinxuye qinxuye changed the title Implement storage lib of vineyard backend [storage][vineyard] Implement storage lib of vineyard backend Feb 4, 2021
@acezen
Copy link
Contributor Author

acezen commented Feb 4, 2021

@wjsi Could you please help to check why vineyard storage is not covered?

I'm not very sure, I think it is becase there are three pytest call and the result for the first one is overrided by the later run. Maybe we need to combine like https://github.com/mars-project/mars/blob/master/.github/workflows/run-tests.sh#L16

Perhaps, is it possible to combine them into one? @acezen

ok

@qinxuye
Copy link
Collaborator

qinxuye commented Feb 4, 2021

Hi, @acezen, @wjsi is on it about combing coverage

qinxuye
qinxuye approved these changes Feb 4, 2021
View reviewed changes
Copy link
Collaborator

@qinxuye qinxuye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@qinxuye qinxuye merged commit 3803923 into mars-project:master Feb 4, 2021
sighingnow added a commit to acezen/mars that referenced this pull request Feb 4, 2021
@sighingnow
Fixes the coverage for pr mars-project#1952.
35100ae 
Signed-off-by: Tao He <linzhu.ht@alibaba-inc.com>
@sighingnow sighingnow mentioned this pull request Feb 4, 2021
Merged
qinxuye pushed a commit to qinxuye/mars that referenced this pull request Feb 9, 2021
@acezen
[storage][vineyard] Implement storage lib of vineyard backend (mars-p…
755c697 
…roject#1952)
qinxuye pushed a commit to qinxuye/mars that referenced this pull request Feb 17, 2021
@acezen
[storage][vineyard] Implement storage lib of vineyard backend (mars-p…
2a2d5d9 
…roject#1952)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sighingnow sighingnow sighingnow left review comments

@qinxuye qinxuye qinxuye approved these changes

@hekaisheng hekaisheng Awaiting requested review from hekaisheng

@wjsi wjsi Awaiting requested review from wjsi

Assignees
No one assigned
Labels
mod: storage mod: vineyard integration type: feature New feature
Projects
None yet
Milestone
v0.7.0a6
Development

Successfully merging this pull request may close these issues.
5 participants
@acezen @hekaisheng @qinxuye @sighingnow @wjsi