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

[Ray] Fix ray executor progress test #3033

Merged
merged 2 commits into from
May 16, 2022

Conversation

zhongchun
Copy link
Contributor

@zhongchun zhongchun commented May 13, 2022

What do these changes do?

Ray executor progress test does not run due to lack of await. This pr fixed it.

Related issue number

Fixes #xxxx

Check code requirements

  • tests added / passed (if needed)
  • Ensure all linting tests pass, see here for how to run them

@zhongchun zhongchun requested a review from a team as a code owner May 13, 2022 09:19

t1 = t.sum()
r = t1.map_chunk(f1)
info = await session.execute(r)
Copy link
Contributor

Choose a reason for hiding this comment

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

How long does this test case take? I noticed that there is a time.sleep(2) in the f1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It takes about 5s on mars executor, 10s on ray executor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make it faster? Ray backend is slower because:

Copy link
Contributor Author

@zhongchun zhongchun May 13, 2022

Choose a reason for hiding this comment

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

Thanks for you reminder. I've fixed it. Now its cost is about 6s on ray.

@qinxuye qinxuye added type: bug Something isn't working to be backported Indicate that the PR need to be backported to stable branch mod: ray integration labels May 13, 2022
@qinxuye qinxuye added this to PR-In progress in v0.10 Release via automation May 13, 2022
@qinxuye qinxuye added this to the v0.10.0a1 milestone May 13, 2022
@qinxuye qinxuye added type: tests and removed type: bug Something isn't working labels May 13, 2022
@fyrestone fyrestone added this to In Progress in Ray DAG Execution Backend via automation May 13, 2022
@fyrestone fyrestone moved this from In Progress to Done in Ray DAG Execution Backend May 13, 2022
@fyrestone fyrestone moved this from Done to In Progress in Ray DAG Execution Backend May 13, 2022
@qinxuye
Copy link
Collaborator

qinxuye commented May 16, 2022

@fyrestone Could you please review this PR to see if there is any further issue?

Copy link
Contributor

@fyrestone fyrestone 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
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 de933b9 into mars-project:master May 16, 2022
v0.10 Release automation moved this from PR-In progress to PR-Done May 16, 2022
Ray DAG Execution Backend automation moved this from In Progress to Done May 16, 2022
wjsi pushed a commit to wjsi/mars that referenced this pull request May 23, 2022
@wjsi wjsi added backported already PR has been backported and removed to be backported Indicate that the PR need to be backported to stable branch labels May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants