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.

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

[core] Prestart worker with runtime env #49994

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Jan 21, 2025

Troubleshooting result for runtime env performance issue:

Reading through the code, if runtime env empty, we prestart worker, otherwise we skip:

task_spec.HasRuntimeEnv() || task_spec.GetLanguage() != ray::Language::PYTHON) {

After the change, the performance is much better: https://buildkite.com/ray-project/release/builds/30878

@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Jan 21, 2025
@dentiny dentiny force-pushed the hjiang/prestart-worker-with-runtime-env branch 4 times, most recently from fbcd4bd to 4166389 Compare January 22, 2025 00:15
Signed-off-by: dentiny <[email protected]>
@dentiny dentiny force-pushed the hjiang/prestart-worker-with-runtime-env branch from 4166389 to c04f15d Compare January 22, 2025 00:32
Signed-off-by: dentiny <[email protected]>
@dentiny dentiny changed the title prestart worker with runtime env [core] Prestart worker with runtime env Jan 22, 2025
@dentiny dentiny requested review from jjyao and rynewang January 22, 2025 05:10
@dentiny dentiny requested a review from jjyao January 22, 2025 10:37
src/ray/raylet/worker_pool.cc 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
Signed-off-by: dentiny <[email protected]>
@dentiny dentiny requested a review from jjyao January 22, 2025 23:15
src/ray/raylet/worker_pool.cc Outdated Show resolved Hide resolved
src/ray/raylet/worker_pool.cc 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
Signed-off-by: dentiny <[email protected]>
@dentiny dentiny requested a review from jjyao January 23, 2025 00:52
Signed-off-by: dentiny <[email protected]>
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.

Add some tests

src/ray/raylet/worker_pool.cc Outdated Show resolved Hide resolved
@dentiny
Copy link
Contributor Author

dentiny commented Jan 23, 2025

Add some tests

Do you think it good enough to serve as assertion on #50003?

Signed-off-by: dentiny <[email protected]>
@dentiny dentiny requested a review from jjyao January 23, 2025 16:35
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
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. We can merge after adding tests.

@dentiny
Copy link
Contributor Author

dentiny commented Jan 25, 2025

LG. We can merge after adding tests.

Test added

@dentiny dentiny requested a review from jjyao January 25, 2025 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants