[serve.llm][bug] Copied Deprecation utility from RLlib to avoid unnecessary imports #56190
Conversation
…when using serve.llm Signed-off-by: ahao-anyscale <ahao@anyscale.com>
python/ray/serve/llm/__init__.py
Outdated
| @@ -1,5 +1,8 @@ | |||
| from typing import TYPE_CHECKING, Any, Dict, Optional, Union | |||
|
|
|||
| # Using Deprecated copied from ray.rllib.utils.deprecation since they are returning better messages. | |||
| # TODO: Ray core should inherit that. | |||
There was a problem hiding this comment.
nit: We usually put the name here, so we can map back who wrote the todo (e.g. TODO (ahao))
python/ray/serve/llm/__init__.py
Outdated
| @@ -1,5 +1,8 @@ | |||
| from typing import TYPE_CHECKING, Any, Dict, Optional, Union | |||
|
|
|||
| # Using Deprecated copied from ray.rllib.utils.deprecation since they are returning better messages. | |||
There was a problem hiding this comment.
nit: Put this in the top of the deprecation.py
Signed-off-by: ahao-anyscale <ahao@anyscale.com>
| @@ -0,0 +1,136 @@ | |||
| # Using Deprecated copied from ray.rllib.utils.deprecation since they are returning better messages. | |||
There was a problem hiding this comment.
We should move this module ray._common package
There was a problem hiding this comment.
we have our own common on llm stuff. I dont know if all ray libraries want to use the same style. Putting it in ray._common requires more thinking that we are leaving as a future TODO.
There was a problem hiding this comment.
Using Deprecated copied from ray.rllib.utils.deprecation
That makes it 2 libraries that use this module. Or did we make any changes specific to llm ?
There was a problem hiding this comment.
That makes it 2 libraries that use this module. Or did we make any changes specific to llm ?
No, but that's slower to land that way. Needs coordination with rllib (and potentially others) and we should not block this for PR as it has added unnecessary dependencies the way it was done so far. This is internal api anyways and is lightweight enough to maintain for now and we can reduce the tech debt later.
There was a problem hiding this comment.
but that's slower to land that way. Needs coordination with rllib (and potentially others)
I disagree; it's not that big of a lift to just do it as part of this PR, but that's just my opinion. I am not going to block this PR. Let's atleast create a GH issue for a first time contributor to pick up.
There was a problem hiding this comment.
@abrarsheikh do you guys use a different style for serve? Where are the utilities for serve around deprecation?
Why are these changes needed?
importing ray.serve.llm requires the user to install gymnasium, which is not used by serve.llm. This requirement is due to the usage of the Deprecation utility from RLlib. The utility is copied over to llm._internal.
Related issue number
Closes #56185
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.import ray.serve.llmwithout having gymnasium installed and it did not error.