oresttokovenko opened a new issue, #4271: URL: https://github.com/apache/iceberg-python/issues/4271
### 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 <!-- markdown-link-check-disable-next-line --> - ty includes a [language server](https://docs.astral.sh/ty/features/language-server/), so editors show the same diagnostics (errors and warnings) as CI - Its [type system](https://docs.astral.sh/ty/features/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 <!-- markdown-link-check-disable-next-line --> [#3964](https://github.com/apache/iceberg-python/pull/3964) added the ty configuration, and [#3965](https://github.com/apache/iceberg-python/pull/3965) fixed Pydantic subclass constructors and test annotations, which cut the count from 3,547 to 655 diagnostics at the time. [#4207](https://github.com/apache/iceberg-python/pull/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 <!-- markdown-link-check-disable-next-line --> ### Stage 1: Parity (done, [#4207](https://github.com/apache/iceberg-python/pull/4207)) - [x] Scope both checkers to the same files: `files = ["pyiceberg"]` for mypy, `src.include = ["pyiceberg"]` for ty, both at Python 3.11 - [x] Give each ty rule that replaces a mypy flag the same level mypy used, and name the flag next to it - [x] 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_ENV` the hook runner exports - [x] Report ty on every pull request without blocking: the `typecheck-ty` job sets `continue-on-error` and stays out of `python-ci-required` - [x] Add `make typecheck-ty`. The hook is manual-stage, so `make lint` does not run it - [x] 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 - [x] 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-type` 120 and `unresolved-attribute` 116. Sampling them shows `BytesIO` against the `OutputStream` protocol, which is the same finding as `pyiceberg/avro/file.py:260`, and attribute access on `X | None` unions, which is the same as the package's 53 `unresolved-attribute` sites. 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 leave `tests/` 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.py` and `mkdocs/gen_doc_stubs.py` - [ ] Set `analysis.respect-type-ignore-comments = false`. The 121 `# type: ignore` comments in `pyiceberg/` hide 65 diagnostics, so fix those or convert each comment to a `# ty: ignore[rule]` first <!-- markdown-link-check-disable-next-line --> - [ ] Add Ruff's [`ANN0` and `ANN2` rules](https://docs.astral.sh/ruff/rules/#flake8-annotations-ann) with `flake8-annotations.mypy-init-return = true`. They flag three functions today, and they are the three that mypy already exempts: `expressions/literals.py:90` and `cli/console.py:44` carry `# type: ignore[no-untyped-def]`, `utils/deprecated.py:31` carries a bare `# type: ignore`. So this converts three suppressions rather than annotating new code. `deprecated` needs `ParamSpec` - [ ] Decide the `implicit_reexport` gap, 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 to `python-ci-required`, and sync `.asf.yaml`. The `if: github.event_name == 'pull_request'` guard has to go first, because `python-ci-required` also runs on `merge_group` and a job that is skipped there fails its every-result-is-success loop - [ ] Delete the mypy hook with its stub packages, `[tool.mypy]`, the `mypy` pin and the mypy mentions in `AGENTS.md`. Keep the `mypy-boto3-*` packages, because `catalog/glue.py` and `catalog/dynamodb.py` import 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: ignore` comments. Fix the code, or use `# type: ignore[code] # ty: ignore[rule]`. In 17 of them, `as_bound` returns `type[...]` in a class that has a field named `type`. `builtins.type[...]` fixes these in both checkers - [ ] Then bring `tests/` in with path-level relaxation, not another directive wave: `[[tool.ty.overrides]]` with `include = ["tests"]` and `[tool.ty.overrides.rules]`, removing one rule per pull request. The 56 `# type: ignore` comments in `tests/` that ty already reports as unused can go in one mechanical commit - [ ] Delete `redundant-cast`, `redundant-condition`, the two unused-ignore rules and the `missing-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 ```toml [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-strict` reports 16, `unsound-assignment` 21, `dynamic-function-decorator-return` 50 and `unsound-return-statement` 175 diagnostics in `pyiceberg/`. Turn each one on only after its diagnostics are fixed - [ ] `possibly-unresolved-reference`, `possibly-missing-attribute`, `possibly-missing-import` and `unsound-yield` report nothing in `pyiceberg/` today <!-- markdown-link-check-disable-next-line --> - [ ] Add [`@override`](https://docs.python.org/3/library/typing.html#typing.override) when you change an override, then turn on `missing-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-comments` is `false` and `blanket-ignore-comment` is on - [ ] The stage 4 configuration is in place, and `error-on-warning` is still at its default - [ ] mypy, its configuration and its CI step are gone ## Decisions needed <!-- markdown-link-check-disable-next-line --> - ty has no rule that matches mypy's [`implicit_reexport = false`](https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-no-implicit-reexport). 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
