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

[Air][Data] Don't promote locality_hints for split #26647

Merged
merged 2 commits into from
Jul 18, 2022
Merged

[Air][Data] Don't promote locality_hints for split #26647

merged 2 commits into from
Jul 18, 2022

Conversation

scv119
Copy link
Contributor

@scv119 scv119 commented Jul 17, 2022

Signed-off-by: scv119 [email protected]

Why are these changes needed?

Since locality_hints is an experimental feature, we stop promoting it in doc and don't enable it in AIR. See #26641 for more context

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@scv119
add
3c38275 
Signed-off-by: scv119 <[email protected]>
@scv119 scv119 marked this pull request as ready for review July 17, 2022 19:09
@scv119 scv119 mentioned this pull request Jul 17, 2022
Merged
6 tasks
@matthewdeng
Copy link
Contributor

In order to disable for AIR we should change it in Train as well

ray/python/ray/train/_internal/dataset_spec.py

Lines 48 to 52 in 38c9e1d

return dataset_or_pipeline.split(
len(training_worker_handles),
equal=True,
locality_hints=training_worker_handles,
)

ray/python/ray/train/_internal/dataset_spec.py

Lines 209 to 213 in 38c9e1d

dataset_splits = dataset.split(
len(training_worker_handles),
equal=True,
locality_hints=training_worker_handles,
)

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jul 17, 2022
@scv119
address comments
aba4eca 
Signed-off-by: scv119 <[email protected]>
scv119 added a commit that referenced this pull request Jul 18, 2022
@scv119
[Data][split] use split_at_indices for equal split without locality h…
5ce06ce 
…ints (#26641)

This PR replaces dataset.split(.., equal=True) implementation by dataset.split_at_indices() . My experiments (the script
) showed that dataset.split_at_indices() have more predictable performance than the dataset.split(…)

Concretely: on 10 m5.4xlarge nodes with 5000 iops disk

calling ds.split(81) on 200GB dataset with 400 blocks: the split takes 20-40 seconds, split_at_indices takes ~12 seconds.

calling ds.split(163) on 200GB dataset with 400 blocks, the split takes 40-100 seconds, split_at_indices takes ~24 seconds.

I don’t have much insight of dataset.split implementation, but with dataset.split_at_indices() we are just doing SPREAD to num_split_at_indices tasks, which yield much stable performance.

Note: clean up the usage of experimental locality_hints in #26647
@scv119 scv119 merged commit b20f5f5 into ray-project:master Jul 18, 2022
@scv119 scv119 added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Jul 18, 2022
jianoaix pushed a commit to jianoaix/ray that referenced this pull request Jul 18, 2022
@scv119
[Data][split] use split_at_indices for equal split without locality h…
95cfc7d 
…ints (ray-project#26641)

This PR replaces dataset.split(.., equal=True) implementation by dataset.split_at_indices() . My experiments (the script
) showed that dataset.split_at_indices() have more predictable performance than the dataset.split(…)

Concretely: on 10 m5.4xlarge nodes with 5000 iops disk

calling ds.split(81) on 200GB dataset with 400 blocks: the split takes 20-40 seconds, split_at_indices takes ~12 seconds.

calling ds.split(163) on 200GB dataset with 400 blocks, the split takes 40-100 seconds, split_at_indices takes ~24 seconds.

I don’t have much insight of dataset.split implementation, but with dataset.split_at_indices() we are just doing SPREAD to num_split_at_indices tasks, which yield much stable performance.

Note: clean up the usage of experimental locality_hints in ray-project#26647
Signed-off-by: Ubuntu <[email protected]>
jianoaix pushed a commit to jianoaix/ray that referenced this pull request Jul 18, 2022
@scv119
[Air][Data] Don't promote locality_hints for split (ray-project#26647)
78783dc 
Why are these changes needed?
Since locality_hints is an experimental feature, we stop promoting it in doc and don't enable it in AIR. See ray-project#26641 for more context

Signed-off-by: Ubuntu <[email protected]>
xwjiang2010 pushed a commit to xwjiang2010/ray that referenced this pull request Jul 19, 2022
@scv119 @xwjiang2010
[Data][split] use split_at_indices for equal split without locality h…
cd060bd 
…ints (ray-project#26641)

This PR replaces dataset.split(.., equal=True) implementation by dataset.split_at_indices() . My experiments (the script
) showed that dataset.split_at_indices() have more predictable performance than the dataset.split(…)

Concretely: on 10 m5.4xlarge nodes with 5000 iops disk

calling ds.split(81) on 200GB dataset with 400 blocks: the split takes 20-40 seconds, split_at_indices takes ~12 seconds.

calling ds.split(163) on 200GB dataset with 400 blocks, the split takes 40-100 seconds, split_at_indices takes ~24 seconds.

I don’t have much insight of dataset.split implementation, but with dataset.split_at_indices() we are just doing SPREAD to num_split_at_indices tasks, which yield much stable performance.

Note: clean up the usage of experimental locality_hints in ray-project#26647
Signed-off-by: Xiaowei Jiang <[email protected]>
xwjiang2010 pushed a commit to xwjiang2010/ray that referenced this pull request Jul 19, 2022
@scv119 @xwjiang2010
[Air][Data] Don't promote locality_hints for split (ray-project#26647)
035a853 
Why are these changes needed?
Since locality_hints is an experimental feature, we stop promoting it in doc and don't enable it in AIR. See ray-project#26641 for more context

Signed-off-by: Xiaowei Jiang <[email protected]>
@matthewdeng matthewdeng mentioned this pull request Jul 25, 2022
Merged
6 tasks
Stefan-1313 pushed a commit to Stefan-1313/ray_mod that referenced this pull request Aug 18, 2022
@scv119
[Data][split] use split_at_indices for equal split without locality h…
ea9790a 
…ints (ray-project#26641)

This PR replaces dataset.split(.., equal=True) implementation by dataset.split_at_indices() . My experiments (the script
) showed that dataset.split_at_indices() have more predictable performance than the dataset.split(…)

Concretely: on 10 m5.4xlarge nodes with 5000 iops disk

calling ds.split(81) on 200GB dataset with 400 blocks: the split takes 20-40 seconds, split_at_indices takes ~12 seconds.

calling ds.split(163) on 200GB dataset with 400 blocks, the split takes 40-100 seconds, split_at_indices takes ~24 seconds.

I don’t have much insight of dataset.split implementation, but with dataset.split_at_indices() we are just doing SPREAD to num_split_at_indices tasks, which yield much stable performance.

Note: clean up the usage of experimental locality_hints in ray-project#26647
Signed-off-by: Stefan van der Kleij <[email protected]>
Stefan-1313 pushed a commit to Stefan-1313/ray_mod that referenced this pull request Aug 18, 2022
@scv119
[Air][Data] Don't promote locality_hints for split (ray-project#26647)
82affc4 
Why are these changes needed?
Since locality_hints is an experimental feature, we stop promoting it in doc and don't enable it in AIR. See ray-project#26641 for more context

Signed-off-by: Stefan van der Kleij <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amogkam amogkam amogkam left review comments

@jianoaix jianoaix jianoaix approved these changes

@ericl ericl ericl approved these changes

@clarkzinzow clarkzinzow Awaiting requested review from clarkzinzow

@jjyao jjyao Awaiting requested review from jjyao

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

Assignees

@matthewdeng matthewdeng

@clarkzinzow clarkzinzow

@amogkam amogkam

@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.
6 participants
@scv119 @matthewdeng @ericl @amogkam @jianoaix @clarkzinzow