GeezerVison is showing github.com — Add type annotations to functions? · Issue #179 · python-graphblas/python-graphblas
Add type annotations to functions? #179DescriptionThis would probably be straightforward to do. For functions that accept matrix expressions and can auto-compute, I would probably want a MatrixType that is a union of e.g. Matrix, TransposedMatrix, MatrixInfixExpr, MatrixExpression, and MatrixIndexExpr.
If we add type annotations, I would want to run mypy in CI.
What do y'all think? I wouldn't mind seeing this added, but I'm unlikely to add it any time soon. Activityeriknw commented on Mar 18, 2022 Actually, I think this would be really great to have. I'd even be in favor of adding type annotations to class attributes where it makes sense. Having type annotations would probably be helpful for the long tail of maintenance and to help new contributors (heh, or returning contributors) figure out what's going on. I wouldn't be surprised if it caught a bug or two too.
So, big +1 from me. Any volunteers? Reacted by Sultan Orazbayev SultanOrazbayev commented on Nov 1, 2022 I can work on this, however some initial hand-holding might be needed. I will join tomorrow's community call to see if this will work. eriknw commented on Nov 1, 2022 eriknw commented on Nov 3, 2022 Alright, I think the best way to begin adding type annotations is very gradually with generics (and without running mypy).
For example, we can make types such as:
from typing import TypeVar
ScalarLike = TypeVar("ScalarLike") # Scalar, ScalarExpression, int, float, list, dict, etc.
VectorLike = TypeVar("VectorLike") # Vector, VectorExpression, etc.
MatrixLike = TypeVar("MatrixLike") # Matrix, TransposedMatrix, MatrixExpression, etc.and use these as input types.
As @jim22k suggested today, it's probably best to begin with user-facing objects Scalar, Vector, and Matrix. We should be able to handle most return types such as VectorExpression. In core/vector.py, we may need to define MatrixExpression as a generic type for use with Vector.outer. For now, inline type annotations are for readers, not for type checkers.
To make type checkers happy, I think we'll want to add .pyi stub files that can be exact and use the correct types. Mypy is able to generate stubs via stubgen graphblas. I would wait to do this though. The mypy documentation appears pretty good at a glance.
I think the eventual target is to run checks with mypy, maybe pyright (to make sure VSCode is happy), and maybe pydantic. It'll be non-trivial to get there, but I think it would improve the code base. Reacted by Sultan Orazbayev MridulS commented on Nov 3, 2022
and without running mypy
Any specific reason for this? Would it be better to first add mypy to the CI? We can still "typecheck" without having any types. eriknw commented on Nov 3, 2022 @MridulS, I didn't know you know about type-checking--this is great!
Yeah, if we can figure out how to run mypy even with no type annotations, then we probably should.
My naive attempt, mypy graphblas, showed a lot of errors, but I'm new to mypy and am probably doing things wrong. SultanOrazbayev commented on Nov 7, 2022 Checking the docs on using mypy with an existing codebase and experimenting a bit locally with mypy, there are at least a couple of options:
- Setup a CI so that it checks only a specific file where the type annotation work will be done first and then gradually expand the range of files. Roughly:
mypy --ignore-missing-imports --follow-imports=skip graphblas/core/matrix.py This will currently run without any errors (without any type definitions), so should be a good test for checking validity of introduced type hints.
- Setup a CI to check all files, but suppress the non-critical errors via setup a mypy.ini with the following contents:
[mypy]
ignore_missing_imports = True
ignore_errors = True I think first option is a bit more explicit in terms of coverage, so if I start working on https://github.com/python-graphblas/python-graphblas/blob/main/graphblas/core/matrix.py that could be the first file to check. eriknw commented on Nov 7, 2022 Very nice!
This shows how much I know, which isn't much :-P. I'm happy to follow what you think is best here @SultanOrazbayev. I think I'm going to learn a lot from this effort.
Do you think we'll ever need to "cheat" by e.g. adding graphblas/core/typing.py like so:
from typing import TypeVar, Union
Matrix = TypeVar("Matrix")
TransposedMatrix = TypeVar("TransposedMatrix")
MatrixInfixExpr = TypeVar("MatrixInfixExpr")
MatrixExpression = TypeVar("MatrixExpression")
MatrixIndexExpr = TypeVar("MatrixIndexExpr")
# MatrixLike can be used as function input argument type
MatrixLike = Union[Matrix, TransposedMatrix, MatrixInfixExpr, MatrixExpression, MatrixIndexExpr]Reacted by Sultan Orazbayev SultanOrazbayev commented on Nov 8, 2022 Not sure if I understand correctly, but if you are suggesting that we collect all the typing definitions in one file, that makes a lot of sense. I started placing typing definitions in the same file, but realize this won't work as the same definitions will need to be reused in other files.
In terms on naming, will the suggested path graphblas/core/typing.py cause import conflicts due to naming? e.g. within graphblas/core/*py scripts, when I run from typing import TypeVar, isn't it going to use graphblas/core/typing.py (instead of the built-in typing)? eriknw commented on Nov 8, 2022 I think we should try to use the actual types in type annotations. I worry that this will be difficult or virtually impossible due to how interdependent everything is and how limited forward declaration of types may be.
When using the real type is not feasible, I think it's reasonable to use a fake type (using TypeVar) of the same name instead. Again, I'm not an expert here, and I wouldn't be surprised if mypy can work around my concerns. But, in the spirit of "gradual typing", I'm okay with adding type annotations even if some are only for humans and currently worthless for type validation tools. That is, I would defer "let's figure out how to use actual types" until later if it's really hard. Maybe we'll need to use stub files later on.
Since Python 3, import typing and from typing import TypeVar will always refer to the builtin module (i.e., it's absolute, not relative), and e.g. from .typing import foo is always relative in the current package. Reacted by Sultan Orazbayev eriknw commented on Mar 30, 2023 The discussions in these links seem informative and helpful:
Reacted by Sultan Orazbayev eriknw commented on Apr 6, 2023 MonkeyType is an interesting and potentially useful tool for creating stub files and adding annotations:
https://github.com/Instagram/MonkeyType
Example usage:
$ monkeytype run -m pytest
$ monkeytype stub graphblas.core.matrix
It still needs a human touch, but it appears to be a useful tool to get things started. I don't know if it can create "modern" annotations such as not using Optional, Union, or List, but there's proabably a way to do that.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|