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

[data] [streaming] No preserve order by default #32300

Merged
merged 5 commits into from
Feb 8, 2023
Merged

[data] [streaming] No preserve order by default #32300

merged 5 commits into from
Feb 8, 2023
+21 −10

Conversation

ericl
Copy link
Contributor

@ericl ericl commented Feb 8, 2023

Signed-off-by: Eric Liang ekhliang@gmail.com

Why are these changes needed?

Preserve order decreases performance; set it off by default.

@ericl
flip on by default
60b066b 
Signed-off-by: Eric Liang <ekhliang@gmail.com>
@ericl ericl assigned c21 Feb 8, 2023
jianoaix
jianoaix reviewed Feb 8, 2023
View reviewed changes
Copy link
Contributor

@jianoaix jianoaix left a comment

Choose a reason for hiding this comment

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

LG for performance, as long as the most use case doesn't actually need order preservation.

python/ray/data/_internal/execution/interfaces.py Outdated Show resolved Hide resolved
@ericl
fix tests
e332923 
Signed-off-by: Eric Liang <ekhliang@gmail.com>
@ericl
fix doctest
1cdf6d2 
Signed-off-by: Eric Liang <ekhliang@gmail.com>
@ericl
fix doctest
5ca21ab 
Signed-off-by: Eric Liang <ekhliang@gmail.com>
@ericl
update comment
dae7b2f 
Signed-off-by: Eric Liang <ekhliang@gmail.com>
@ericl ericl added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Feb 8, 2023
clarkzinzow
clarkzinzow approved these changes Feb 8, 2023
View reviewed changes
Copy link
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.

LGTM! I'm guessing that we'll eventually expose this as an AIR-level config, and maybe make the default for streaming training ingest to be True?

@ericl ericl merged commit f05eeb4 into ray-project:master Feb 8, 2023
@ericl
Copy link
Contributor Author

ericl commented Feb 8, 2023

I think we'd still default to false, but yeah it should be exposed in the AIR config.

cadedaniel added a commit that referenced this pull request Feb 14, 2023
@cadedaniel
Revert "[data] [streaming] No preserve order by default (#32300)"
1b5d7c8 
This reverts commit f05eeb4.
@cadedaniel cadedaniel mentioned this pull request Feb 14, 2023
Closed
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
@ericl @edoakes
[data] [streaming] No preserve order by default (ray-project#32300)
a76e44e 
Signed-off-by: Eric Liang ekhliang@gmail.com

Why are these changes needed?
Preserve order decreases performance; set it off by default.

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jianoaix jianoaix jianoaix left review comments

@clarkzinzow clarkzinzow clarkzinzow approved these changes

@scv119 scv119 Awaiting requested review from scv119

@jjyao jjyao Awaiting requested review from jjyao

@c21 c21 Awaiting requested review from c21

Assignees

@stephanie-wang stephanie-wang

@c21 c21

@clarkzinzow clarkzinzow

@jianoaix jianoaix

Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ericl @clarkzinzow @jianoaix @stephanie-wang @c21