rustworkx icon indicating copy to clipboard operation
rustworkx copied to clipboard

Add annotation stubs for `PyGraph` and `PyDiGraph`

Open IvanIsCoding opened this issue 4 years ago • 6 comments

Related to #352

This PR adds type annotation stubs, it contains .pyi files with type annotations. I started by annotating the PyGraph and PyDiGraph files, as well as the custom types.

I tried to follow the PEP 561 convetion: we include .pyi files in the same directory as our Python code. Note that this is a first step in annotations: they are partial, we can add more as we go

We also test the annotations using pytest-mypy-testing. I had to add a new Tox environment to keep them separate from the unittests we have. The tests are under the tests/stubs directory.

Some items we need to handle because the PR has been open for long:

  • [ ] Add PyGraph and PyDiGraph methods that were added after the PR was created
  • [ ] Add custom return types that were added after the PR was created

IvanIsCoding avatar Aug 03 '21 02:08 IvanIsCoding

Pull Request Test Coverage Report for Build 2778787088

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.008%) to 97.14%

Totals Coverage Status
Change from base Build 2778191284: 0.008%
Covered Lines: 12499
Relevant Lines: 12867

💛 - Coveralls

coveralls avatar Aug 03 '21 06:08 coveralls

I need to verify the newly added functions to PyGraph and PyDiGraph, and also check if there were updates to function signaturs. But this PR is ready to review

IvanIsCoding avatar Oct 18 '21 20:10 IvanIsCoding

I've no power on retworkx - can someone please assign me to this so I don't forget to check it?

jakelishman avatar Dec 07 '21 17:12 jakelishman

I'm generally in favour of libraries providing type hints if at all possible - it's hopefully not too much work, and lets downstream users make the choice over whether or not to use them.

I think there's a couple of problems with the typing here, though, some of which seem to stem from design decisions in PyDiGraph and PyGraph. The edge data is inherently nullable, but the type hints here don't account for that. This means that the types are inconsistent, and all graph type hints should effectively be PyGraph[S, Union[T, None]]. You don't want users to do that, so any PyGraph function that returns type T should instead return Union[T, None]. Obviously this means that it'll be harder for users to write compliant code, but that's actually a function of the internal typing of PyGraph, and technically it's required whether or not the type hint says that. (See also: why everyone hates nullable types!)

I am back on this and I think the solution is along the lines of restricting the methods you outlined.

So basically what we want is to never allows users to do this:

graph: PyGraph[str, int] = PyGraph()
node_a: int = graph.add_node("A")
node_b: int = graph.add_node("B")
graph.add_edges_from_no_data([(node_a, node_b)]) # mypy catches this error

But let users do this:

graph: PyGraph[str, Optional[int]] = PyGraph()
node_a: int = graph.add_node("A")
node_b: int = graph.add_node("B")
graph.add_edges_from_no_data([(node_a, node_b)]) # mypy should let this pass

That way we do not need to make users handle None unless they chose to

IvanIsCoding avatar May 06 '22 01:05 IvanIsCoding

I rewrote the custom return types annotations from zero. Because they are generated from the same Rust macro, I created an abstract class representing each macro and inherited from them. It shortens the annotations from that section considerably.

I also made the types covariant to handle some issues pointed earlier.

IvanIsCoding avatar May 06 '22 23:05 IvanIsCoding

Pull Request Test Coverage Report for Build 4378374086

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 97.145%

Files with Coverage Reduction New Missed Lines %
src/shortest_path/all_pairs_dijkstra.rs 1 99.27%
<!-- Total: 1
Totals Coverage Status
Change from base Build 4376765708: 0.03%
Covered Lines: 14053
Relevant Lines: 14466

💛 - Coveralls

coveralls avatar Sep 18 '22 02:09 coveralls

Alright, this is ready for review after so long. mypy.stubtest checks that the stubs in this PR are identical to the text_signature from our Rust code and that the return types are valid.

Some things might be missing (e.g. all functions from rustworkx.rustworkx), but this is a good start to support annotations

IvanIsCoding avatar Feb 02 '23 00:02 IvanIsCoding