Repository navigation
dev: bring ty to parity with mypy and suppress what it reports today - #4207
Draft
oresttokovenko wants to merge 13 commits into
Draft
oresttokovenko wants to merge 13 commits into
oresttokovenko wants to merge 13 commits into
Conversation
ty is not the gate, and reporting 451 diagnostics in CI would block every
PR, so the hook is manual-stage only. It sits next to mypy in the same
config file, pinned to the same version as the dev dependency group, and
runs via:
uv run prek run ty --stage manual -a
The mypy pre-commit hook runs 1.18.2 in an isolated environment, so a locally installed mypy disagreed with it: 2.4.0 reported 8 errors the gate did not, and without the hook's exclusion a plain 'mypy .' picked up 9k errors from generated Thrift bindings in vendor/. Pinning the dev dependency and excluding vendor/ make 'uv run mypy pyiceberg tests' and the gate agree on the same 0.
tests/ is 400 of the 851 diagnostics, and the package is what a gate would have to cover first. Add tests/ back before ty is considered for the gate, since mypy checks them today.
mypy runs with disallow_any_generics, so bare dicts and fields are rejected there, while ty leaves missing-type-argument at ignore by default. Enabling it surfaces 102 findings (451 -> 553 with tests/ excluded).
type: ignore was covering two real gaps: an unparameterised ClassVar[dict] and an unannotated __new__. Both checkers accept the class once the field and the constructor are annotated, and ty has no rule requiring parameter annotations, so the suppressions could not be replaced by configuration. __new__ returns Self rather than Singleton: a concrete return type makes every subclass constructor resolve to Singleton and produces 340 phantom diagnostics. __deepcopy__ takes memo again, matching copy.deepcopy and pydantic's RootModel (dropping it raised TypeError in test_deepcopy_of_singleton_fixed_type).
ty does not parse the bracketed form at all, while a bare # type: ignore counts as a suppression it may not need. 55 of the bare ignores in pyiceberg suppressed only mypy errors, so each one produced unused-type-ignore-comment. Naming the code removes those 55 warnings (549 -> 494 ty diagnostics) and leaves mypy passing, while the comments now say which error they cover. Lines that both checkers flag would need '# ty: ignore[rule]' appended after the mypy comment; none of the 55 here do.
The registry was ClassVar[dict[Any, Any]], so a wrongly typed key or a
non-Singleton value went unnoticed. The key is now (class, positional
args, hashable kwargs) and the value a Singleton.
The value cannot be Self: ClassVar rejects type variables, and both mypy
and ty reject Generic[Self] ('not a valid argument to Generic'), so the
return needs a cast. It is sound because cls is part of the key.
oresttokovenko
marked this pull request as draft
October 30, 2026 23:27
requires-python is 3.11, so the typing_extensions re-export trips UP035.
mypy checked all 214 files while ty started on the 91 in pyiceberg/, so neither result was comparable. Both now check pyiceberg/ at Python 3.11. Each ty rule that replaces a mypy flag is named next to it, and the modules that import optional extras allow their own unresolved imports, which is what ignore_missing_imports did globally. Adds a types group holding ty and the stub packages, to measure ty in the same bare environment the mypy hook runs in.
The ty-pre-commit hook resolved the project environment, so ty saw every library while the mypy hook saw only stubs. A local hook installs ty and the stubs into the hook environment, which ty picks up through the VIRTUAL_ENV the runner exports, so both checkers now see the same types. Flags live in the entry because hook args are also passed to uv pip install when the hook environment is built. The six --config lines are the deltas this bare environment needs, since most inline directives cover diagnostics that require the real libraries. Adds make typecheck-ty, because the hook is manual-stage and make lint skips it.
470 diagnostics in 45 files, suppressed in place so each can be deleted as the underlying error is fixed. The rules stay at "error", so a new diagnostic on an unsuppressed line still fails, and unused-ignore-comment stays on, so fixing a site without deleting its directive fails too. Rules total: missing-type-argument 126, invalid-method-override 104, invalid-argument-type 82, unresolved-attribute 54, invalid-return-type 44, no-matching-overload 36, then a tail of seven. Two signatures in io/pyarrow.py and one call in table/update/__init__.py were reflowed by ruff format, because the comments pushed them past 130 characters.
7 of 24 tasks
Runs ty in the project environment next to the mypy hook, as a non-blocking continue-on-error step on the 3.11 leg, so the 470 diagnostics that the inline comments suppress are what CI measures rather than the stub-only hook. No new job and no new required check, so python-ci-required and .asf.yaml stay as they are.
oresttokovenko
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Stage 1 of #4271. Puts ty and mypy on the same footing so the two can be compared, then suppresses what ty reports on the package today.
pyiceberg/(91 files) at Python 3.11. mypy checked all 214 files and ty the 91 in the package, so the two results were not comparable.disallow_any_generics,no_implicit_optional,warn_redundant_casts,warn_unreachable,warn_unused_ignores.implicit_reexporthas no ty equivalent, which [EPIC] Migrate static type checking to ty #4271 tracks as an open decision.make typecheck-tyruns it.uv run ty checkas acontinue-on-errorstep in the existinglint-and-unit-testjob, on the same 3.11 leg as the mypy hook. No new job, no new required check, no.asf.yamlchange.# ty: ignore[rule]per site, with 403 comments. Every rule stays at"error", so a new error on an unsuppressed line still fails, and a comment left behind after a fix fails as an unused suppression.Scope reduction to review: mypy now has
files = ["pyiceberg"]and its hook afiles: ^pyiceberg/filter, so the required mypy check no longer coverstests/(120 files) or the three scripts mypy used to check. That is what makes the two comparable. The alternative is leaving the shared mypy config alone and runningmypy pyicebergonly for the comparison. Say which you prefer.Are these changes tested?
uv run ty checkpasses,make typecheck-typasses,make lintpasses, and mypy still passes on the same 91 files. Each mirrored rule was measured before being set at"error": all report nothing inpyiceberg/at that level exceptmissing-type-argument, whose 126 diagnostics are among the suppressed ones.Are there any user-facing changes?
No. Tooling and configuration only, no runtime changes.