-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
[Core] Rationalize boolean environment variable handling #19550
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: main
Are you sure you want to change the base?
Conversation
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
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.
Summary of Changes
Hello @art-dsit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses the current inconsistency in handling boolean environment variables by introducing a dedicated helper function. This function provides a single, clear way to parse boolean values from the environment, improving code readability and maintainability by replacing varied parsing methods with a consistent approach across the codebase.
Highlights
- Consistent Boolean Parsing: Introduced a new helper function
parse_bool_env
invllm/envs.py
to standardize how boolean environment variables are interpreted. This function accepts case-insensitive "true" or "1" asTrue
and uses a specified default otherwise. - Refactor Environment Variable Handling: Updated numerous existing environment variable parsing lambdas within the
_ENV_VARS
dictionary to utilize the newparse_bool_env
helper, replacing inconsistent ad-hoc parsing logic (e.g.,bool(int(...))
,.lower() == "true"
).
Using Gemini Code Assist
The 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 /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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 helper function parse_bool_env
to standardize the parsing of boolean environment variables. This is a good step towards improving code consistency and maintainability. The changes update numerous environment variable handlers to use this new function. Note that this standardization changes the exact set of string values interpreted as boolean for some variables, which is the intended rationalization but might affect users relying on previous, less consistent behavior.
"VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE": | ||
lambda: bool( | ||
os.environ.get("VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE", "1") != "0"), | ||
lambda: parse_bool_env("VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE", True), | ||
|
||
# Feature flag to enable/disable Inductor standalone compile. | ||
# In torch <= 2.7 we ignore this flag; in torch >= 2.8 this is | ||
# enabled by default. | ||
"VLLM_USE_STANDALONE_COMPILE": | ||
lambda: os.environ.get("VLLM_USE_STANDALONE_COMPILE", "1") == "1", | ||
lambda: parse_bool_env("VLLM_USE_STANDALONE_COMPILE", 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.
The original logic for VLLM_USE_STANDALONE_COMPILE
treated only the exact string "1" as True
(when unset, it defaulted to "1" which was then compared to "1"). The new parse_bool_env
function with a default of True
will treat unset, "1", and "true" as True
, and "0", "false", and other strings as False
. This is a change in behavior for string values other than "1". While standardizing to "1"/"true" is good, this change in behavior for non-"1" strings should be noted, as users might have relied on the old behavior.
# If set, the OpenAI API server will stay alive even after the underlying | ||
# AsyncLLMEngine errors and stops serving requests | ||
"VLLM_KEEP_ALIVE_ON_ENGINE_DEATH": | ||
lambda: bool(os.getenv("VLLM_KEEP_ALIVE_ON_ENGINE_DEATH", 0)), | ||
lambda: parse_bool_env("VLLM_KEEP_ALIVE_ON_ENGINE_DEATH"), |
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.
The original logic for VLLM_KEEP_ALIVE_ON_ENGINE_DEATH
used bool(os.getenv("..."))
with a default of 0
. This would treat any non-empty string (including "0", "false", "true", etc.) as True
, and only None
or 0
as False
. The new parse_bool_env
function with a default of False
will only treat "1" and "true" as True
, and everything else (including other non-empty strings) as False
. This is a significant change in how various string values are interpreted.
# If this env var is set to 1, vLLM will skip the peer-to-peer check, | ||
# and trust the driver's peer-to-peer capability report. | ||
"VLLM_SKIP_P2P_CHECK": | ||
lambda: os.getenv("VLLM_SKIP_P2P_CHECK", "0") == "1", | ||
lambda: parse_bool_env("VLLM_SKIP_P2P_CHECK"), |
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.
# Gaudi3. This is only applicable to HPU contiguous cache. If set to true, | ||
# contiguous cache fetch will be used. | ||
"VLLM_USE_HPU_CONTIGUOUS_CACHE_FETCH": | ||
lambda: os.environ.get("VLLM_CONTIGUOUS_PA", "true").lower() in | ||
("1", "true"), | ||
lambda: parse_bool_env("VLLM_CONTIGUOUS_PA", 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.
The original logic for VLLM_USE_HPU_CONTIGUOUS_CACHE_FETCH
treated "1" and "true" (case-insensitive) as True
, and other strings like "0" or "false" as True
(due to the default "true" and the in
check). The new parse_bool_env
function with a default of True
will treat unset, "1", and "true" as True
, and "0", "false", and other strings as False
. This changes the behavior for "0" and "false".
def parse_bool_env(name: str, default: bool = False) -> bool: | ||
"""Parse a boolean environment variable consistently. | ||
|
||
Accepts "1" or "true" (case-insensitive) as True, | ||
and anything else (including blank/unset) as the supplied default value. | ||
""" | ||
value = os.environ.get(name) | ||
if value is None: | ||
return default | ||
return value.lower() in ("1", "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.
# If true, will load models from ModelScope instead of Hugging Face Hub. | ||
# note that the value is true or false, not numbers | ||
"VLLM_USE_MODELSCOPE": | ||
lambda: os.environ.get("VLLM_USE_MODELSCOPE", "False").lower() == "true", | ||
lambda: parse_bool_env("VLLM_USE_MODELSCOPE"), |
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.
The comment for VLLM_USE_MODELSCOPE
mentions that the value is "true" or "false", not numbers. However, the new parse_bool_env
function also accepts "1" as True
. Consider updating the comment to reflect this.
# If true, will load models from ModelScope instead of Hugging Face Hub. | |
# note that the value is true or false, not numbers | |
"VLLM_USE_MODELSCOPE": | |
lambda: os.environ.get("VLLM_USE_MODELSCOPE", "False").lower() == "true", | |
lambda: parse_bool_env("VLLM_USE_MODELSCOPE"), | |
# If true, will load models from ModelScope instead of Hugging Face Hub. | |
# Accepts "1" or "true" (case-insensitive). | |
"VLLM_USE_MODELSCOPE": | |
lambda: parse_bool_env("VLLM_USE_MODELSCOPE"), |
|
||
# Whether to log responses from API Server for debugging | ||
"VLLM_DEBUG_LOG_API_SERVER_RESPONSE": | ||
lambda: os.environ.get("VLLM_DEBUG_LOG_API_SERVER_RESPONSE", "False" | ||
).lower() == "true", | ||
lambda: parse_bool_env("VLLM_DEBUG_LOG_API_SERVER_RESPONSE"), |
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.
Similar to VLLM_USE_MODELSCOPE
, the comment for VLLM_DEBUG_LOG_API_SERVER_RESPONSE
implies only "true" is accepted, but "1" is now also accepted. Consider updating the comment.
# Whether to log responses from API Server for debugging | |
"VLLM_DEBUG_LOG_API_SERVER_RESPONSE": | |
lambda: os.environ.get("VLLM_DEBUG_LOG_API_SERVER_RESPONSE", "False" | |
).lower() == "true", | |
lambda: parse_bool_env("VLLM_DEBUG_LOG_API_SERVER_RESPONSE"), | |
# Whether to log responses from API Server for debugging | |
# Accepts "1" or "true" (case-insensitive). | |
"VLLM_DEBUG_LOG_API_SERVER_RESPONSE": | |
lambda: parse_bool_env("VLLM_DEBUG_LOG_API_SERVER_RESPONSE"), |
3870229
to
9f64589
Compare
Signed-off-by: Art.OCathain <[email protected]>
This pull request has merge conflicts that must be resolved before it can be |
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.Purpose
The handling of boolean env vars is confusing at the moment. For example
VLLM_DEBUG_LOG_API_SERVER_RESPONSE
must be set to "true" (or "TRUE", "True", "tRuE" etc) - "1" won't workVLLM_USE_V1
must be set to "1" - "True" won't workI've added
parse_bool_env
and changed all the handlers to use it.Test Plan
Test Result
(Optional) Documentation Update