Repository navigation
fix(vllm): stop the envs __getattr__ hiding typos from mypy - #13680
ayaangazali wants to merge 1 commit into
Conversation
`dynamo/vllm/envs.py` resolves its environment-backed values through a
module-level `__getattr__`. mypy treats a module that defines `__getattr__` as
possibly having any attribute, so a typo checks clean:
envs.DYN_FORWADPASS_METRIC_PORT # note the missing R
and it checks clean in every module that imports `envs`, not only here.
The module already declares `DYN_FORWARDPASS_METRIC_PORT: int` under
`TYPE_CHECKING`, so the intent to keep the name statically visible is already
expressed; only the guard was missing. Putting `__getattr__` behind
`if not TYPE_CHECKING` makes the checker see exactly the declared name and
nothing else.
No runtime change. The value still resolves lazily through
`environment_variables`, an unknown name still raises `AttributeError`, the
port range validation still fires, `is_set` and `__dir__` are untouched.
Same correction @GuanLuo asked for on ai-dynamo#13626, and the same one applied to the
two `dynamo/common` packages in ai-dynamo#13666.
Signed-off-by: ayaangazali <ayaangazali.work@gmail.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
|
👋 Hi ayaangazali! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
external-contribution
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe change limits the module-level lazy ChangesVLLM environment lookup
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This localized change preserves runtime environment lookup behavior while making misspelled attributes fail static type checking; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Summary
dynamo/vllm/envs.pyresolves its environment-backed values through a module-level__getattr__. mypy treats a module that defines__getattr__as possibly having any attribute, so a typo checks clean:and it checks clean in every module that imports
envs, not just inside this file.What makes this the clearest of these cases is that the module already declares
DYN_FORWARDPASS_METRIC_PORT: intunderTYPE_CHECKING. The intent to keep the name statically visible is already expressed in the file; only the guard was missing. Putting__getattr__behindif not TYPE_CHECKINGmakes the checker see exactly the declared name and nothing else.This is the same correction @GuanLuo asked for on #13626, and the same one applied to the two
dynamo/commonpackages in #13666. Kept to one file and one owner area on purpose rather than bundling the remaining packages together.Validation
Checked with a control, because "mypy reports no error" can also mean the module was never really analysed. The declared name is used as that control: it resolves to
intboth before and after, so the probe is known to be reading real type information.reveal_type(envs.DYN_FORWARDPASS_METRIC_PORT)intintenvs.DYN_TYPO_DOES_NOT_EXISTModule has no attribute "DYN_TYPO_DOES_NOT_EXIST" [attr-defined]Runtime behaviour is unchanged, exercised directly rather than argued:
envs.DYN_FORWARDPASS_METRIC_PORTresolves to the default20380.20999is honoured on the next access, so the lookup is still lazy rather than captured at import.99still raises the range error (port 99 is outside of the registered port range), so_resolve_portvalidation is intact.AttributeError: module 'dynamo.vllm.envs' has no attribute 'DYN_NOPE'.__dir__()still returns['DYN_FORWARDPASS_METRIC_PORT']andis_setstill works.Test runs:
pytest components/src/dynamo/vllm/testsis 113 passed, 7 errors, identical on cleanmainwith the change stashed. The 7 aretest_state_agent.pycollection errors unrelated to this file.test_vllm_unit.pyis the one test module that touchesenvs, and it cannot be collected here at all (ModuleNotFoundError: No module named 'vllm'), on this branch and on cleanmainalike. I did not want to claim coverage I do not have, so the runtime checks above were run directly against the module instead.mypyclean on the file,pre-commit run --filespasses with zero failing hooks.Summary by CodeRabbit