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

Add annotation stubs for PyGraph and PyDiGraph #401

Merged
merged 101 commits into from
Mar 9, 2023

Conversation

IvanIsCoding
Copy link
Collaborator

@IvanIsCoding IvanIsCoding commented Aug 3, 2021

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 mypy.stubtest. This was the most reliable approach. For now, we ignore missing stubs and focus only on the functions imported from Rust code

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

  • Add custom return types that were added after the PR was created

@coveralls
Copy link

coveralls commented Aug 3, 2021

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

@@ -1066,7 +1066,7 @@ impl PyDiGraph {
/// ``(source, target, weight)`` where source and target are integer
/// node indices. If the node index is not present in the graph
/// nodes will be added (with a node weight of ``None``) to that index.
#[pyo3(text_signature = "(self, edge_lsit, /)")]
#[pyo3(text_signature = "(self, edge_list, /)")]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FWIW, this typo has been on our code base for 2 years. mypy caught it

tox.ini Outdated
basepython = python3
envdir = .tox/stubs
deps =
mypy @ git+https://github.com/python/mypy.git
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to keep using the latest version until python/mypy#14041 is out

Copy link
Member

Choose a reason for hiding this comment

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

I think this was released as of Mypy 1.0 a couple of weeks ago.

@IvanIsCoding
Copy link
Collaborator Author

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

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

This looks good to merge to me! Fantastic work, Ivan - I know I'm not one of the actual retworkx devs so it's a bit weird coming from me, but still!

I left a question or two inline (mostly for future reference), and maybe one place where's the possibility for marginally better typing, but nothing that actually needs action on this PR. I'd recommend to merge it now before it gets out-of-date again - it's been a great effort since the correct typing here is quite complex with the extra constraints on the Self type, and some of the additional co-/contra-/in- variant typing.

.github/workflows/main.yml Show resolved Hide resolved
Comment on lines +34 to +38
def add_edges_from(
self,
obj_list: Sequence[Tuple[int, int, T]],
/,
) -> List[int]: ...
Copy link
Member

Choose a reason for hiding this comment

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

I was a little surprised to learn that PyO3 won't convert Iterable as a valid conversion type for Vec<_>, but you're definitely correct with the hint here. I guess maybe PyO3 didn't want to do conversions that require multiple reallocations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PyO3 has some weird quirks with iterations (e.g. #614 that broke us) so sometimes I just accept the Vec<_> conversion and if it is not in our tests it is not "canonical"

Comment on lines +79 to +81
def from_adjacency_matrix(
matrix: np.ndarray, /, null_value: float = ...
) -> PyDiGraph[int, float]: ...
Copy link
Member

Choose a reason for hiding this comment

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

Numpy technically provide stronger type hints now, and Unpack+TypeVarTuple give some sort of way of specifying ndarray shapes / dtypes. Unless you particularly want to look into that right now, though, I'd not worry about it in favour of just landing this PR.

Comment on lines +139 to +146
def to_dot(
self,
/,
node_attr: Optional[Callable[[S], Dict[str, str]]] = ...,
edge_attr: Optional[Callable[[T], Dict[str, str]]] = ...,
graph_attr: Optional[Dict[str, str]] = ...,
filename: Optional[str] = ...,
) -> Optional[str]: ...
Copy link
Member

Choose a reason for hiding this comment

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

Again, this could be done later if you'd rather just land the PR:

I think there's a way to teach type checkers that the return type is dependent on the types of one of the arguments. It's made more complex because of the default value of the argument, so I don't precisely know the correct syntax, but it's something like

@typing.overload
def to_dot(self, /, ..., filename: None = ...) -> str: ...
@typing.overload
def to_dot(self, /, ..., filename: str = ...) -> None: ...
def to_dot(self, /, ..., filename = None): ...

I don't remember exactly how you sort out the syntax in a typing stub to combine the overload with the default value, but it's something along those lines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like this, I will try implementing it

rustworkx/iterators.pyi Show resolved Hide resolved
rustworkx/py.typed Show resolved Hide resolved
tox.ini Outdated
basepython = python3
envdir = .tox/stubs
deps =
mypy @ git+https://github.com/python/mypy.git
Copy link
Member

Choose a reason for hiding this comment

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

I think this was released as of Mypy 1.0 a couple of weeks ago.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks for adding this and sticking with it so long! It will be great to finally merge this as the longest open PR we've ever had in rustworkx :)

@mtreinish mtreinish added the automerge Queue a approved PR for merging label Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants