Skip to content

[data] New executor backend [1/n]--- Add basic interfaces#31216

Merged
ericl merged 6 commits intoray-project:masterfrom
ericl:interfaces-1
Dec 22, 2022
Merged

[data] New executor backend [1/n]--- Add basic interfaces#31216
ericl merged 6 commits intoray-project:masterfrom
ericl:interfaces-1

Conversation

@ericl
Copy link
Copy Markdown
Contributor

@ericl ericl commented Dec 20, 2022

Why are these changes needed?

This PR adds the basic interfaces and feature flags; split out from https://github.com/ray-project/ray/pull/30903/files

See REP ray-project/enhancements#18 for more details.

Copy link
Copy Markdown
Contributor

@c21 c21 left a comment

Choose a reason for hiding this comment

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

Thanks @ericl. Mostly looks good to me. Have some questions/minor comments.

Signed-off-by: Eric Liang <ekhliang@gmail.com>
Copy link
Copy Markdown
Contributor

@c21 c21 left a comment

Choose a reason for hiding this comment

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

LGTM, cc @clarkzinzow as well.

@ericl ericl merged commit 2cd4637 into ray-project:master Dec 22, 2022
Copy link
Copy Markdown
Contributor

@clarkzinzow clarkzinzow left a comment

Choose a reason for hiding this comment

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

Apologies for not getting this review in before the merge, mostly nits about type annotations and docstrings.

input. For most operators, this is always `0` since there is only
one upstream input operator.
"""
raise NotImplementedError
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How do we plan to propagate whether we need to preserve order down to operators at execution time?

Right now, the MapOperator in the BulkExecutor PR has hardcoded order preservation into its execution state, without a convenient PhysicalOperator API to change that behavior at execution time (not instantiation time). It looks like we need a PhysicalOperator API for registering/binding execution options, maybe an API that's symmetrical to PhysicalOperator.shutdown().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probably it will be added to execute()... maybe we should just omit it for now.

Copy link
Copy Markdown
Contributor Author

@ericl ericl left a comment

Choose a reason for hiding this comment

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

Thanks--- comments incorporated into the main PR.

AmeerHajAli pushed a commit that referenced this pull request Jan 12, 2023
tamohannes pushed a commit to ju2ez/ray that referenced this pull request Jan 25, 2023
…t#31216)

This PR adds the basic interfaces and feature flags; split out from https://github.com/ray-project/ray/pull/30903/files

See REP ray-project/enhancements#18 for more details.

Signed-off-by: tmynn <hovhannes.tamoyan@gmail.com>
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.

5 participants