-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add stubtest to CI for type annotation validation #11124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
293b7e2
0b7df6d
d25b867
9d802ce
9c9a4ab
3aab2c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -188,6 +188,52 @@ jobs: | |
| name: codecov-umbrella | ||
| fail_ci_if_error: false | ||
|
|
||
| stubtest: | ||
| name: Stubtest | ||
| runs-on: "ubuntu-latest" | ||
| needs: [detect-ci-trigger, cache-pixi-lock] | ||
| # Phase 1: Non-blocking (informational only) | ||
| # Change to 'false' once stubtest is stable to make it required | ||
| continue-on-error: true | ||
| defaults: | ||
| run: | ||
| shell: bash -l {0} | ||
| env: | ||
| PIXI_ENV: test-py313-with-typing | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Restore cached pixi lockfile | ||
| uses: Parcels-code/pixi-lock/restore@9a2866f8258b87a3c616d5ad7d51c6cd853df78b | ||
| with: | ||
| cache-key: ${{ needs.cache-pixi-lock.outputs.cache-key }} | ||
| - uses: prefix-dev/setup-pixi@v0.9.4 | ||
| with: | ||
| pixi-version: ${{ needs.cache-pixi-lock.outputs.pixi-version }} | ||
| cache: true | ||
| environments: ${{ env.PIXI_ENV }} | ||
| cache-write: ${{ github.event_name == 'push' && github.ref_name == 'main' }} | ||
|
|
||
| - name: Version info | ||
| run: | | ||
| pixi run -e ${{env.PIXI_ENV}} -- python xarray/util/print_versions.py | ||
|
|
||
| - name: Install type stubs | ||
| run: | | ||
| pixi run -e ${{env.PIXI_ENV}} -- python -m mypy --install-types --non-interactive xarray/ || true | ||
|
|
||
| - name: Run stubtest (core modules) | ||
| run: | | ||
| pixi run -e ${{env.PIXI_ENV}} -- python -m mypy.stubtest \ | ||
| xarray.core.dataarray \ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason why not run against full
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The earlier version ran against more modules, but some of the source code fixes needed to pass stubtest caused mypy to fail — the two tools have opposing perspectives on the same annotations. Rather than debugging those conflicts across the whole codebase in one PR, I scoped it down to the three core modules as pure infrastructure (CI job + allowlist, no source changes) to avoid that tension entirely. Running against full xarray produces ~6,600 errors, which would need a much larger allowlist. Happy to expand coverage incrementally in follow-ups. |
||
| xarray.core.dataset \ | ||
| xarray.core.variable \ | ||
| --mypy-config-file pyproject.toml \ | ||
| --allowlist _stubtest/allowlist.txt | ||
|
|
||
| pyright: | ||
| name: Pyright | ${{ matrix.pixi-env }}" | ||
| runs-on: "ubuntu-latest" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| # Stubtest Allowlist for xarray | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see a lot of stuff here that is typing only. I am actually not sure what the best way forward is here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the review! You're exactly right — stubtest was designed for separate .pyi stub files where type-check-only symbols simply don't appear in the stubs. With inline annotations, every TYPE_CHECKING import gets flagged because it exists in the source but not at runtime. That accounts for ~60% of the allowlist. The earlier version of this PR tried to address some of these by modifying source code (removing obsolete type: ignore comments, adjusting signatures), but some of those fixes caused mypy to fail — the two tools pull annotations in opposite directions. stubtest wants them to match runtime, mypy wants internal consistency, and in xarray's architecture with heavy TYPE_CHECKING blocks those goals conflict. I scoped it down to pure infrastructure (zero source changes) so the tools can't interfere with each other, which is why the allowlist is larger than ideal. That said, each allowlist entry documents an intentional decision, and the categorized regex patterns should make it straightforward to chip away at entries in follow-up PRs as the typing infrastructure evolves. |
||
| # ================================ | ||
| # This file contains patterns for symbols that stubtest reports as errors | ||
| # but are working correctly at runtime. | ||
| # | ||
| # Format: Each line is a regex pattern matching fully qualified names to ignore. | ||
| # See: https://mypy.readthedocs.io/en/stable/stubtest.html#allowlist | ||
|
|
||
| # ============================================================================= | ||
| # typing module re-exports (metaclass/signature differences) | ||
| # ============================================================================= | ||
|
|
||
| # Any has metaclass differences between stub and runtime | ||
| xarray\.core\.(dataarray|dataset|variable)\.Any$ | ||
| xarray\.core\.(dataarray|dataset|variable)\.Any\.__new__$ | ||
|
|
||
| # Callable also has metaclass differences | ||
| xarray\.core\.(dataarray|dataset|variable)\.Callable$ | ||
|
|
||
| # Self is a TYPE_CHECKING import | ||
| xarray\.core\.(dataarray|variable)\.Self$ | ||
|
|
||
| # TypeVar has __mro_entries__ at runtime but not in stubs | ||
| xarray\.core\.dataarray\.TypeVar\.__mro_entries__$ | ||
|
|
||
| # ============================================================================= | ||
| # TYPE_CHECKING imports - not present at runtime | ||
| # ============================================================================= | ||
|
|
||
| # Type aliases from xarray.core.types | ||
| xarray\.core\.(dataarray|dataset|variable)\.Dims$ | ||
| xarray\.core\.(dataarray|dataset)\.DatetimeLike$ | ||
| xarray\.core\.(dataarray|dataset)\.DatetimeUnitOptions$ | ||
| xarray\.core\.(dataarray|dataset)\.ErrorOptions$ | ||
| xarray\.core\.(dataarray|dataset|variable)\.ErrorOptionsWithWarn$ | ||
| xarray\.core\.(dataarray|dataset)\.GroupIndices$ | ||
| xarray\.core\.(dataarray|dataset)\.GroupInput$ | ||
| xarray\.core\.(dataarray|dataset)\.Grouper$ | ||
| xarray\.core\.(dataarray|dataset)\.InterpOptions$ | ||
| xarray\.core\.(dataarray|dataset)\.NetcdfWriteModes$ | ||
| xarray\.core\.(dataarray|dataset|variable)\.PadModeOptions$ | ||
| xarray\.core\.(dataarray|dataset|variable)\.PadReflectOptions$ | ||
| xarray\.core\.(dataarray|dataset|variable)\.QuantileMethods$ | ||
| xarray\.core\.(dataarray|dataset)\.QueryEngineOptions$ | ||
| xarray\.core\.(dataarray|dataset)\.QueryParserOptions$ | ||
| xarray\.core\.(dataarray|dataset)\.ReindexMethodOptions$ | ||
| xarray\.core\.(dataarray|dataset)\.ResampleCompatible$ | ||
| xarray\.core\.(dataarray|dataset)\.Resampler$ | ||
| xarray\.core\.(dataarray|dataset)\.SideOptions$ | ||
| xarray\.core\.(dataarray|dataset)\.CoarsenBoundaryOptions$ | ||
| xarray\.core\.(dataarray|dataset)\.ZarrWriteModes$ | ||
| xarray\.core\.(dataarray|dataset)\.ZarrStore$ | ||
| xarray\.core\.(dataarray|dataset)\.ZarrStoreLike$ | ||
| xarray\.core\.(dataarray|dataset)\.ArrayLike$ | ||
| xarray\.core\.dataset\.CFCalendar$ | ||
| xarray\.core\.dataset\.CombineAttrsOptions$ | ||
| xarray\.core\.dataset\.CompatOptions$ | ||
| xarray\.core\.dataset\.JoinOptions$ | ||
| xarray\.core\.dataset\.CoercibleMapping$ | ||
| xarray\.core\.dataset\.CoercibleValue$ | ||
| xarray\.core\.dataset\.DataVars$ | ||
| xarray\.core\.dataset\.DsCompatible$ | ||
|
|
||
| # TypeVars | ||
| xarray\.core\.(dataarray|dataset)\.T_ChunkDimFreq$ | ||
| xarray\.core\.(dataarray|dataset)\.T_ChunksFreq$ | ||
| xarray\.core\.(dataarray|dataset|variable)\.T_Chunks$ | ||
| xarray\.core\.(dataarray|dataset)\.T_NetcdfEngine$ | ||
| xarray\.core\.(dataarray|dataset)\.T_NetcdfTypes$ | ||
| xarray\.core\.(dataarray|dataset)\.T_Xarray$ | ||
| xarray\.core\.dataarray\.T_XarrayOther$ | ||
| xarray\.core\.dataset\.T_DatasetPadConstantValues$ | ||
| xarray\.core\.variable\.T_DuckArray$ | ||
| xarray\.core\.variable\.T_VarPadConstantValues$ | ||
|
|
||
| # External library types (dask, delayed, etc.) | ||
| xarray\.core\.(dataarray|dataset|variable)\.ChunkManagerEntrypoint$ | ||
| xarray\.core\.(dataarray|dataset)\.DaskDataFrame$ | ||
| xarray\.core\.(dataarray|dataset)\.Delayed$ | ||
| xarray\.core\.dataset\.AbstractDataStore$ | ||
| xarray\.core\.dataarray\.iris_Cube$ | ||
|
|
||
| # DataArray TYPE_CHECKING import in dataset module | ||
| xarray\.core\.dataset\.DataArray$ | ||
|
|
||
| # NamedArray TYPE_CHECKING import | ||
| xarray\.core\.variable\.NamedArray$ | ||
|
|
||
| # ============================================================================= | ||
| # GroupBy/Rolling/Coarsen/Weighted/Resample classes (TYPE_CHECKING imports) | ||
| # ============================================================================= | ||
|
|
||
| xarray\.core\.dataarray\.DataArrayGroupBy$ | ||
| xarray\.core\.dataarray\.DataArrayCoarsen$ | ||
| xarray\.core\.dataarray\.DataArrayRolling$ | ||
| xarray\.core\.dataarray\.DataArrayWeighted$ | ||
| xarray\.core\.dataarray\.DataArrayResample$ | ||
| xarray\.core\.dataset\.DatasetGroupBy$ | ||
| xarray\.core\.dataset\.DatasetCoarsen$ | ||
| xarray\.core\.dataset\.DatasetRolling$ | ||
| xarray\.core\.dataset\.DatasetWeighted$ | ||
| xarray\.core\.dataset\.DatasetResample$ | ||
|
|
||
| # ============================================================================= | ||
| # CFTimeIndex properties - read-only at runtime | ||
| # ============================================================================= | ||
|
|
||
| xarray\.core\.(dataarray|dataset)\.CFTimeIndex\.date_type$ | ||
| xarray\.core\.(dataarray|dataset)\.CFTimeIndex\.day$ | ||
| xarray\.core\.(dataarray|dataset)\.CFTimeIndex\.dayofweek$ | ||
| xarray\.core\.(dataarray|dataset)\.CFTimeIndex\.dayofyear$ | ||
| xarray\.core\.(dataarray|dataset)\.CFTimeIndex\.days_in_month$ | ||
| xarray\.core\.(dataarray|dataset)\.CFTimeIndex\.hour$ | ||
| xarray\.core\.(dataarray|dataset)\.CFTimeIndex\.microsecond$ | ||
| xarray\.core\.(dataarray|dataset)\.CFTimeIndex\.minute$ | ||
| xarray\.core\.(dataarray|dataset)\.CFTimeIndex\.month$ | ||
| xarray\.core\.(dataarray|dataset)\.CFTimeIndex\.second$ | ||
| xarray\.core\.(dataarray|dataset)\.CFTimeIndex\.year$ | ||
|
|
||
| # ============================================================================= | ||
| # Plot accessors and methods | ||
| # ============================================================================= | ||
|
|
||
| xarray\.core\.dataarray\.DataArray\.plot$ | ||
| xarray\.core\.(dataarray|dataset)\.Dataset\.plot$ | ||
| xarray\.core\.dataarray\.DataArrayPlotAccessor\.(contour|contourf|imshow|line|pcolormesh|scatter|step|surface)$ | ||
| xarray\.core\.dataset\.DatasetPlotAccessor\.(quiver|scatter|streamplot)$ | ||
|
|
||
| # ============================================================================= | ||
| # __array__ method - complex numpy protocol | ||
| # ============================================================================= | ||
|
|
||
| xarray\.core\.(dataarray|dataset)\.Dataset\.__array__$ | ||
|
|
||
| # ============================================================================= | ||
| # Mapping/MutableMapping/Sequence/Iterable protocol methods | ||
| # ============================================================================= | ||
|
|
||
| xarray\.core\.(dataarray|dataset|variable)\.Mapping\.get$ | ||
| xarray\.core\.(dataarray|dataset|variable)\.Mapping\.__reversed__$ | ||
| xarray\.core\.(dataarray|dataset|variable)\.Sequence\.index$ | ||
| xarray\.core\.(dataarray|dataset)\.Iterable\.__class_getitem__$ | ||
| xarray\.core\.(dataarray|dataset)\.MutableMapping\.(pop|setdefault)$ | ||
| xarray\.core\.(dataarray|dataset)\.PathLike\.__class_getitem__$ | ||
| xarray\.core\.dataset\.FrozenMappingWarningOnValuesAccess\.get$ | ||
|
|
||
| # Number.__hash__ | ||
| xarray\.core\.dataset\.Number\.__hash__$ | ||
|
|
||
| # ============================================================================= | ||
| # IO protocol (typing.IO) | ||
| # ============================================================================= | ||
|
|
||
| xarray\.core\.dataset\.IO\.(closed|mode|name|read|readline|readlines|seek|truncate|write|writelines|__iter__|__next__)$ | ||
|
|
||
| # ============================================================================= | ||
| # Variable/IndexVariable methods inherited from numpy-like interface | ||
| # ============================================================================= | ||
|
|
||
| xarray\.core\.(dataarray|dataset|variable)\.Variable\.(item|searchsorted)$ | ||
| xarray\.core\.(dataarray|dataset|variable)\.IndexVariable\.(item|searchsorted)$ | ||
| xarray\.core\.dataarray\.DataArray\.(item|searchsorted)$ | ||
| xarray\.core\.dataarray\.DataArrayArithmetic\.(item|searchsorted)$ | ||
| xarray\.core\.variable\.VariableArithmetic\.(item|searchsorted)$ | ||
|
|
||
| # ============================================================================= | ||
| # Subclass pattern for Variable | ||
| # ============================================================================= | ||
|
|
||
| xarray\.core\.variable\.<subclass.* | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That feels a bit wacky. I always wanted to add the stubs dependencies to the environments directly for better transparency.
But since the mypy job does this as well, I don't see a better way as well.
Let's keep this for the future.