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

[Datasets] Run _get_read_tasks with NodeAffinitySchedulingStrategy #33212

Merged
merged 5 commits into from
Mar 14, 2023
Merged

[Datasets] Run _get_read_tasks with NodeAffinitySchedulingStrategy #33212

merged 5 commits into from
Mar 14, 2023

Conversation

c21
Copy link
Contributor

@c21 c21 commented Mar 10, 2023 •
edited
Loading

Why are these changes needed?

This PR is to change _get_read_tasks always running as a Ray task at same node. For Ray client, the task will be executed on head node. The motivation for this PR is to simplify the logic so we don't run _get_read_tasks on arbitrary node.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • 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 :(

@c21 c21 force-pushed the fix-get-read-task branch from 439145a to 5972050 Compare March 13, 2023 00:31
@c21 c21 mentioned this pull request Mar 13, 2023
Closed
7 tasks
c21 and others added 4 commits March 13, 2023 22:52
@c21
Run _get_read_tasks at same node
d7cb328 
Signed-off-by: Cheng Su <scnju13@gmail.com>
@matthewdeng @c21
Avoid caching scheduling strategy
8cb05d2 
Signed-off-by: Matthew Deng <matt@anyscale.com>
@matthewdeng @c21
Update note
49f3b00 
Signed-off-by: Matthew Deng <matt@anyscale.com>
@c21
Add unit test
ed885cb 
Signed-off-by: Cheng Su <scnju13@gmail.com>
@c21 c21 force-pushed the fix-get-read-task branch from aef795d to ed885cb Compare March 14, 2023 05:52
@c21
Remove extra empty line
64b964a 
Signed-off-by: Cheng Su <scnju13@gmail.com>
@c21 c21 changed the title [WIP][Datasets] Run _get_read_tasks with NodeAffinitySchedulingStrategy [Datasets] Run _get_read_tasks with NodeAffinitySchedulingStrategy Mar 14, 2023
@c21 c21 added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Mar 14, 2023
@ericl ericl merged commit e5a52e5 into ray-project:master Mar 14, 2023
@c21 c21 deleted the fix-get-read-task branch March 14, 2023 18:51
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request Mar 21, 2023
@c21 @matthewdeng @ProjectsByJackHe
[Datasets] Run _get_read_tasks with NodeAffinitySchedulingStrategy (r…
cafdebe 
…ay-project#33212)

This PR is to change `_get_read_tasks` always running as a Ray task at same node. For Ray client, the task will be executed on head node. The motivation for this PR is to simplify the logic so we don't run `_get_read_tasks` on arbitrary node.

Co-authored-by: Matthew Deng <matt@anyscale.com>
Signed-off-by: Jack He <jackhe2345@gmail.com>
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
@c21 @matthewdeng @edoakes
[Datasets] Run _get_read_tasks with NodeAffinitySchedulingStrategy (r…
2b42740 
…ay-project#33212)

This PR is to change `_get_read_tasks` always running as a Ray task at same node. For Ray client, the task will be executed on head node. The motivation for this PR is to simplify the logic so we don't run `_get_read_tasks` on arbitrary node.

Co-authored-by: Matthew Deng <matt@anyscale.com>
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
peytondmurray pushed a commit to peytondmurray/ray that referenced this pull request Mar 22, 2023
@c21 @matthewdeng @peytondmurray
[Datasets] Run _get_read_tasks with NodeAffinitySchedulingStrategy (r…
603752c 
…ay-project#33212)

This PR is to change `_get_read_tasks` always running as a Ray task at same node. For Ray client, the task will be executed on head node. The motivation for this PR is to simplify the logic so we don't run `_get_read_tasks` on arbitrary node.

Co-authored-by: Matthew Deng <matt@anyscale.com>
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
@c21 @matthewdeng @elliottower
[Datasets] Run _get_read_tasks with NodeAffinitySchedulingStrategy (r…
0174b17 
…ay-project#33212)

This PR is to change `_get_read_tasks` always running as a Ray task at same node. For Ray client, the task will be executed on head node. The motivation for this PR is to simplify the logic so we don't run `_get_read_tasks` on arbitrary node.

Co-authored-by: Matthew Deng <matt@anyscale.com>
Signed-off-by: elliottower <elliot@elliottower.com>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
@c21 @matthewdeng @ProjectsByJackHe
[Datasets] Run _get_read_tasks with NodeAffinitySchedulingStrategy (r…
9d626e4 
…ay-project#33212)

This PR is to change `_get_read_tasks` always running as a Ray task at same node. For Ray client, the task will be executed on head node. The motivation for this PR is to simplify the logic so we don't run `_get_read_tasks` on arbitrary node.

Co-authored-by: Matthew Deng <matt@anyscale.com>
Signed-off-by: Jack He <jackhe2345@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewdeng matthewdeng matthewdeng approved these changes

@ericl ericl ericl approved these changes

@scv119 scv119 Awaiting requested review from scv119

@clarkzinzow clarkzinzow Awaiting requested review from clarkzinzow

@jjyao jjyao Awaiting requested review from jjyao

@jianoaix jianoaix Awaiting requested review from jianoaix

Assignees

@ericl ericl

@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.
4 participants
@c21 @ericl @matthewdeng @jianoaix