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

feat: Ray integration #621

Merged
merged 30 commits into from
Apr 6, 2023
Merged

feat: Ray integration #621

merged 30 commits into from
Apr 6, 2023

Conversation

jiashenC
Copy link
Member

@jiashenC jiashenC commented Mar 23, 2023

  • Auto-detect availability of GPUs.
  • Enable CircleCI auto-testing for Ray execution.
    • Only test CPU Ray on CircleCI.
    • Disable coverage report where it causes long hang for multi-process execution.
  • Address integration test failures for Ray. Here are a list of causes that I address in this PR.
    • Bug in Ray optimizer that does not correctly yield, which often results infinite loop in optimization step.
    • A tree-like execution plan was not previously supported for Ray, mainly because the queue population does not support that yet. For a quick merge now, I do a post-processing to disable Exchange plan for all execution plans that have branch.

Copy link
Member

@gaurav274 gaurav274 left a comment

Choose a reason for hiding this comment

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

Can you enable circleci to run with ray? It is fine if it does not pass. We can do for python 3.10 version only.

@jiashenC
Copy link
Member Author

Can you enable circleci to run with ray? It is fine if it does not pass. We can do for python 3.10 version only.

@gaurav274 I enable the circleci. It does not have GPU resource, so we need to change that as well.

@xzdandy
Copy link
Collaborator

xzdandy commented Mar 24, 2023

I think lateral join becomes linear now. So ray should be able to support function scan without worrying about tree-like execution plan.

@jarulraj jarulraj changed the title Fix ray feat: Ray integration Mar 26, 2023
@jiashenC jiashenC force-pushed the fix-ray branch 2 times, most recently from 4e08a14 to 9aa9fac Compare April 4, 2023 01:11
plan.append_child(child)

if isinstance(plan, ExchangePlan):
if is_top:
Copy link
Member

Choose a reason for hiding this comment

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

I think this code piece can be simplified. How about you assert on is_top and len(plan.children) == 1 and always return plan.children. This is also simplify the above check on List.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a temporary work around? I feel We should be able to support branch and createMat. But I agree it is not priority.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This is temporary workaround as a quick way to support Ray. I agree we can definitely extend to more use cases.

else:
upper = ExchangePlan(
parallelism=2,
ray_conf={"num_gpus": 1}
Copy link
Member

Choose a reason for hiding this comment

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

@kaushikravichandran This can be moved outside to a heuristic in a future PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Noted

Copy link
Member

@gaurav274 gaurav274 left a comment

Choose a reason for hiding this comment

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

LGTM. We can wait for Andy's review before merging.

@gaurav274
Copy link
Member

Please add a test case where the exchange removal code is triggered. Thanks!

@gaurav274
Copy link
Member

@kaushikravichandran Let us wait for a few builds and then promote ray from experimental to default feature. Thanks!

Copy link
Collaborator

@xzdandy xzdandy 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. Please check minor comments.

@jiashenC
Copy link
Member Author

jiashenC commented Apr 5, 2023

@xzdandy @gaurav274 Fix all comments. Should be ready.

@gaurav274 gaurav274 merged commit 8d104fb into master Apr 6, 2023
@gaurav274 gaurav274 deleted the fix-ray branch April 6, 2023 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants