search

Qiskit / rustworkx

A high performance Python graph library implemented in Rust.
https://www.rustworkx.org
Apache License 2.0
1.03k stars 145 forks source link

`bfs_search`, etc. have incorrect type stubs #1130

Closed p0llard closed 6 months ago

p0llard commented 6 months ago

https://github.com/Qiskit/rustworkx/blob/6e37baf11c0bda266ca6e95999d9d749f0d6204e/src/traversal/mod.rs#L424-L429 shows source is an Optional<Vec<usize>> but the Python type stubs show the binding taking a single int: https://github.com/Qiskit/rustworkx/blob/6e37baf11c0bda266ca6e95999d9d749f0d6204e/rustworkx/__init__.pyi#L552-L556

The examples in the documentation (correctly) show singleton lists being passed: https://github.com/Qiskit/rustworkx/blob/6e37baf11c0bda266ca6e95999d9d749f0d6204e/rustworkx/__init__.py#L1533

If a single int is passed, then

TypeError: argument 'source': 'int' object cannot be converted to 'Sequence'

is raised.

I'm not familar enough with PyO3 to know whether it would allow implicit conversions of any Iterable[int]; this page implies it needs to be a list, but the error message (which comes from PyO3) implies it might accept collections.abc.Sequence or something similar.

p0llard commented 6 months ago

I'm not familar enough with PyO3 to know whether it would allow implicit conversions of any Iterable[int]; this page implies it needs to be a list, but the error message (which comes from PyO3) implies it might accept collections.abc.Sequence or something similar.

I carried out a quick experiment, and it seems that arbitrary collections.abc.Sequence subclasses are indeed accepted, and work as expected.

IvanIsCoding commented 6 months ago

Because our library is written in Rust we had to manually add the type stubs.

Not only did I get bfs_search wrong, I also got dfs_search and dijkstra_search incorrectly as well. I think we can release a fix for this for sure in 0.15 and maybe it warrants a 0.14.2 release.

In the meantime you will need to ignore the annotation if you are using mypy/pyright/etc

JPena-code commented 6 months ago

Hi @IvanIsCoding. I would like to work on this issue, if possible can you assign it to me

IvanIsCoding commented 6 months ago

Hi @IvanIsCoding. I would like to work on this issue, if possible can you assign it to me

Sounds good. Check https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md. You will need to generate a release note + change the rustworkx.pyi + init.pyi files. If you include universal and type-specific functions for Dijkstra, BFS, and DFS they will be 9 in total

IvanIsCoding commented 6 months ago

I will try to target release the fix for this with the fix for https://github.com/Qiskit/rustworkx/issues/1117 in 0.14.2