-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Don't release resources during plasma fetch #13025
Conversation
@@ -274,6 +274,10 @@ RAY_CONFIG(int32_t, minimum_gcs_reconnect_interval_milliseconds, 5000) | |||
/// Whether start the Plasma Store as a Raylet thread. | |||
RAY_CONFIG(bool, plasma_store_as_thread, false) | |||
|
|||
/// Whether to release worker CPUs during plasma fetches. | |||
/// See https://github.com/ray-project/ray/issues/12912 for further discussion. | |||
RAY_CONFIG(bool, release_resources_during_plasma_fetch, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this flag? I can't really think of any cases where we would want this to be true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's here just in case we want to easily disable it (feature flag).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I still think it'd be nice to remove the feature flag and/or split the Fetch message in the future.
@@ -563,5 +563,59 @@ def test_fusion_objects(tmp_path, shutdown_only): | |||
assert is_test_passing | |||
|
|||
|
|||
# https://github.com/ray-project/ray/issues/12912 | |||
def do_test_release_resource(tmp_path, expect_released): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also parametrize the test with True/False for expect_released.
Yeah, I think we can clean up the call names in a later PR (avoid refactoring and feature changes in the same PR). |
Why are these changes needed?
This implements the change proposed in #12912 in a minimal way. It also cleans up the core worker a bit, removing unused flags.