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

Typing: visit not re-exported and partial/missing annotations #1352

Open
barakatzir opened this issue Jan 5, 2025 · 5 comments · May be fixed by #1353
Open

Typing: visit not re-exported and partial/missing annotations #1352

barakatzir opened this issue Jan 5, 2025 · 5 comments · May be fixed by #1353
Labels
bug Something isn't working

Comments

@barakatzir
Copy link

barakatzir commented Jan 5, 2025

Information

I found two issues will using rustworkx.visit:

  1. re-export of visit modules currently doesn't work for mypy for some reason
  2. there are several partial & missing type annotations remaining in rustworkx.

More info:

  1. The following python code doesn't pass mypy:

    from __future__ import annotations
    import rustworkx as rx
    from typing import override
    
    rx.visit  # error: Module "rustworkx" does not explicitly export attribute "visit"  [attr-defined]
    
    # error: Class cannot subclass "BFSVisitor" (has type "Any")  [misc]
    # error: Name "rx.visit.BFSVisitor" is not defined  [name-defined]
    class MyVistor(rx.visit.BFSVisitor):
        # error: Method "discover_vertex" is marked as an override, but no base method was found with this name  [misc]
        @override
        def discover_vertex(self, v: int) -> None:
            return

    To solve this I suggest replacing in rustworkx/__init__.pyi the line import rustworkx.visit as visit with from . import visit as visit. This seems to fix the re-export locally for me. After the fix I only get the expected error: Missing type parameters for generic type "BFSVisitor" [type-arg] from running mypy on the script above (this type error is unavoidable until Allow some custom return types to be annotated as generic classes at runtime  #1349 is addressed).

  2. Looking at visit.pyi I noticed that most of the methods do not have return type (should be -> None). This raised a flag for me since I expected rustworkx's tests to catch partial typing. It seems that stubtest does not test for partial type hints - I tried running it and it did complain about several __all__ attributes missing in stubs but nothing else (rustworkx.generators.__all__, rustworkx.rustworkx.__all__, rustworkx.visualization.graphviz.__all__ and rustworkx.visualization.matplotlib.__all__).

    However, if I run mypy --strict -p rustworkx I get a bunch of missing generic's parameters and some other type annotations. Either this or pyright --verifytypes rustworkx catches also a lot of errors. I suggest adding one of these or both as a test(s) to rustworkx (and fixing the errors they emit).

  • rustworkx version: 0.15.1
  • Python version: 3.13
  • Operating system: ubuntu 24

What is the current behavior?

  1. re-export of visit modules currently doesn't work for mypy for some reason
  2. there are several partial & missing type annotations remaining in rustworkx.

Steps to reproduce the problem

See above.


If my suggestions above sound good to you, I'm willing to open a PR and try implementing them.


PS, I really am using BFSVisitor, so this isn't just a continuation of my previous issue regarding generics.
Thanks for responding and addressing, all my type-related issues so far. I know it might be petty of me since it doesn't change the runtime behavior by much, but I love using rustworkx and I love having my code type-checked.

@barakatzir barakatzir added the bug Something isn't working label Jan 5, 2025
@barakatzir barakatzir changed the title Typing issue in rustworkx.visit Typing: visit not re-exported and partial/missing annotations Jan 5, 2025
@IvanIsCoding
Copy link
Collaborator

IvanIsCoding commented Jan 5, 2025

I will post multiple comments but first things first.The missing re-export is a mistake. I will accept PRs to fix it or I will send one myself later. Notice that it is very easy to miss this kind of thing unfortunately, import rustworkx.visit is a statement you'd think would work, see #960 for some previously related discussions.

Also, thanks for continuously testing our library! I appreciate the issues. I know Qiskit uses the visit module however it doesn’t consider type annotations. You might be the first user that is actually checking them with mypy

@IvanIsCoding
Copy link
Collaborator

My second comment is regarding testing the stubs. This was surprisingly difficult in #401, you might find a couple commits trying to use https://github.com/davidfritzsche/pytest-mypy-testing which is the best tooling I found for the task.

Most codebases organically get a good check because they run mypy or pyright to check the implementations themselves. However, for us running mypy on the code does very little as the implementation is in Rust. Another alternative would be to run mypy in the test cases, but at the time this did not yield as good results as I thought. I tried using pytest-mypy-testing as the next best thing, but simple mypy updates broke the assertions so that was abandoned.

The mypy stub testing is the only tool that I found that gives a decent reminder about type annotations on CI. It is of course incomplete: it does not check the types of the input, the return type, or generics (see #1340 for an example). However, it does check that every method is annotated which is a good start to prevent us sliding away from annotations.

I am open to adding pyright --verifytypes rustworkx to our CI to check the stubs if the feedback it returns is both useful and stable.

@IvanIsCoding
Copy link
Collaborator

Lastly, visit.pyi has methods that are extermely flexible. I think because only the input is specificied in the visitor pattern, they should return Any.

I assume when I originally wrote the annotations the implicit Any would be enough. But I know that might not be true with all type checkers in strict settings, it reminds me of #1242 and #1246

@IvanIsCoding IvanIsCoding linked a pull request Jan 5, 2025 that will close this issue
@barakatzir
Copy link
Author

barakatzir commented Jan 6, 2025

Qiskit not considering type annotations is another woe of mine.

Regarding testing that the runtime types match the type annotations, I stumbled upon three types of tests:

  1. The most common for extension modules is mypy's stubtest
  2. the next best thing is have some python test cases type-check successfully, these test cases can double as python test cases that are run with a python interpreter.
  3. Lastly, python test-cases that fail type check or include reveal_type and verify that the type checker ouput is as expected. I suspect that it's best to rely only on mypy/pyright's error codes and not the error messages since they are not guaranteed to be stable. This approach can be further matched against the runtime's reveal_type output (added to stdlib typing in python 3.11), but I haven't seen tests with such further checks.

An example using all these approaches is numpy: https://github.com/numpy/numpy/blob/main/numpy/typing/tests/test_typing.py

What I had in mind is something different. Simply running mypy or pyright on rustworkx, as I suggested, will only type check the 'pyi' stubs and the few 'py' files it has. This will not guarantee mtaching of type annotations with types at runtime, but will ensure that the annotations are consistent and complete.
Most of the error codes that pyright and mypy emitted were regarding missing type parameters for generics, so many will probably be fixed by PR #1246.

@barakatzir
Copy link
Author

barakatzir commented Jan 6, 2025

I ran mypy on the Specify-Any-for-PyGraph/PyDiGraph branch for PR #1246.
It found few enough errors to list them all here (mypy --strict rustworkx with mypy 1.14.1):

...
rustworkx/rustworkx.pyi:208: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:216: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:597: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:604: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:865: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:871: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:894: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:901: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:909: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:916: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:976: error: Missing type parameters for generic type "BFSVisitor"  [type-arg]
rustworkx/rustworkx.pyi:977: error: Missing type parameters for generic type "DFSVisitor"  [type-arg]
rustworkx/rustworkx.pyi:978: error: Missing type parameters for generic type "DijkstraVisitor"  [type-arg]
rustworkx/rustworkx.pyi:1081: error: Missing type parameters for generic type "dtype"  [type-arg]
rustworkx/rustworkx.pyi:1081: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:1237: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:1241: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:1315: error: Function is missing a type annotation for one or more arguments  [no-untyped-def]
rustworkx/rustworkx.pyi:1400: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:1404: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/rustworkx.pyi:1516: error: Function is missing a type annotation for one or more arguments  [no-untyped-def]
rustworkx/__init__.pyi:281: error: Missing type parameters for generic type "BFSVisitor"  [type-arg]
rustworkx/__init__.pyi:282: error: Missing type parameters for generic type "DFSVisitor"  [type-arg]
rustworkx/__init__.pyi:283: error: Missing type parameters for generic type "DijkstraVisitor"  [type-arg]
rustworkx/__init__.pyi:292: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/__init__.pyi:303: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/__init__.pyi:322: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/__init__.pyi:328: error: Missing type parameters for generic type "ndarray"  [type-arg]
rustworkx/__init__.pyi:460: error: Function is missing a type annotation for one or more arguments  [no-untyped-def]
rustworkx/__init__.pyi:595: error: Function is missing a type annotation for one or more arguments  [no-untyped-def]

It also found the missing return types in visit.pyi, but those are fixed in PR #1353 (I replaced them with ...).

These are still mostly missing generic's parameters, but much less than before. This also shows that the different ...Visitor classes also need either explicit parameter types with Any or have a set default in TypeVar.
I think that the numpy.dtype and numpy.ndarray should probably be set with Any-s (or something else if appropriate) instead of the implicit Any. Notice that dtype and ndarray missing type parameters can also leak via pyright: if someone has the pyrightconfig.json set with "typeCheckingMode": "standard", then pyright/pylance will emit an error. In VSCode:

from __future__ import annotations
import rustworkx as rx

graph: rx.PyDiGraph[float, float] = rx.PyDiGraph()
dist_arr = rx.distance_matrix(graph, 1, False, float('nan')) # Type of "dist_arr" is "ndarray[Unknown, Unknown]" Pylance(reportUnknownVariableType)

The other few missing type annotations are:

  • in rustworkx/rustworkx.pyi:1315 the PyGraph.__setstate__ method has missing annotation for state.
  • in rustworkx/rustworkx.pyi:1516 the PyDiGraph.__setstate__ method has missing annotation for state.
  • in rustworkx/__init__.pyi:460 the function bipartite_layout has missing annotations for first_nodes parameter.
  • in rustworkx/__init__.pyi:595 the function bellman_ford_shortest_paths has missing annotations for source parameter.

The last two should definitely be fixed, while the first two will probably not be encountered since it's not common to explicitly call __setstate__ (but I do think they also deserve type annotations).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants