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

[serve] Set "memory" to None in ray_actor_options by default #23619

Merged
merged 6 commits into from
Apr 1, 2022

Conversation

shrekris-anyscale
Copy link
Contributor

@shrekris-anyscale shrekris-anyscale commented Mar 30, 2022

Why are these changes needed?

By default, Serve sets "memory" in ray_actor_options to 0. However, this is simply ignored in the resources_from_resource_arguments() function. Additionally, setting memory to 0 is explicitly disallowed in the to_memory_units() function.

This change sets Serve's default value for "memory" to None, so it uses Ray's default "memory" value.

Related issue number

Closes #23616.

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
    • New unit tests are added to test_config.py.

@simon-mo
Copy link
Contributor

alternatively, try unset the field if not specified? This should work with any future Ray default values.

@edoakes
Copy link
Contributor

edoakes commented Mar 31, 2022

@suquark @shrekris-anyscale I'm not quite understanding the context here. Does this mean that setting memory to 0 in the Ray API isn't supported? Is this a bug in the API? Setting it to 1 seems like a hack to me.

@suquark
Copy link
Member

suquark commented Mar 31, 2022

@edoakes yes, Ray "intentionally" does not support 0 memory (but you can set memory=None), limited here:

resources["memory"] = ray_constants.to_memory_units(memory, round_up=True)
. But it could be just due to some tech debts, another solution is to support 0 memory and converts memory=None to memory=0

@shrekris-anyscale
Copy link
Contributor Author

@edoakes @simon-mo In that case, should we set the default "memory" value to None instead of 0 or 1? @simon-mo unsetting all the fields could also work, but Serve's defaults are sometimes different than Ray (e.g. Serve's num_cpus uses 1 instead of the number of virtual cores).

@edoakes
Copy link
Contributor

edoakes commented Mar 31, 2022

@shrekris-anyscale yes I think we should just set it to None in that caes

@shrekris-anyscale shrekris-anyscale changed the title [serve] Make default "memory" in ray_actor_options 1 [serve] Set "memory" to None in ray_actor_options by default Mar 31, 2022
@shrekris-anyscale
Copy link
Contributor Author

@edoakes I changed the default to None. Do you happen to know what Ray's default "memory" value is?

@@ -258,13 +258,14 @@ def _validate(self):
raise ValueError("num_gpus in ray_actor_options must be >= 0.")
self.resource_dict["GPU"] = num_gpus

if self.ray_actor_options.get("memory", None) is None:
self.ray_actor_options["memory"] = 0
self.ray_actor_options.setdefault("memory", None)
memory = self.ray_actor_options["memory"]
Copy link
Member

Choose a reason for hiding this comment

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

actually you should not change the options (this changes options, making it harder to understand and debug), you just skip setting memory, that will make memory None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by changes options? Wouldn't setting memory to None and not setting memory at all have identical effects? These ray_actor_options ultimately get passed into an actor's options call, which also sets memory to None by default.

Copy link
Member

@suquark suquark Apr 1, 2022

Choose a reason for hiding this comment

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

Later PR deprecates this behavior (also see #api-changes): #23127. But it does not matter too much.

@suquark
Copy link
Member

suquark commented Mar 31, 2022

also I am thinking if we should be explicit about default values? when users want to set memory to 0, you just set 0 instead of default value; when it is not set, you are using default value, which may not be 0.

I strongly feel it is a bad design if the default value is bound with 0, and you cannot use 0 directly.

@shrekris-anyscale
Copy link
Contributor Author

I strongly feel it is a bad design if the default value is bound with 0, and you cannot use 0 directly.

This change raises an exception if the user passes in 0. Assuming Ray doesn't use 0 by default, the memory won't be 0 under any case.

@suquark suquark merged commit d4747d2 into ray-project:master Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug][Serve] Set memory to 0 never works in serve
4 participants