Repository navigation
[Doc] Add environment variables configuration reference - #6217
Conversation
Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com>
Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com>
| def get_env_vars(): | ||
| try: | ||
| from vllm_omni.config.environment_variables import ENVIRONMENT_VARIABLES | ||
| except ImportError: |
There was a problem hiding this comment.
The failure modes that motivate this fallback aren't all ImportErrors. Importing vllm_omni.config.environment_variables executes vllm_omni/__init__.py, which imports vllm_omni.patch — a bare import torch (patch.py:7) raises OSError on a broken CUDA install, and the patch logic can also raise AssertionError (patch.py:73) or RuntimeError (patch.py:249); see the note in vllm_omni/__init__.py that patch may throw on vLLM version mismatch. None of those are ImportError subclasses, so get_env_vars() crashes on exactly the broken installs collect_env.py exists to diagnose — a regression vs. main, which never imported vllm_omni here. Since the allowlist is optional, suggest except Exception here (the (ImportError, NameError, AttributeError, OSError) tuple used for torch at line 26 would still miss the patch RuntimeError/AssertionError).
| _REFERENCE_PAGE = _REPO_ROOT / "docs" / "configuration" / "environment_variables.md" | ||
|
|
||
|
|
||
| def _literal_environment_accesses(path: Path) -> set[str]: |
There was a problem hiding this comment.
This gate only sees os.getenv("LITERAL")-shaped accesses, but the package already reads ~24 sites through shapes it cannot see: module-constant indirection (os.environ.get(AUDIO_SAMPLE_RATE_ENV) at vllm_omni/metrics/definitions.py:367), wrapper helpers (_choice_env/is_set in entrypoints/openai/video_stream_envs.py — both public video variables are read only this way), the pydantic-settings VLLM_OMNI_SERVER_* family (no os. call at all), and import os as _os (modeling_moss_tts_local_depth.py:221). A new variable added in any of these styles lands with no CI signal — the exact silent drift this inventory exists to prevent. Could the scan additionally resolve module-level all-caps string constants passed as the first argument, and match "X" in os.environ membership literals? If that is more machinery than wanted, the literal-only scope should at least be stated in the policy sentence at the end of the reference page so readers don't assume full coverage.
Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com>
Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com>
| assert {name for name in public_names if not name.startswith("VLLM_OMNI_")} == legacy_public_names | ||
|
|
||
|
|
||
| def test_statically_resolvable_environment_accesses_are_classified(): |
There was a problem hiding this comment.
This direction (code → inventory) is now solid, but the reverse has no guard: when a model is deleted or refactored away from its env switches, its MODEL_SPECIFIC/BENCHMARK_TRANSITIONAL rows linger with no CI signal, and the pinned counts in test_inventory_matches_reviewed_snapshot_counts actively discourage cleaning them up. Since several of those names are read through indirect sites, the robust form is a substring liveness check rather than the scanner: assert each name in those two categories still appears textually somewhere under vllm_omni/. That makes model removal update the inventory automatically.
| not listed as public usage options here because doing so would turn implementation | ||
| escape hatches into an accidental compatibility contract. | ||
|
|
||
| Every audited model-specific name has a migration disposition in the |
There was a problem hiding this comment.
The dispositions are unenforced promises as it stands — nothing forces a promote to ever execute, so the 54 can only shrink if each batch has an owner. Worth opening the per-family tracking issue(s) alongside this PR so they don't live only in this tuple: the 11 internalize rows need no work, the 5 deprecate_remove rows are trivial deletions, and the 38 promote/request-scope rows collapse into ~8 family-sized batches (DYNIN 7, INTERNVLA_A1 4, GEPARD 6, COSYVOICE3/MIMO/MINICPMO 3 each, COSMOS3 2, plus singles).
Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a reviewed, non-executable inventory of environment-variable names used in vLLM-Omni, adds centralized documentation for the public Omni-owned subset, and wires CI drift checks plus safer environment collection behavior to prevent accidental expansion of the supported env-var contract.
Changes:
- Add
ENVIRONMENT_VARIABLE_INVENTORYwith ownership/category metadata, model-switch migration dispositions, and redaction flags. - Add CI tests that (a) enforce reviewed inventory counts, (b) detect new direct env reads that aren’t classified, and (c) require public Omni variables to appear in the new docs page.
- Update
collect_env.pyto report only safe/public Omni values, preserve usefulness when Omni import fails, and avoid printing redacted/secret-like keys.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
vllm_omni/config/environment_variable_inventory.py |
New reviewed inventory of env-var names with categories/dispositions and redaction metadata. |
tests/config/test_environment_variables.py |
New drift/coverage tests that gate inventory growth, enforce doc coverage, and validate collector redaction behavior. |
docs/configuration/README.md |
Link configuration docs to the new Environment Variables reference page. |
docs/configuration/environment_variables.md |
New centralized environment-variable reference documenting the reviewed public Omni contract and governance boundary. |
docs/cli/README.md |
Cross-link CLI docs to the new Environment Variables page for precedence/evaluation-time guidance. |
collect_env.py |
Extend environment collection to include safe public Omni allowlist, redact sensitive keys, and remain functional on Omni import failures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def _resolve_environment_name(expression: ast.expr, constants: dict[str, str]) -> str | None: | ||
| value: str | None = None | ||
| if isinstance(expression, ast.Constant) and isinstance(expression.value, str): | ||
| value = expression.value | ||
| elif isinstance(expression, ast.Name): | ||
| value = constants.get(expression.id) | ||
| return value if value is not None and value.isupper() else None |
|
This PR appears to belong to: docs/design/module/vllm_omni_config.md. Module owners: @lishunyang12 @alex-jw-brooks @hsliuustc0106, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
|
I think we also need to update the related skills if the users want to add a new env var, where they should follow our general rule |
Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com>
Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com>
Signed-off-by: hsliuustc0106 <liuhongsheng4@huawei.com>
…#6217) Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com> Signed-off-by: AndyZhou952 <jzhoubc@connect.ust.hk>
… drift (vllm-project#6631) Signed-off-by: hsliuustc0106 <liuhongsheng4@huawei.com> Signed-off-by: AndyZhou952 <jzhoubc@connect.ust.hk>
…#6217) Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com>
… drift (vllm-project#6631) Signed-off-by: hsliuustc0106 <liuhongsheng4@huawei.com>
这两个 env 由上游 PR vllm-project#5638（f201b717，2026-08-04）引入、并收录于上游 env 文档（vllm-project#6217）， 不属于本线新增 —— 上一条提交把它们当成本线的 env 兜底删掉了，判断错误，现按上游原样恢复。 本线真正新增并转换的只有 VLLM_OMNI_C2W_SINGLE_ROW（c51109d4，up/main 无此变量）， 该项的 yaml 化（code2wav_single_row_batch）保持不变。
…#6217) Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com>
… drift (vllm-project#6631) Signed-off-by: hsliuustc0106 <liuhongsheng4@huawei.com>
Summary
envscopingvllm.envs-style resolver and require theVLLM_OMNI_prefix for future public namescollect_env.pywith the safe public Omni allowlist and keep diagnostics usable when Omni import failsWhy
vLLM-Omni currently reads environment variables across process startup, configuration, workers, model integrations, and benchmarks. Treating every discovered name as public would accidentally turn model-specific debug and compatibility switches into supported API.
This PR establishes a reviewed public boundary, records ownership for the remaining names, and gives model owners explicit migration targets without changing existing runtime behavior.
Upstream alignment
The page follows vLLM's placement under User Guide → Configuration, reuses the reviewed inventory in environment collection, and links inherited behavior to the aligned vLLM 0.26 reference.
This PR intentionally stops at audit and governance metadata:
ENVIRONMENT_VARIABLE_INVENTORYclassifies names but does not resolve values. Existing consumers continue to own parsing, defaults, precedence, and evaluation time. Converging the 22 public Omni settings on a typed/lazy resolver is follow-up work rather than expanding this audit across all 148 variables.Existing public names with older prefixes are grandfathered for compatibility. A regression test requires every future public Omni-owned name to use
VLLM_OMNI_.Model-specific boundary
The existing model-only environment switches are classified as migration inputs, not promoted into public API:
The exact decisions live in
vllm_omni/config/environment_variable_inventory.py. Implementation is tracked in #6232 as family-sized packages that require a named assignee and model-owner review.Validation
uvx pre-commit run --files ...mkdocs build --strict(exit 0)Fixes #6211