Repository navigation
fix(common): stop the lazy __getattr__ hiding typos from mypy - #13666
ayaangazali wants to merge 1 commit into
Conversation
`dynamo/common/__init__.py` and `dynamo/common/configuration/__init__.py` both
resolve their exports through a module-level `__getattr__`. mypy treats a module
with `__getattr__` as possibly having any attribute, so a typo such as
from dynamo.common import config_dumps # note the s
from dynamo.common.configuration import ArgGroups
type-checks clean, in every module that imports from these packages rather than
only in the package itself. `dynamo.common` is imported across the tree, so the
lost typo detection is not local.
Declare the real exports under `TYPE_CHECKING` and put the `__getattr__` behind
`if not TYPE_CHECKING`. The checker then sees exactly the names each package
actually provides, and the runtime keeps resolving them lazily as before.
Verified against a control rather than assumed: modules with no `__getattr__`
(`dynamo.common.multimodal`, `dynamo.planner.core.budget`) already reject an
unknown name, which is what these two now do too.
No runtime change. Importing `dynamo.common` still pulls in none of
`config_dump`, `constants`, `snapshot` or `utils` until one is touched, the
lazy import still caches into module globals, `__all__` is unchanged, and an
unknown attribute still raises `AttributeError`.
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
fix
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 (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe package initializers now declare lazy exports during type checking. Runtime ChangesLazy module typing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change narrows static type checking for two package exports while preserving runtime lazy loading and existing behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
|
Re-checked on current Both modules still define an unguarded module-level Probe: from dynamo.common import DoesNotExistAtAll
from dynamo.common.configuration import AlsoNotRealOn One note on reproducing it, because my first attempt did not and it would be easy to conclude there is nothing here. Running mypy against the installed package returns Nothing pushed, diff unchanged since it was opened, still MERGEABLE with no failing checks. |
Summary
dynamo/common/__init__.pyanddynamo/common/configuration/__init__.pyboth resolve their exports through a module-level__getattr__. mypy treats a module that defines__getattr__as possibly having any attribute, so a typo type-checks clean:The part that makes this worth fixing is that the widening is not local to these files. It applies in every module that imports from these packages, and
dynamo.commonis imported across the tree.The fix is to declare the real exports under
TYPE_CHECKINGand put__getattr__behindif not TYPE_CHECKING. The checker then sees exactly the names each package provides; the runtime keeps resolving them lazily, unchanged.This is the same correction @GuanLuo asked for on #13626, applied to the two other packages in
dynamo/commonthat have the pattern. It is deliberately scoped to those two so it stays in one owner area.Three more packages have the same shape and are not touched here, because they belong to other areas and I did not want to make this a four-area review. Measured the same way, all three currently accept an unknown name:
dynamo/sglang/request_handlers/__init__.pydynamo/planner/plugins/transport/__init__.pydynamo/vllm/envs.pyenvs.pyis the interesting one: it already declaresDYN_FORWARDPASS_METRIC_PORTunderTYPE_CHECKING, so half the pattern is there and only the guard is missing. Happy to follow up on all three, together or separately, whichever suits the owners.Validation
Checked with a control, since "mypy reports no error" can mean the module was never really analysed. Modules with no
__getattr__were confirmed to reject an unknown name first, so the probe is known to be capable of failing:dynamo.commonhas no attribute "ZZZDoesNotExist" [attr-defined]dynamo.common.configurationhas no attribute "ZZZDoesNotExist" [attr-defined]dynamo.common.multimodal(control, no__getattr__)dynamo.planner.core.budget(control, no__getattr__)Real names still resolve, and with precise types rather than
Any:Runtime behaviour is unchanged, verified rather than argued:
import dynamo.common, none ofconfig_dump,constants,snapshot,utilsis insys.modules. Laziness is preserved.dynamo.common.utilsthen loads it and caches it into module globals.ArgGroup,ConfigBaseandenv_or_defaultall resolve fromdynamo.common.configuration.AttributeErrorat runtime on both packages.__all__is unchanged on both.Test runs, with the same numbers on clean
mainwith the change stashed, so the failures below are pre-existing and unrelated:pytest components/src/dynamo/common/tests/— 488 passed, 14 failed, 6 skipped, identical before and after. The 14 aretest_video_utils.pycodec tests.pytest components/src/dynamo/planner/tests/unit— 608 passed, 1 failed, 3 skipped, identical before and after, excluding three modules that fail to collect on any tree here for missing optional deps (pmdarimaand friends).mypyclean on both changed files,pre-commit run --filespasses with zero failing hooks.Summary by CodeRabbit