Add type annotations to functions?

This 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.

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?

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.

@SultanOrazbayev

Thanks @SultanOrazbayev, I'm delighted to hear this and look forward to seeing you tomorrow!

We'll be happy to hand-hold to get you started. I've been trying (and failing) to work on the contributor guidelines document for a few months now (my bad!), so the weekly call and joining us on discord and/or slack for async chatting are the best ways to get started and ask questions.

Discord (Community Packages / #graphblas): https://discord.com/invite/vur45CbwMz
Slack (#python-bindings channel): https://join.slack.com/t/thegraphblas/shared_invite/zt-1iatqh6eq-39HFogw1U8kKRKo2YQOFzQ

I prefer Discord and would like to move away from Slack, but I suspect we'll be at both for a while.

@jim22k

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.

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.

@MridulS

@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.

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.

@SultanOrazbayev

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]

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)?

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.

The discussions in these links seem informative and helpful:

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.

Navigation Menu

Repository navigation

Add type annotations to functions? #179

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Add type annotations to functions? #179

Description

Activity

eriknw commented on Mar 18, 2022

SultanOrazbayev commented on Nov 1, 2022

eriknw commented on Nov 1, 2022

eriknw commented on Nov 3, 2022

MridulS commented on Nov 3, 2022

eriknw commented on Nov 3, 2022

SultanOrazbayev commented on Nov 7, 2022

eriknw commented on Nov 7, 2022

SultanOrazbayev commented on Nov 8, 2022

eriknw commented on Nov 8, 2022

eriknw commented on Mar 30, 2023

eriknw commented on Apr 6, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions