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 networkx stubs #10544

Merged
merged 38 commits into from Dec 15, 2023
Merged

Add networkx stubs #10544

merged 38 commits into from Dec 15, 2023

Conversation

rhelmot
Copy link
Contributor

@rhelmot rhelmot commented Aug 8, 2023

networkx is a widely used library for graph analysis in scientific computing. It is a great candidate for type checking because it is absolutely full to the brim of container types. It is also unlikely to have types merged into upstream anytime soon. See discussion in the staging repository for this PR: fmagin/networkx-stubs#8

These stubs are by no means complete. They are complete for my personal use case (digraphs) and I spent about 3 hours trying to bang out correct stubs for everything else, but this library is gigantic. Everything I didn't get to remains the stubgen defaults. If this isn't the convention and I just just delete the unmodified stuff, please let me know!

The main concern I have is that this library has a few optional dependencies and has public interfaces which take types from these dependencies. These dependencies are declared as extras, so I believe the CI machinery here should be able to handle it, but some of them do not themselves have types available through any venue. I am not adding types to scipy. This triggers some type errors, and I don't know if those will be an issue. Please advise :)

cc @fmagin @Avasam

@github-actions

This comment has been minimized.

Avasam

This comment was marked as resolved.

@rhelmot
Copy link
Contributor Author

rhelmot commented Aug 31, 2023

I have attempted to fix all the issues!

One problem I ran into is that mypy is very unhappy with the way I tried to express a common pattern in networkx which would be a lot easier with higher-kinded types:

def builder_func(something: SomeCollection[T], create_using: Type[GenericGraph[T]]) -> GenericGraph[T]: ...

GenericGraph here should be a TypeVar bounded to Graph, but that's obviously not an option. Instead. I tried typing four overloads for each function like this, one for Graph and each of its subclasses, but that results in the admonition that the latter overloads will never be used because the first one is broader. How can I work around this?

Another overload-related problem that mypy complains about is the pattern of changing the return type based on a flag parameter, True to enable returning a specialized subclass instead of the simpler base case. For example:

    @overload
    def __call__(
        self,
        nbunch: NBunch[_Node] = ...,
        data: Literal[False] = ...,
        default: Incomplete = ...,
    ) -> OutEdgeView[_Node]: ...
    @overload
    def __call__(
        self,
        nbunch: NBunch[_Node] = ...,
        data: Literal[True] = ...,
        default: Incomplete = ...,
    ) -> OutEdgeDataView[_Node, tuple[_Node, _Node, dict[str, Incomplete]]]: ...
    @overload
    def __call__(
        self, nbunch: NBunch[_Node] = ..., data: str = ..., default: _U | None = None
    ) -> OutEdgeDataView[_Node, tuple[_Node, _Node, _U]]: ...

To me, this seems unambiguous. However, mypy is adamant that Overloaded function signatures 1 and 2 overlap with incompatible return types, ditto for 1 and 3. What can be done?

@github-actions

This comment has been minimized.

Avasam

This comment was marked as resolved.

@Avasam
Copy link
Sponsor Collaborator

Avasam commented Oct 18, 2023

@rhelmot Are you still interested in completing this PR? My suggestions above should get you pretty close. I am interested in seeing networkx stubs into typeshed and out of microsoft/python-type-stubs.

@rhelmot
Copy link
Contributor Author

rhelmot commented Oct 18, 2023

I am still interested, I just have a lot on my plate. If you'd like to push the project along, feel free to open a pull request to my fork.

rhelmot and others added 10 commits October 22, 2023 14:43
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@rhelmot
Copy link
Contributor Author

rhelmot commented Oct 23, 2023

I am trying to merge in the networkx 3.2 changes. this could take a moment.

@github-actions

This comment has been minimized.

@rhelmot
Copy link
Contributor Author

rhelmot commented Nov 14, 2023

I would say this is ready to merge even though it still targets an old version.

@srittau srittau requested a review from Avasam November 15, 2023 10:25
stubs/networkx/networkx/convert_matrix.pyi Outdated Show resolved Hide resolved
stubs/networkx/networkx/convert.pyi Outdated Show resolved Hide resolved
stubs/networkx/networkx/convert_matrix.pyi Outdated Show resolved Hide resolved
stubs/networkx/networkx/convert_matrix.pyi Outdated Show resolved Hide resolved
stubs/networkx/networkx/convert_matrix.pyi Outdated Show resolved Hide resolved
stubs/networkx/networkx/convert_matrix.pyi Outdated Show resolved Hide resolved
stubs/networkx/networkx/convert_matrix.pyi Outdated Show resolved Hide resolved
stubs/networkx/networkx/convert_matrix.pyi Outdated Show resolved Hide resolved
stubs/networkx/networkx/convert_matrix.pyi Outdated Show resolved Hide resolved
Avasam

This comment was marked as resolved.

Avasam

This comment was marked as resolved.

Avasam

This comment was marked as resolved.

@Avasam
Copy link
Sponsor Collaborator

Avasam commented Nov 28, 2023

@rhelmot Quick update, since I am now a Contributor to the typeshed project on GitHub, I can update your branch directly and apply the above suggestions. If that's something you'd like to save you some time.

I just have a lot on my plate. If you'd like to push the project along, feel free to open a pull request to my fork.

@rhelmot
Copy link
Contributor Author

rhelmot commented Nov 28, 2023

Yes, I would appreciate that, thank you!

This comment has been minimized.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Sponsor Collaborator

@Avasam Avasam left a comment

Choose a reason for hiding this comment

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

Thanks a lot @rhelmot for the migration from https://github.com/fmagin/networkx-stubs !

This PR is now in a state I'd feel confortable merging knowing that it is not meant to be a complete stub. Any non-obvious typing decision have been documented as comments. There's a substantial amount of manual typing in some modules, so I'll give a couple weeks for other maintainers to take a quick look, see if I missed anything.

Once merged, the next steps as far as I'm concerned would be:

@Avasam
Copy link
Sponsor Collaborator

Avasam commented Dec 15, 2023

It's been a couple weeks and I don't see any objection. In it goes! Thanks again @rhelmot . I'll probably take on parity with https://github.com/microsoft/python-type-stubs/tree/main/stubs/networkx next.

@Avasam Avasam changed the title Add networkx Add networkx stubs Dec 15, 2023
@Avasam Avasam merged commit 658dd55 into python:main Dec 15, 2023
48 checks passed
@Avasam Avasam mentioned this pull request Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants