add pyrefly type checking for new type errors - #2591
Conversation
pavelzw
changed the title
| environment-file: environment.yml | ||
| micromamba-version: 1.5.12-0 | ||
| cache-environment: true | ||
| cache-environment: false |
There was a problem hiding this comment.
cache-environment: true will give you a false sense of security since you don't use a lockfile.
| - setuptools>=80 | ||
| - setuptools_scm>=8.1 | ||
| - pre-commit | ||
| - pyrefly |
There was a problem hiding this comment.
this will lead to breakages with new pyrefly (or other dependencies') versions being released. my preferred workflow is having a lockfile to ensure CI is reproducibly green and then adding a regular lockfile updating job for pixi.lock as described in https://pixi.prefix.dev/latest/integration/ci/updates_github_actions/ and fix eventual failures here directly.
i strongly believe that this is a more seamless workflow and adding semver-style pins to everything and just running pixi upgrade which bumps those pins would make life simpler here.
happy to do introduce pixi here as well if this is wished but i don't want to impose my beliefs on other people (in this repository at least)
(i currently use pixi anyway on this repo with pixi.* and .pixi in .gitignore because i don't have conda/mamba installed on my system; having first party support for pixi in this repo would be nice anyway for people like me)
a pixi-based workflow would also make things like #2589 easier
| - setuptools>=80 | ||
| - setuptools_scm>=8.1 | ||
| - pre-commit | ||
| - pyrefly |
There was a problem hiding this comment.
other alternatives might be ty or mypy. i don't have strong opinions on this (apart from me wanting to not use mypy/pyright because they are slow) and don't know whether the python ecosystem will converge on ty or pyrefly or zuban. just using pyrefly because i've tried it out before and @ytausch likes it
There was a problem hiding this comment.
Some thoughts about mypy without having done any deep looking at the tradeoffs on this particular repo:
- correctness
- they all have false negatives/positives
- performance
mypyrecently added multi-core support, and is moving key pieces intorust- we (speaking as a member of @conda-forge/mypy) ship the
dmypyextra by default (e.g.psutil)
- we (speaking as a member of @conda-forge/mypy) ship the
- integration
- the editor plugins are fine
pyproject.tomlis a fine place to write[tool]config, this repo hardly needs more/.{whatever}files
- governance
mypyis an official (or at least blessed) python subproject vs a Big Corp project
There was a problem hiding this comment.
until mypy has support for a proper lsp and is on a similar level in terms of performance as ty/pyrefly, i personally will not care about mypy
There was a problem hiding this comment.
language server
👋 @conda-forge/mypy-ls
performance
🤷
$> time mypy -p conda_smithy
real 0m0.823s
user 0m2.251s
sys 0m0.383s$> time pyrefly conda_smithy
real 0m0.684s
user 0m1.207s
sys 0m0.148s$> time ty check conda_smithy
real 0m0.404s
user 0m1.330s
sys 0m0.149sThere was a problem hiding this comment.
https://github.com/python-lsp/pylsp-mypy does not look very "official (or at least blessed)" to me tbh; something directly part of mypy would be preferred by me
There was a problem hiding this comment.
this would allow us introducing pyrefly now without needing to fix 200 previous errors. would be nice to get rid of this file slowly through follow-ups
There was a problem hiding this comment.
summary of remaining errors by codex:
• There are 220 current Pyrefly type errors in the project without the baseline.
With .pyrefly-baseline.json, CI reports 0 errors, because those 220 existing diagnostics are baselined. The current breakdown is led by:
- missing-attribute: 69
- bad-argument-type: 27
- unsupported-operation: 19
- bad-override: 18
- bad-assignment: 18
- no-matching-overload: 14
Command used:
pixi run pyrefly check --python-interpreter-path $(pixi run which python) --output-format min-text --summary=none --progress-bar no --count-errors=0
There was a problem hiding this comment.
this would allow us introducing pyrefly now without needing to fix 200 previous errors.
Could we not do a couple small precursor PRs that tackle different classes of errors before we switch on pyrefly? I'm not opposed to start with an exclude list, but if we can reduce this a bit beforehand, all the better.
There was a problem hiding this comment.
also fine with me, i can see what my agent can do
There was a problem hiding this comment.
discussed a bit with my agent and we got the following plan:
🤖 yap
• Best grouping is by root cause/subsystem, not by Pyrefly error code. A single root cause often produces several codes, and fixing by code tends to create noisy cast-heavy PRs.
I’d split it like this:
PR 1: Obvious Runtime/Typing Fixes
Low-risk fixes that are either real bugs or tiny annotation mismatches.
Include:
- bootstrap-obvious-ci-and-miniconda.py: urllib.urlretrieve should be urllib.request.urlretrieve.
- simplejson.decoder.JSONDecodeError import/use.
- datetime(..., tzinfo=...) issue in deprecations.py.
- validate_schema.py optional schema_file typing.
- contextmanager generator return in tests.
- Simple uninitialized locals like old_val, runreqs_spacing, gh.
Goal: knock out easy errors without touching architecture.
PR 2: Linter Message Class Contracts
This is self-contained and has lots of override errors.
Include:
- conda_smithy/linter/messages/base.py
- conda_smithy/linter/messages/recipe.py
- conda_smithy/linter/messages/conda_forge.py
- related tests/registry typing
Fix pattern:
- Make base methods/properties have signatures that subclasses can legally override.
- Use ClassVar where class-level metadata is intended.
- Avoid mutable class attributes unless typed as ClassVar.
- Make render/documentation hooks return one consistent type.
This should clean up most bad-override, some bad-return, and not-callable.
PR 3: Pydantic Schema Typing
Keep conda_smithy/schema.py isolated. It has a distinct failure mode.
Include:
- model_config should likely be typed as ClassVar[ConfigDict].
- Dynamic create_model(...) calls need better field typing, probably local helper wrappers/casts.
- Functions returning generated models should return type[BaseModel], not BaseModel, if they return classes.
- Literal field defaults need narrowing or explicit casts.
Do not mix this with linter recipe fixes; it will be big enough on its own.
PR 4: Recipe YAML Shape Narrowing
This is probably the highest-value PR, but it needs care.
Include:
- conda_smithy/lint_recipe.py
- conda_smithy/linter/lints.py
- conda_smithy/linter/hints.py
- conda_smithy/linter/utils.py
- tests/test_lint_recipe.py
Fix pattern:
- Add small helper functions for dynamic recipe data:
- as_mapping(value) -> Mapping[str, Any] | None
- as_mutable_mapping(value) -> dict[str, Any]
- as_list_of_mappings(value) -> list[dict[str, Any]]
- as_string_list(value) -> list[str]
- Narrow before calling .get, .items, or linter helper functions.
- Prefer normalizing once near parse boundaries over sprinkling cast(...) everywhere.
This should eliminate many missing-attribute, bad-argument-type, and test fixture errors.
PR 5: Azure/GitHub Client Response Typing
This is mostly third-party client typing friction.
Include:
- conda_smithy/azure_ci_utils.py
- conda_smithy/anaconda_token_rotation.py
- conda_smithy/feedstock_tokens.py
- possibly conda_smithy/feedstock_io.py
Fix pattern:
- Add local typed wrappers around client calls.
- Use typing.cast(...) at API boundaries only.
- Add explicit None checks before indexing/attribute access.
- Avoid propagating object | Unknown | None deeper into the code.
This should clean up most Azure token/build-definition errors.
PR 6: PyGithub / External Library Stub Mismatches
Keep this separate because some fixes may be casts or ignores rather than code changes.
Include:
- conda_smithy/github.py
- conda_smithy/cirun_utils.py
- conda_smithy/feedstocks.py
- tests/test_feedstock_io.py
Fix pattern:
- If runtime behavior is correct but stubs are incomplete, use narrow cast(...) near the call site.
- If Pyrefly exposes a real bug, fix the call.
- Avoid adding broad ignores for whole files.
PR 7: Variant Algebra Type Cleanup
This is algorithmic and should be reviewed separately.
Include:
- conda_smithy/variant_algebra.py
- tests/test_variant_algebra.py
Fix pattern:
- Define aliases for variant values, e.g. VariantValue = str | float, VariantList = list[str] | list[float].
- Narrow before indexing.
- Split mixed numeric/string logic where Pyrefly cannot follow it.
PR 8: Remaining Tests-Only Cleanup
After production code is cleaner, handle test fixtures.
Include:
- tests/test_feedstock_tokens.py
- leftover tests/test_lint_recipe.py
- leftover registry/schema tests
Fix pattern:
- Annotate fixture return types.
- Cast exception messages after checking they are not None.
- Avoid intentionally heterogeneous test literals being inferred too broadly.
Recommended order: 1, 2, 3, 4, 5, 6, 7, 8.
The key rule: each PR should reduce one class of uncertainty. Avoid a “make Pyrefly green” mega-PR, and avoid adding broad suppressions early. Use casts mainly at external API boundaries and dynamic YAML boundaries; inside core logic, prefer real narrowing helpers and
clearer types.
PR 1 is #2593
| - pydantic>=2.11,<3 | ||
| - pixi>=0.59.0 | ||
| - jsonschema | ||
| - simplejson |
There was a problem hiding this comment.
|
I'd prefer for us to first draw down the set of existing errors to something like <20. |
|
I can start working on the next pr once #2593 is merged |
4 participants
as discussed in #2589
Closes #2602
Checklist
newsentry with any new deprecations added to theDeprecatedsection.python -m conda_smithy.schema)python -m conda_smithy.linter.messages)