Repository navigation
fix(sglang): give the lazily exported handlers real types - #13682
ayaangazali wants to merge 1 commit into
Conversation
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 request handler module adds ChangesTyped lazy handler exports
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change restores static types for lazily exported SGLang handlers without changing runtime imports or 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)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
|
The From the job log, the failure is in Exit 35 is a TLS connect failure fetching the release tarball. Three things put it outside this PR: The asset is fine. That same URL returns 200 on retry, so the release was not removed and this is a transient network failure on the runner rather than a bad reference.
My four sibling PRs all pass Not pushing anything. There is no code fix for a failed binary download, and a speculative commit would only add noise. This needs a re-run. Worth separating from the other |
`dynamo/sglang/request_handlers/__init__.py` resolves its eleven handler classes through a module-level `__getattr__` so that importing the package does not pull in every backend submodule. mypy treats a module that defines `__getattr__` as possibly having any attribute, which costs two things here rather than one: - every handler resolves as `Any` for all consumers, so nothing about their constructors or methods is checked at a call site - a typo such as `DecodeWorkerHandlr` checks clean, in every module importing from this package Declare the eleven names under `TYPE_CHECKING`, mirroring `_EXPORTS`, and put `__getattr__` behind `if not TYPE_CHECKING`. The checker then sees the real classes, so `DecodeWorkerHandler` resolves to its actual constructor signature instead of `Any`, and an unknown name is reported with a suggestion. No runtime change. Importing the package still pulls in none of the handler submodules, `_EXPORTS` and `__all__` are untouched at eleven names each, and an unknown attribute still raises `AttributeError`. Signed-off-by: ayaangazali <ayaangazali.work@gmail.com>
1ab00dd to
f5b8864
Compare
GitHub Actions
Inactive
|
Rebased onto current The diagnosis in my earlier comment is unchanged: the failure was in Confirmed it was safe before force-pushing rather than assuming: no maintainer has posted Re-verified the behaviour on the new base: importing the package still pulls in none of the handler submodules, and |
|
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/sglang/request_handlers/__init__.pyresolves its eleven handler classes through a module-level__getattr__so that importing the package does not pull in every backend submodule.mypy treats a module defining
__getattr__as possibly having any attribute, and here that costs two things rather than one:Anyfor all consumers, so nothing about their constructors or methods is checked at a call site.The first is the bigger loss and is what makes this worth more than typo detection.
DecodeWorkerHandleris constructed in the sglang worker paths, and until now mypy had nothing to check those calls against.Declaring the eleven names under
TYPE_CHECKING, mirroring_EXPORTS, and putting__getattr__behindif not TYPE_CHECKINGgives the checker the real classes back.This completes the set. Same correction @GuanLuo asked for on #13626, already applied to the two
dynamo/commonpackages (#13666),dynamo/vllm/envs.py(#13680) andplanner/plugins/transport(#13681). One file and one owner area per PR on purpose; this is the last package in the repo with the pattern.Validation
reveal_type(DecodeWorkerHandler)Anydef (engine: Any, config: dynamo.sglang.args.Config, publisher: ... , shutdown_event: ..., enable_frontend_decoding: bool =, first_token_source: ...) -> ...DecodeWorkerHandlerAnyAnyimport DecodeWorkerHandlrhas no attribute "DecodeWorkerHandlr"; maybe "DecodeWorkerHandler"? [attr-defined]The "0 remaining
Any" line is the check that matters, and it doubles as the completeness proof that theTYPE_CHECKINGblock covers_EXPORTSexactly: any name I had missed would still have revealed asAny.Runtime behaviour unchanged, verified directly:
import dynamo.sglang.request_handlers,sys.modulescontains none of the handler submodules. Laziness is the reason this__getattr__exists, so this was the thing worth proving._EXPORTSand__all__are untouched at eleven names each, and every__all__entry is still covered by_EXPORTS.AttributeError: module 'dynamo.sglang.request_handlers' has no attribute 'DecodeWorkerHandlr'.mypyclean on the file.pre-commit run --filesinitially failed onisort, which reordered the new imports; re-run after applying that, it passes with zero failing hooks.Not verified here: I have no SGLang installed, so actually constructing a handler raises
ModuleNotFoundErroron this machine, on this branch and onmainalike. The type-level checks above were run against the declarations rather than by instantiating anything.Summary by CodeRabbit