Skip to content
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

Corrections and enchancements to type stubs #1349

Merged
merged 83 commits into from
Oct 22, 2024

Conversation

RandallPittmanOrSt
Copy link
Contributor

@RandallPittmanOrSt RandallPittmanOrSt commented Jul 12, 2024

This PR includes some corrections and enhancements to the type stubs provided in #1302. Some test cases and examples were modified during the process of testing the stubs on them with mypy.

Enhancements

  • Reorganized all the type literals into a meaningful hierarchy.
  • Variable.dtype and Variable.datatype properties are overloaded via faux descriptor class workaround (ref. these mypy and pyright issues). This is the only way we can actually get correct types out of these in static analysis.
  • mypy checks in CI have been made more exhaustive by using --check-untyped-defs. (Some nuisance errors had to be silenced with the --allow-redefinition flag and some # type: ignore comments.)
  • Allow more verbosity in test/run_all.py

Fixes

  • To stubs:
    • All function/method arguments that accept literals also need to accept the base types (e.g. str or int). It may be argued that this renders inclusion of the type literals ('u2', 'i4', etc.) moot but they are left in as they can be useful in IDE pop-up documentation.
    • Move dtype_is_complex into __init__.pyi. It is publicly accessible in netCDF4 due to import of __all__ from netCDF4._netCDF4.
    • The GetSetItemKey for type alias __getitem__ and __setitem__ needs to also accept float (might be a mathematical integer) and str (might be convertible to integer) and also needs separate list[] generics defined for each possible type, rather than list[int | bool | etc...] due to lists be invariant and mutable. EDIT: We decided to accept anything for key.
    • Dimension can be created with float size if it's a mathematical integer.
    • zlib, shuffle, fletcher32, and contiguous arguments for creating a variable don't have to be bool, just "truthy", so they are changed to Any.
    • Variable typing and overloading
      • The TypeVar for Variable (now called VarT) has been changed to be unbound. The types in the old bound= argument weren't really correct nor useful.
      • There are some overloads for Variable.__new__ (and createVariable) where if a variable is created using a NumPy type or str as the datatype, then the type of the variable can be known.
      • The type of a variable created with a datatype other than str or a NumPy type cannot be defined statically. In the future, we might be able to make CompoundType, EnumType, and/or VLType generic in which case we could possibly use those specified generics as types for Variable.
    • When creating an EnumType, the enum_dictarg has been changed toMappinginstead ofdict`... This helps with [co/in]variance issues.
    • Dataset.__dealloc__ is a Cython-only method and not publically accessible, so it was removed from the stubs.
  • Various minor fixes made to test cases based on new mypy runs
    • test_enum.EnumDictTestCase wasn't called due to bad indentation of methods. Fixed some issues with that test case.
    • Use ma.masked_array instead of ma.core.MaskedArray. NumPy has gone back and forth with MaskedArray being part of ma.core or just ma depending on NumPy version. ma.masked_array appears to be a reliable alias in all cases.
    • Add some isinstance assertions for type narrowing
    • Add a couple of assertions where something wasn't actually being tested
      • test_masked.py - around line 96
      • test_multifile.py - around line 77
    • Rename some variables where the same name was being used with different meanings in adjacent contexts. (test_multifile.py)

"Cosmetic" changes

Future possible changes

It might be better to allow any string or int where we're requiring Literal arguments in functions, while still providing the Literals for auto-completion tooling and documentation. Many of the modifications to the test cases could be reverted if we did this.

@CLAassistant
Copy link

CLAassistant commented Jul 12, 2024

CLA assistant check
All committers have signed the CLA.

@RandallPittmanOrSt
Copy link
Contributor Author

I've decided that running mypy against the existing tests is good enough for now (not going to write more typing-focused tests).

@RandallPittmanOrSt RandallPittmanOrSt marked this pull request as ready for review July 31, 2024 19:06
@RandallPittmanOrSt
Copy link
Contributor Author

I haven't had time to try to set up the whole CI environment on my own machine but I don't think the failures are caused by this PR.

@jswhit
Copy link
Collaborator

jswhit commented Aug 22, 2024

yes, the mpi test failure is due an update in the conda mpi package - nothing to do with this PR. Just awaiting @headtr1ck's review now.

Copy link
Contributor

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some good improvements, but I got some questions :)

test/test_compression_blosc.py Outdated Show resolved Hide resolved
test/test_compression_bzip2.py Outdated Show resolved Hide resolved
examples/mpi_example.py Outdated Show resolved Hide resolved
src/netCDF4/__init__.pyi Show resolved Hide resolved
src/netCDF4/__init__.pyi Outdated Show resolved Hide resolved
src/netCDF4/__init__.pyi Outdated Show resolved Hide resolved
src/netCDF4/__init__.pyi Show resolved Hide resolved
src/netCDF4/__init__.pyi Outdated Show resolved Hide resolved
src/netCDF4/__init__.pyi Show resolved Hide resolved
src/netCDF4/__init__.pyi Outdated Show resolved Hide resolved
@jswhit
Copy link
Collaborator

jswhit commented Sep 22, 2024

@headtr1ck are you ok with this PR now?

@jswhit
Copy link
Collaborator

jswhit commented Oct 19, 2024

Would like to make a 1.7.2 release this week, and include this if possible. @headtr1ck could you please complete your review?

Copy link
Contributor

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the changed test files this looks good!
Sorry for the delay in review :)

src/netCDF4/__init__.pyi Outdated Show resolved Hide resolved
src/netCDF4/__init__.pyi Outdated Show resolved Hide resolved
examples/parallel_test.nc Outdated Show resolved Hide resolved
test/test_compression.py Show resolved Hide resolved
test/test_enum.py Show resolved Hide resolved
@headtr1ck
Copy link
Contributor

I guess we have to solve some merge conflicts.
The test failures seems quite random and not related.

@headtr1ck
Copy link
Contributor

@jswhit looks good now!
Thanks!

@jswhit
Copy link
Collaborator

jswhit commented Oct 22, 2024

@RandallPittmanOrSt if you update your fork to the latest develop I can merge

@RandallPittmanOrSt
Copy link
Contributor Author

@jswhit Done.

@jswhit
Copy link
Collaborator

jswhit commented Oct 22, 2024

OK thanks @RandallPittmanOrSt - merging now

@jswhit jswhit merged commit 14012a5 into Unidata:master Oct 22, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants