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

[core] Decouple create worker vs pop worker request. #47694

Merged
merged 14 commits into from
Oct 10, 2024
Merged

[core] Decouple create worker vs pop worker request. #47694

merged 14 commits into from
Oct 10, 2024

Conversation

rynewang
Copy link
Contributor

Now, when you call PopWorker(), it finds an idle one or creates a worker. If a new worker is created, the worker is associated to the request and can only be used by it.

This PR decouples the worker creation and the worker-to-task assignment, by adding an abstraction namely PopWorkerRequest. Now, if a req triggers a worker creation, the req is put into a queue. If there are workers ready, that is a PushWorker is called, either from a newly started worker or a released worker, Ray matches the first fitting request in the queue. This reduces latency.

Later it can also be used to pre-start workers more meaningfully.

@rynewang
decouple worker starting vs task pop worker reqs
c144846 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@rynewang
fixes, wordings
c00c238 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@rynewang
add unit test
55688db 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@rynewang
Copy link
Contributor Author

@jjyao pls take a look at the last commit to see if it makes sense.

@rynewang
Merge remote-tracking branch 'origin/master' into decouple-worker-req-2
7a715e6 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@rynewang
fix bug and remove redaundant arg
467940f 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@rynewang rynewang assigned rynewang and unassigned jjyao Sep 17, 2024
@rynewang
fix root matching
6e3e152 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
jjyao
jjyao reviewed Sep 26, 2024
View reviewed changes
Copy link
Collaborator

@jjyao jjyao left a comment

Choose a reason for hiding this comment

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

will review later

src/ray/raylet/worker_pool.cc Outdated Show resolved Hide resolved
jjyao
jjyao reviewed Oct 4, 2024
View reviewed changes
Copy link
Collaborator

@jjyao jjyao left a comment

Choose a reason for hiding this comment

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

lg

src/ray/raylet/worker_pool.cc Outdated Show resolved Hide resolved
src/ray/raylet/worker_pool.cc Outdated Show resolved Hide resolved
src/ray/raylet/worker_pool.cc Outdated Show resolved Hide resolved
src/ray/raylet/worker_pool.cc Outdated Show resolved Hide resolved
src/ray/raylet/worker_pool.cc Outdated Show resolved Hide resolved
@rynewang
renaming
941efa5 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@rynewang
rename, revert test
5114133 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@jjyao jjyao added the go add ONLY when ready to merge, run all tests label Oct 9, 2024
jjyao
jjyao reviewed Oct 9, 2024
View reviewed changes
Copy link
Collaborator

@jjyao jjyao left a comment

Choose a reason for hiding this comment

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

LG. Can you add some tests and then we can merge it

@rynewang
unit test
0fb249d 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@rynewang
fix cpp tests
a8e8f27 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@rynewang rynewang requested review from pcmoritz, raulchen and a team as code owners October 9, 2024 23:45
@rynewang
wording
6a65622 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@jjyao
Copy link
Collaborator

jjyao commented Oct 10, 2024

there is test failure

rynewang and others added 3 commits October 10, 2024 00:34
@rynewang
Merge branch 'master' into decouple-worker-req-2
c4e8bd7
@rynewang
test waits
5808783 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@rynewang
spike detection
ac9d36d 
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@rynewang rynewang assigned jjyao and unassigned jjyao Oct 10, 2024
@rynewang rynewang merged commit 301a3f0 into ray-project:master Oct 10, 2024
5 checks passed
@rynewang rynewang deleted the decouple-worker-req-2 branch October 11, 2024 05:01
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@rynewang @ujjawal-khare
[core] Decouple create worker vs pop worker request. (ray-project#47694)
f0204bc 
Now, when you call PopWorker(), it finds an idle one or creates a
worker. If a new worker is created, the worker is associated to the
request and can only be used by it.

This PR decouples the worker creation and the worker-to-task assignment,
by adding an abstraction namely PopWorkerRequest. Now, if a req triggers
a worker creation, the req is put into a queue. If there are workers
ready, that is a PushWorker is called, either from a newly started
worker or a released worker, Ray matches the first fitting request in
the queue. This reduces latency.

Later it can also be used to pre-start workers more meaningfully.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@rynewang @ujjawal-khare
[core] Decouple create worker vs pop worker request. (ray-project#47694)
f217921 
Now, when you call PopWorker(), it finds an idle one or creates a
worker. If a new worker is created, the worker is associated to the
request and can only be used by it.

This PR decouples the worker creation and the worker-to-task assignment,
by adding an abstraction namely PopWorkerRequest. Now, if a req triggers
a worker creation, the req is put into a queue. If there are workers
ready, that is a PushWorker is called, either from a newly started
worker or a released worker, Ray matches the first fitting request in
the queue. This reduces latency.

Later it can also be used to pre-start workers more meaningfully.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@rynewang @ujjawal-khare
[core] Decouple create worker vs pop worker request. (ray-project#47694)
b0c7f57 
Now, when you call PopWorker(), it finds an idle one or creates a
worker. If a new worker is created, the worker is associated to the
request and can only be used by it.

This PR decouples the worker creation and the worker-to-task assignment,
by adding an abstraction namely PopWorkerRequest. Now, if a req triggers
a worker creation, the req is put into a queue. If there are workers
ready, that is a PushWorker is called, either from a newly started
worker or a released worker, Ray matches the first fitting request in
the queue. This reduces latency.

Later it can also be used to pre-start workers more meaningfully.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@rynewang @ujjawal-khare
[core] Decouple create worker vs pop worker request. (ray-project#47694)
8c82c96 
Now, when you call PopWorker(), it finds an idle one or creates a
worker. If a new worker is created, the worker is associated to the
request and can only be used by it.

This PR decouples the worker creation and the worker-to-task assignment,
by adding an abstraction namely PopWorkerRequest. Now, if a req triggers
a worker creation, the req is put into a queue. If there are workers
ready, that is a PushWorker is called, either from a newly started
worker or a released worker, Ray matches the first fitting request in
the queue. This reduces latency.

Later it can also be used to pre-start workers more meaningfully.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@rynewang @ujjawal-khare
[core] Decouple create worker vs pop worker request. (ray-project#47694)
7eadf76 
Now, when you call PopWorker(), it finds an idle one or creates a
worker. If a new worker is created, the worker is associated to the
request and can only be used by it.

This PR decouples the worker creation and the worker-to-task assignment,
by adding an abstraction namely PopWorkerRequest. Now, if a req triggers
a worker creation, the req is put into a queue. If there are workers
ready, that is a PushWorker is called, either from a newly started
worker or a released worker, Ray matches the first fitting request in
the queue. This reduces latency.

Later it can also be used to pre-start workers more meaningfully.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@rynewang @ujjawal-khare
[core] Decouple create worker vs pop worker request. (ray-project#47694)
961589c 
Now, when you call PopWorker(), it finds an idle one or creates a
worker. If a new worker is created, the worker is associated to the
request and can only be used by it.

This PR decouples the worker creation and the worker-to-task assignment,
by adding an abstraction namely PopWorkerRequest. Now, if a req triggers
a worker creation, the req is put into a queue. If there are workers
ready, that is a PushWorker is called, either from a newly started
worker or a released worker, Ray matches the first fitting request in
the queue. This reduces latency.

Later it can also be used to pre-start workers more meaningfully.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@rynewang @ujjawal-khare
[core] Decouple create worker vs pop worker request. (ray-project#47694)
56e3a64 
Now, when you call PopWorker(), it finds an idle one or creates a
worker. If a new worker is created, the worker is associated to the
request and can only be used by it.

This PR decouples the worker creation and the worker-to-task assignment,
by adding an abstraction namely PopWorkerRequest. Now, if a req triggers
a worker creation, the req is put into a queue. If there are workers
ready, that is a PushWorker is called, either from a newly started
worker or a released worker, Ray matches the first fitting request in
the queue. This reduces latency.

Later it can also be used to pre-start workers more meaningfully.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@rynewang @ujjawal-khare
[core] Decouple create worker vs pop worker request. (ray-project#47694)
6a38914 
Now, when you call PopWorker(), it finds an idle one or creates a
worker. If a new worker is created, the worker is associated to the
request and can only be used by it.

This PR decouples the worker creation and the worker-to-task assignment,
by adding an abstraction namely PopWorkerRequest. Now, if a req triggers
a worker creation, the req is put into a queue. If there are workers
ready, that is a PushWorker is called, either from a newly started
worker or a released worker, Ray matches the first fitting request in
the queue. This reduces latency.

Later it can also be used to pre-start workers more meaningfully.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@rynewang @ujjawal-khare
[core] Decouple create worker vs pop worker request. (ray-project#47694)
3eff78e 
Now, when you call PopWorker(), it finds an idle one or creates a
worker. If a new worker is created, the worker is associated to the
request and can only be used by it.

This PR decouples the worker creation and the worker-to-task assignment,
by adding an abstraction namely PopWorkerRequest. Now, if a req triggers
a worker creation, the req is put into a queue. If there are workers
ready, that is a PushWorker is called, either from a newly started
worker or a released worker, Ray matches the first fitting request in
the queue. This reduces latency.

Later it can also be used to pre-start workers more meaningfully.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
@rynewang rynewang mentioned this pull request Nov 22, 2024
Merged
rynewang added a commit that referenced this pull request Nov 22, 2024
@rynewang
[core] Erases PopWorkerRequest on registration timeout. (#48858)
be0bbd0 
This fixes a bug from #47694. When a PopWorkerRequest is time out, we
should erase it from bookkeeping and invoke the callback with a
`WorkerPendingRegistration` status. However only the latter is done and
we forgot to erase, causing segfaults on a second call.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
MortalHappiness pushed a commit to MortalHappiness/ray that referenced this pull request Nov 22, 2024
@rynewang @MortalHappiness
[core] Erases PopWorkerRequest on registration timeout. (ray-project#…
c0c6d35 
…48858)

This fixes a bug from ray-project#47694. When a PopWorkerRequest is time out, we
should erase it from bookkeeping and invoke the callback with a
`WorkerPendingRegistration` status. However only the latter is done and
we forgot to erase, causing segfaults on a second call.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjyao jjyao jjyao approved these changes

@pcmoritz pcmoritz Awaiting requested review from pcmoritz pcmoritz is a code owner

@raulchen raulchen Awaiting requested review from raulchen raulchen is a code owner

Assignees

@jjyao jjyao

@rynewang rynewang

Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rynewang @jjyao