[EPIC] Migrate static type checking to ty
Maintainer antworten meist innerhalb von 1 Tag
Dieses Issue hat noch niemand übernommen.
Bewertung
- Schwierigkeit
- 5/5
- Geschätzter Aufwand
- Über eine Woche
- Anfängerfreundlichkeit
- 12/100
Rechercherichtung
This is a multi-stage epic, not a starter task. Stage 3 is the most approachable piece: pick one suppressed ty rule such as invalid-method-override and remove its # ty: ignore comments in pyiceberg/. Start by running make typecheck-ty and uv run ty check to reproduce the diagnostics. Done means the rule stays at error and the check passes with the comments removed.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Beschreibung
Feature Request / Improvement
Why
ty is a modern, Rust-based type checker built for fast feedback in CI and editors
- ty is faster, I timed three cold runs (runs without a cache) on the 91 package files, in the same environment. ty took 0.37 to 0.44 s, and mypy took 6.28 to 6.37 s
- ty includes a language server, so editors show the same diagnostics (errors and warnings) as CI
- Its type system narrows types with intersections and skips branches that type inference shows are unreachable, mypy just assumes
Any
The mypy setup we have is broken, and that makes parts of this migration easier than it looks. It passes because its pre-commit environment holds four stub packages, so Pydantic, PyArrow and the rest are Any there. Run the same mypy against the real environment, mypy --python-executable .venv/bin/python, and it reports 561 errors in 35 files.
There is no clean baseline to preserve. Matching mypy's green would mean pointing ty at the same stub environment, which drops its count from 470 to 104. The libraries have to be hidden to reproduce the number we currently call passing. ty reaches the real figure about fifteen times faster.
Prior work
#3964 added the ty configuration, and #3965 fixed Pydantic subclass constructors and test annotations, which cut the count from 3,547 to 655 diagnostics at the time. #4207 adds ty as a pre-commit hook that runs in its own stub environment, and reports on pull requests without blocking them
Baseline
I measured these numbers at commit 12a8f3b1 with ty 0.0.84, mypy 1.18.2 and Python 3.11, after make install. Both checkers now start on the 91 files in pyiceberg/. The rows that say "before stage 1" are what the checker reports on the code itself, with no suppressions; the rows after them are only green because stage 1 added the comments below, so they say nothing about the code being clean
CI does not run the project-environment check at all. make typecheck-ty runs the stub-only hook, which has six rules switched off, so the 470 and the 403 comments are a local measurement until something runs uv run ty check
| Check | Command | Result |
|---|---|---|
| CI mypy (pre-commit environment) | uv run prek run mypy -a |
Passes on 91 files, because this environment cannot see the libraries |
| mypy (project environment) | uv run mypy |
561 errors in 35 files |
| ty (project environment), before stage 1 | uv run ty check |
470 diagnostics in 45 files |
| ty (project environment), stage 1 | uv run ty check |
Passes, covering the 470 with 403 # ty: ignore comments |
| ty (stub-only environment), before stage 1 | ty check against the types group venv |
104 diagnostics, 3 of them artifacts of the missing libraries |
| ty (stub-only environment), stage 1 | make typecheck-ty |
Passes, with the 403 comments plus the hook's six rule deltas |
ty in a tests/ scope |
uv run ty check -c 'src.include = ["pyiceberg", "tests"]' |
403 further diagnostics, all in tests/. Out of scope for now |
The two environments are not interchangeable: the 104 and the 470 describe different code, so suppression comments written against one environment do not hold in the other
Settings
This table shows which rule enforces each setting in [tool.mypy] after mypy is gone
| mypy setting | mypy default | Replacement | Wired |
|---|---|---|---|
disallow_untyped_defs |
false | Ruff ANN0 and ANN2 with mypy-init-return = true |
No, stage 2 |
disallow_any_generics |
false | ty missing-type-argument |
Yes |
warn_unused_ignores |
false | ty unused-ignore-comment and unused-type-ignore-comment |
Yes |
warn_redundant_casts |
false | ty redundant-cast |
Yes |
warn_unreachable |
false | ty redundant-condition, partly |
Yes |
implicit_reexport = false |
true | None. ty has no equivalent rule among its 254 | No, decision needed |
ignore_missing_imports = true |
false | ty allowed-unresolved-imports, per module |
Yes |
no_implicit_optional = true |
true | ty invalid-parameter-default |
Redundant: mypy already defaults to true |
Seven of the eight lines are deviations from a mypy default; no_implicit_optional is not one of them, and running mypy 1.18.2 with an empty configuration shows it already errors on def f(x: int = None). Its ty mirror therefore enforces nothing the project did not already enforce
By default, ty 0.0.84 fails on warnings (terminal.error-on-warning = true). I kept that default, so the rules above stay at "error" and the existing diagnostics are suppressed one site at a time instead of downgrading a rule and hiding every future error of that kind
Plan
Stage 1: Parity (done, #4207)
- Scope both checkers to the same files:
files = ["pyiceberg"]for mypy,src.include = ["pyiceberg"]for ty, both at Python 3.11 - Give each ty rule that replaces a mypy flag the same level mypy used, and name the flag next to it
- Run ty in its own pre-commit environment, which holds only ty and the four stub packages, so both checkers see the same third-party types. ty picks the environment up from the
VIRTUAL_ENVthe hook runner exports - Report ty on every pull request without blocking: the
typecheck-tyjob setscontinue-on-errorand stays out ofpython-ci-required - Add
make typecheck-ty. The hook is manual-stage, somake lintdoes not run it - Suppress the 470 diagnostics in place, one
# ty: ignore[rule]per site, rather than a ratchet that downgrades the 12 rules
Stage 2: Drop mypy
- Keep
tests/out of the ty scope for now. It adds 403 diagnostics, but the two biggest families are the ones the package already carries:invalid-argument-type120 andunresolved-attribute116. Sampling them showsBytesIOagainst theOutputStreamprotocol, which is the same finding aspyiceberg/avro/file.py:260, and attribute access onX | Noneunions, which is the same as the package's 53unresolved-attributesites. So this is package debt at test sites, not mock noise, and it is deferred rather than relaxed. I looked at ten other projects for comparison, five of the most-starred Python ones and five ASF ones: seven either leavetests/out of the checker or include it with the expensive checks switched off for test modules, and none checks tests at full strictness, so no established strategy exists to copy here - Add the three files outside the package that mypy checked before stage 1:
setup.py,dev/provision.pyandmkdocs/gen_doc_stubs.py - Set
analysis.respect-type-ignore-comments = false. The 121# type: ignorecomments inpyiceberg/hide 65 diagnostics, so fix those or convert each comment to a# ty: ignore[rule]first
- Add Ruff's
ANN0andANN2rules withflake8-annotations.mypy-init-return = true. They flag three functions today, and they are the three that mypy already exempts:expressions/literals.py:90andcli/console.py:44carry# type: ignore[no-untyped-def],utils/deprecated.py:31carries a bare# type: ignore. So this converts three suppressions rather than annotating new code.deprecatedneedsParamSpec - Decide the
implicit_reexportgap, or accept losing the check - Remove the six rule deltas from the ty hook, which exist only because the stub environment cannot see the real libraries
- Make ty a required check: drop
continue-on-error, add the job topython-ci-required, and sync.asf.yaml. Theif: github.event_name == 'pull_request'guard has to go first, becausepython-ci-requiredalso runs onmerge_groupand a job that is skipped there fails its every-result-is-success loop - Delete the mypy hook with its stub packages,
[tool.mypy], themypypin and the mypy mentions inAGENTS.md. Keep themypy-boto3-*packages, becausecatalog/glue.pyandcatalog/dynamodb.pyimport them for type checking
Stage 3: Fix one suppressed rule at a time
Pick a rule, delete its suppressions, fix the code, and leave the rule at "error". One rule per pull request. unused-ignore-comment already fails when a site is fixed and its comment stays behind, so nothing can be forgotten
| Rule | Sites |
|---|---|
invalid-method-override |
104 |
missing-type-argument |
93 |
invalid-argument-type |
75 |
unresolved-attribute |
53 |
invalid-return-type |
44 |
no-matching-overload |
36 |
invalid-assignment |
12 |
unsupported-operator |
5 |
missing-argument |
2 |
invalid-type-form |
1 |
not-subscriptable |
1 |
call-non-callable |
1 |
possibly-missing-submodule (warns) |
1 |
pydantic-discarded-extra-argument (warns) |
1 |
The 12 error-level rules are suppressed on 429 sites across 403 comments, because 26 comments name two rules. Taking invalid-method-override first clears 104 of them and needs nothing from the libraries, which makes it the cheapest rule to start with
- Replace the 56 bare
# type: ignorecomments. Fix the code, or use# type: ignore[code] # ty: ignore[rule]. In 17 of them,as_boundreturnstype[...]in a class that has a field namedtype.builtins.type[...]fixes these in both checkers - Then bring
tests/in with path-level relaxation, not another directive wave:[[tool.ty.overrides]]withinclude = ["tests"]and[tool.ty.overrides.rules], removing one rule per pull request. The 56# type: ignorecomments intests/that ty already reports as unused can go in one mechanical commit - Delete
redundant-cast,redundant-condition, the two unused-ignore rules and themissing-override-decorator = "ignore"line from the config once nothing there needs watching
Stage 4: Raise strictness
Apply the configuration ty recommends for strict mode, and fix what it reports
[tool.ty.rules]
blanket-ignore-comment = "error"
dynamic-function-decorator-return = "error"
missing-type-argument = "error"
possibly-unresolved-reference = "warn"
unsound-assignment = "error"
unsound-return-statement = "error"
unsound-yield = "error"
unsupported-dynamic-base = "warn"
# NOTE: the following rules are known to have a significant number of false positives,
# which is mostly unavoidable. Enable them at your own risk!
division-by-zero = "warn"
possibly-missing-attribute = "warn"
possibly-missing-import = "warn"
[tool.ty.analysis]
strict-equality-semantics = true
strict-generic-narrowing = true
[tool.ruff.lint]
extend-select = ["ANN", "PYI", "PGH003"]
preview = true
-
redundant-condition-strictreports 16,unsound-assignment21,dynamic-function-decorator-return50 andunsound-return-statement175 diagnostics inpyiceberg/. Turn each one on only after its diagnostics are fixed -
possibly-unresolved-reference,possibly-missing-attribute,possibly-missing-importandunsound-yieldreport nothing inpyiceberg/today
- Add
@overridewhen you change an override, then turn onmissing-override-decorator, which reports 1,033 diagnostics
Done when
- ty is the required check for
pyiceberg/and the three files outside it.tests/joins once stage 3 has cleared the package - No suppression comments are left,
respect-type-ignore-commentsisfalseandblanket-ignore-commentis on - The stage 4 configuration is in place, and
error-on-warningis still at its default - mypy, its configuration and its CI step are gone
Decisions needed
- ty has no rule that matches mypy's
implicit_reexport = false. Can we accept losing that check? It is what makes mypy report an import as a missing name unless the module re-exports it explicitly - Should mypy keep its current 214-file scope in this PR? Tradeoff: parity is easier to reason about, but narrowing it silently drops
tests/ from the only check that runs today - Delete mypy before or after the suppressions are cleared? Tradeoff: one checker sooner for contributors, versus a longer safety net
cc @rambleraptor @kevinjqliu
- Vorherrschende Sprache
- Python
- Sterne
- 1.2k
- Forks
- 654
- Ø Merge
- 2 T. 11 Std.
- Gemergte PRs (30 T.)
- 122
Entwicklungsumgebung
- Kein Dockerfile und keine Docker-Compose-Datei
- Hat eine Pull-Request-Vorlage
- Kein Beitragsleitfaden
Erste Schritte
- Lesen Sie das ganze Issue und danach den Beitragsleitfaden des Projekts.
- Schreiben Sie ins Issue, dass Sie es übernehmen — das erspart doppelte Arbeit.
- Forken Sie das Repository und arbeiten Sie in einem Branch.
- Öffnen Sie einen Pull Request, der die Issue-Nummer nennt.
Mehr aus apache/iceberg-python
-
`inspect.files()` and the other file metadata tables raise `ArrowInvalid` when a snapshot has no manifestsEvtl. vergeben @YuangGao hat das vor 10 Tagen übernommen. Offenkind:bug
Schwierigkeit 2/5 1-3 Stunden Anfängerfreundlichkeit 72/100
apache/iceberg-python#4269 ·
Maintainer antworten meist innerhalb von 1 Tag
-
`inspect.entries()` raises TypeError when a data file has no column_sizesEvtl. vergeben @Rahul-cs50 hat das vor 14 Tagen übernommen. Offenkind:bug
Schwierigkeit 2/5 1-3 Stunden Anfängerfreundlichkeit 78/100
apache/iceberg-python#4186 · 2 Kommentare ·
Maintainer antworten meist innerhalb von 1 Tag
-
`older_than` reads a naive datetime as UTC and expires snapshots it should keepEvtl. vergeben @Rahul-cs50 hat das vor 13 Tagen übernommen. Offenkind:bug
Schwierigkeit 2/5 1-3 Stunden Anfängerfreundlichkeit 72/100
apache/iceberg-python#4182 · 1 Kommentar ·
Maintainer antworten meist innerhalb von 1 Tag
-
REST: Send empty first-page pageToken when rest-page-size is setEvtl. vergeben @ehddnr301 hat das vor 32 Tagen übernommen. Offenkind:bug
Schwierigkeit 2/5 1-3 Stunden Anfängerfreundlichkeit 82/100
apache/iceberg-python#4096 ·
Maintainer antworten meist innerhalb von 1 Tag
-
PyArrowFileIO: every small S3 write is a 3-request multipart upload; expose allow_delayed_openEvtl. vergeben @David-Banquet hat das vor 29 Tagen übernommen. Offen
Schwierigkeit 2/5 1-3 Stunden Anfängerfreundlichkeit 70/100
apache/iceberg-python#4093 · 3 Kommentare ·
Maintainer antworten meist innerhalb von 1 Tag
Alle Issues in apache/iceberg-python
Ähnliche Issues
-
bug
Schwierigkeit 2/5 1-3 Stunden Anfängerfreundlichkeit 80/100
glossyio/traffic-monitor#312 ·
Maintainer antworten meist innerhalb von 1 Tag
-
local devenv exampleOffendocumentation enhancement
Schwierigkeit 2/5 1-3 Stunden Anfängerfreundlichkeit 76/100
es-ude/elastic-ai.creator#1046 ·
Maintainer antworten meist innerhalb von 3 Tagen
-
feature request
Schwierigkeit 2/5 1-3 Stunden Anfängerfreundlichkeit 65/100
Maintainer antworten meist innerhalb von 1 Tag
-
Schwierigkeit 2/5 1-3 Stunden Anfängerfreundlichkeit 72/100
confident-ai/deepteam#306 ·
Maintainer antworten meist innerhalb von 1 Tag
-
SslLibrary crashes with unhandled OSError: port/proto not found on non-standard SSL portsEvtl. vergeben @himanchalkaushale hat das heute übernommen. Offen
Schwierigkeit 2/5 1-3 Stunden Anfängerfreundlichkeit 82/100
OWASP/Nettacker#1884 · 1 Kommentar ·
Maintainer antworten meist innerhalb von 1 Tag