Enforce 100% type-hint coverage and align typing to PEP 585/604#673
Enforce 100% type-hint coverage and align typing to PEP 585/604#673GiGiKoneti wants to merge 3 commits into
Conversation
|
|
@sadamov |
There was a problem hiding this comment.
Thanks @GiGiKoneti. Since #670 is type-hints only: could we keep executed code identical to main and lean on cast / # type: ignore (and the narrowing asserts you already added) where the checker needs a hint? Inline notes below, this looks good, I just invested a few hours to get this review right. Moving towards fully typed (and enforced) is not a small step!
On the class-level attribute blocks: most attrs are plain self.x = ... in __init__ that mypy already infers, so I've suggested keeping only the ones it can't (register_buffer tensors, graph-dict-loaded features, and the subclass narrowings) and dropping the rest.
Non-blocking: the T | None we own is narrowed two ways, assert ... is not None (pred_std, self.da_state) and nine # type: ignore[union-attr] on self.logger; I'd pick one for the cases we control and keep the coded ignores for the genuinely dynamic ones (self.hparams.*, log_image).
|
Thanks @sadamov for investing the time into this detailed review; all requested changes have been resolved.
All local checks and test suites pass successfully. |
|
I think you forgot to unhide the 14+ collapsed inline suggestions above ^^ or did you not agree with them? |
Co-authored-by Antigravity
30f26e0 to
97e40a2
Compare
I had few doubts earlier actually and also forgot later on ..nvm I figured out things..can u review these implementations please
|
97e40a2 to
dedc3a5
Compare
dedc3a5 to
dd3b094
Compare
|
@sadamov |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@GiGiKoneti nice! could you again make sure to use the full PR template please. PS: I only just pushed five |
|
I updated the pull request description to use the full template and pulled in your docstring alignment commit. I agree with asking on Slack to include this in v0.7.0 without a dev-meeting so we can merge it quickly. I'm sure these type hints will help the active work going on |
Convert the legacy typing aliases left in modules outside the earlier type-hint pass (metrics, custom_loggers, plot_graph, npyfilesmeps/config, and the datastore base docstrings) to native generics and the | union operator, so the codebase-wide PEP 585/604 claim holds. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Approving, thanks @GiGiKoneti, this is in good shape. I pushed fda55de to finish PEP 585/604 in the few modules the earlier pass had not touched (metrics, custom_loggers, plot_graph, npyfilesmeps/config, and the base docstrings), so the codebase-wide claim now holds.
One thing for a follow-up rather than a blocker. The gate is coverage-only right now: the pre-commit mypy hook runs isolated with no torch/numpy/xarray, so ignore_missing_imports turns every library type into Any. It proves every def is annotated (the #670 ask) but will not catch a wrong annotation.
Pointing mypy at the synced deps (mypy --python-version 3.12 neural_lam) finds 7 errors:
store.py:492step_length * np.arange(...)->timedelta * ndarrayutils.py:594rank_zero_only.rank-> overload has no.rankutils.py:747torch.clamp(x, min=<Tensor>, max=<float>)-> no matching overloadplot_example.py:82da.plot(...)-> xarray plot-accessor__call__create_graph.py:67LineCollection(edge_lines)-> ndarray not a Sequencegnn_layers.py:95/:103edge_mlp/aggr_mlp->SplitMLPsvsSequential
The first five are exactly the lines we reverted to keep runtime identical to main, so the clean fix is a targeted # type: ignore[code] each, like set_powerlimits in vis.py. The gnn_layers one wants the edge_mlp: nn.Module / aggr_mlp: nn.Module annotations back, a subset of the block I asked to drop.
So the open question: do we want the gate to actually type-check against the deps (install them in the hook or a separate CI step), or keep it coverage-only for now? My lean is to fix this now for v0.7.0.
|
@sadamov Let's merge this as the coverage gate for v0.7.0 to secure the 100% annotation baseline without further scope creep. I will open a follow-up issue to configure the strict dependency type-check and address the 7 remaining errors, keeping the |
|
@GiGiKoneti sounds good, in the follow-up we can also try to properly implement typing for the 6 cases above and not just ignore it. even if that means that the code behavior slightly changes. |
|
@sadamov Should I create the follow-up issue for this now? |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@GiGiKoneti let's wait until we discuss typing at the next dev meeting on July 13. no need to keep the PR up to date with main until then |
Describe your changes
neural_lam/codebase by annotating all remaining parameters and return values.list,dict, andtuple) and PEP 604 (using the union operator|).flake8line-length issues.disallow_untyped_defs/disallow_incomplete_defs) inpyproject.toml.Issue Link
closes #670
Type of change
Checklist before requesting a review
pullwith--rebaseoption if possible).Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
reflecting type of change (add section where missing):
Checklist for assignee