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.

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

Add experimental Dask-on-Mars support #2289

Merged
merged 15 commits into from
Aug 17, 2021
Merged

Add experimental Dask-on-Mars support #2289

merged 15 commits into from
Aug 17, 2021

Conversation

loopyme
Copy link
Contributor

@loopyme loopyme commented Aug 4, 2021

What do these changes do?

Add experimental Dask-on-Mars support, which make it possible to run Dask collections on Mars.

@qinxuye
Copy link
Collaborator

qinxuye commented Aug 5, 2021

We don't have the habit to use sort of experimental stuff, cuz users anyway need to change their code. I recommend to rename experimental to contrib, like contrib/dask.

@qinxuye
Copy link
Collaborator

qinxuye commented Aug 5, 2021

What's more, you can create a separate CI test in platform-ci, refer to https://github.com/mars-project/mars/blob/master/.github/workflows/platform-ci.yml to add a new ci in which dask installed.

@qinxuye qinxuye added this to the v0.8.0a2 milestone Aug 5, 2021
@loopyme
Copy link
Contributor Author

loopyme commented Aug 7, 2021

I am not familiar with CI tests, will this works?

@qinxuye
Copy link
Collaborator

qinxuye commented Aug 9, 2021

Seems dask test timeout. Could you please address the issue please?

@loopyme
Copy link
Contributor Author

loopyme commented Aug 9, 2021

Working on that

@qinxuye qinxuye marked this pull request as ready for review August 12, 2021 05:58
@qinxuye qinxuye marked this pull request as draft August 12, 2021 05:58
@qinxuye
Copy link
Collaborator

qinxuye commented Aug 12, 2021

Is this PR ready for a review? maybe we could mark it as some milestone now?

@loopyme loopyme marked this pull request as ready for review August 12, 2021 08:16
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, coverage could be increased a bit.

mars/contrib/dask/converter.py Outdated Show resolved Hide resolved
mars/contrib/dask/tests/test_dask.py Outdated Show resolved Hide resolved
@hekaisheng
Copy link
Contributor

Will convert_dask_collection method be exposed to users? It seems that the recommended way to execute Dask collections is df.compute(scheduler=mars_scheduler), could you add docs in user guide?

@loopyme
Copy link
Contributor Author

loopyme commented Aug 13, 2021

Will convert_dask_collection method be exposed to users? It seems that the recommended way to execute Dask collections is df.compute(scheduler=mars_scheduler), could you add docs in user guide?

Exposing convert_dask_collection method to user sounds better to me, since it may be more robust than mars_scheduler. Besides, it allows users to modifying the task graph based on the original Dask collections using Remote API or other Mars features. If user only need to run Dask collections, mars_scheduler will be better choice.

I am working on doc

@qinxuye
Copy link
Collaborator

qinxuye commented Aug 16, 2021

@loopyme
Copy link
Contributor Author

loopyme commented Aug 16, 2021

Sorry about the typo

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

Copy link
Member

@wjsi wjsi left a comment

Choose a reason for hiding this comment

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

LGTM

@wjsi wjsi merged commit 131c23a into mars-project:master Aug 17, 2021
@qinxuye
Copy link
Collaborator

qinxuye commented Aug 17, 2021

Congrats, look forward to seeing more contributions from you.

@loopyme loopyme deleted the dask_on_mars branch August 17, 2021 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants