-
Notifications
You must be signed in to change notification settings - Fork 6.7k
[serve] Add warning for env vars with no RAY_SERVE_ prefix #55864
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: axreldable <[email protected]>
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.
Code Review
This pull request introduces a warning for environment variables used in Ray Serve that do not have the RAY_SERVE_
prefix. This is a good step towards standardizing environment variable names. However, the current implementation has a few issues.
First, the implementation is incomplete. The warning is only triggered for functions that use _get_env_value
internally (like get_env_int
, get_env_float
). The get_env_str
and get_env_bool
functions do not use _get_env_value
and thus will not trigger the warning. This should be fixed to ensure all environment variables are checked.
Second, there is a bug in how the warning's stacklevel
is determined. It's hardcoded, which will cause warnings to point to the wrong location in the code for some call stacks.
I've added specific comments with suggestions for fixing these issues, improving the implementation, and adding necessary test coverage.
Signed-off-by: axreldable <[email protected]>
…tant_utils.py Signed-off-by: axreldable <[email protected]>
Signed-off-by: axreldable <[email protected]>
/gemini |
Hi @axreldable! It looks like you might have intended to trigger a code review. If so, please use the command |
/gemini help |
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
/gemini review |
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.
Code Review
This pull request introduces a FutureWarning
for Ray Serve environment variables that are missing the RAY_SERVE_
prefix, which will be required in a future version. The implementation correctly centralizes the warning logic in a new _name_warning
function called from the _get_env_value
helper. The changes also refactor existing get_env_*
functions to use this central helper, ensuring consistent behavior. The tests have been properly updated to account for the new warning.
I have two suggestions for improvement. First, the stacklevel
for the warning is hardcoded, which can lead to incorrect source line reporting for warnings from nested helper functions. Second, I've identified a pre-existing type hint inaccuracy in get_env_str
that this PR's refactoring makes more apparent. Addressing these points will enhance the code's correctness and the overall developer experience.
Signed-off-by: axreldable <[email protected]>
/gemini review |
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.
Code Review
This pull request introduces a helpful warning for Ray Serve environment variables that are missing the RAY_SERVE_
prefix, which will be required in a future version. The changes are well-implemented, refactoring existing utility functions to consistently apply the new warning. The accompanying test updates are also correct and verify the new behavior. I have one minor suggestion to improve code style.
Hi @abrarsheikh ! Could you please check this? |
Here’s the approach I’d suggest:
We can’t immediately enforce the renaming of existing variables, since that would be too disruptive. These variables have been around for a long time, and forcing a change would risk breaking production setups when users upgrade. |
Why are these changes needed?
Ray Serve
environment variables. Since version2.50.0
the prefixRAY_SERVE_
will be required for all env variables. Warning message contains an instruction for users to change wrong env names.Commands:
Output:
get_env_str
andget_env_bool
to use internal_get_env_value
+ update type hints (after Gemini review)Related issue number
Closes #55453
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.