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

NodeIndices does not implement sequence protocol #696

Closed
polwel opened this issue Oct 10, 2022 · 3 comments · Fixed by #730
Closed

NodeIndices does not implement sequence protocol #696

polwel opened this issue Oct 10, 2022 · 3 comments · Fixed by #730
Assignees
Labels
bug Something isn't working stable-backport-potential This PR or issue is potentially worth backporting for inclusion in a stable branch

Comments

@polwel
Copy link

polwel commented Oct 10, 2022

Information

  • rustworkx version: rustworkx-0.12.0-cp39-cp39-win_amd64.whl from PyPI
  • Python version: 3.9.12
  • Rust version: n/a
  • Operating system: Windows 10

What is the current behavior?

Minimal example:

>>> import rustworkx as rx
>>> graph = rx.generators.directed_path_graph(5)
>>> reversed(rx.topological_sort(graph))
Traceback (most recent call last):
  File "C:\Users\polw\venv-dev\lib\site-packages\IPython\core\interactiveshell.py", line 3378, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-4-c1f474a6ec26>", line 1, in <module>
    reversed(rx.topological_sort(graph))
TypeError: rustworkx.NodeIndices is not a sequence

From Python, I can use both len() and indexing on rustworkx.NodeIndices. So that works.

What is the expected behavior?

No error :)

reversed consumes sequences, and NodeIndices is documented to implement the sequence protocol.

Steps to reproduce the problem

See above.

@polwel polwel added the bug Something isn't working label Oct 10, 2022
@polwel
Copy link
Author

polwel commented Oct 10, 2022

FWIW, on python 3.7 it fails with a different message: TypeError: object of type 'rustworkx.NodeIndices' has no len(). Haven't tested other versions.

On both systems retworkx 0.11 worked fine.

@mtreinish mtreinish added the stable-backport-potential This PR or issue is potentially worth backporting for inclusion in a stable branch label Oct 10, 2022
@mtreinish
Copy link
Member

Sigh I thought pyo3 fixed all these compatibility issues (we hit similar things earlier in the cycle after upgrading our pyo3 version). Thanks for reporting this I'll see if we're missing something on the newer pyo3 version and get it to act like a sequence properly again, and in the worst case we can just downgrade pyo3 to a version that is doing the correct thing.

@mtreinish mtreinish self-assigned this Oct 10, 2022
@IvanIsCoding
Copy link
Collaborator

We need to add tests to cover some behaviors and expected use cases of the custom return types, it's similar to #614 that we took some PyO3 "magic" for granted and they changed the API

tuxu added a commit to tuxu/rustworkx that referenced this issue Nov 7, 2022
Add the `sequence` option to PyO3's `pyclass`, so that the `sq_length`
slot is implemented [1]. Implementing this method is required for
sequence types, or Python C API functions like `PySequence_Size` will
fail with an error.

The `reversed` built-in method relies on `PySequence_*` methods. A test
reversing `NodeIndices` is added to guard against future violations of
the sequence protocol.

Fixes Qiskit#696.

[1]: PyO3/pyo3#2567
@mergify mergify bot closed this as completed in #730 Nov 9, 2022
mergify bot pushed a commit that referenced this issue Nov 9, 2022
* Fix support of sequence protocol for returned lists

Add the `sequence` option to PyO3's `pyclass`, so that the `sq_length`
slot is implemented [1]. Implementing this method is required for
sequence types, or Python C API functions like `PySequence_Size` will
fail with an error.

The `reversed` built-in method relies on `PySequence_*` methods. A test
reversing `NodeIndices` is added to guard against future violations of
the sequence protocol.

Fixes #696.

[1]: PyO3/pyo3#2567

* Add release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
mergify bot pushed a commit that referenced this issue Nov 9, 2022
* Fix support of sequence protocol for returned lists

Add the `sequence` option to PyO3's `pyclass`, so that the `sq_length`
slot is implemented [1]. Implementing this method is required for
sequence types, or Python C API functions like `PySequence_Size` will
fail with an error.

The `reversed` built-in method relies on `PySequence_*` methods. A test
reversing `NodeIndices` is added to guard against future violations of
the sequence protocol.

Fixes #696.

[1]: PyO3/pyo3#2567

* Add release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
(cherry picked from commit 4592bc0)
mergify bot added a commit that referenced this issue Nov 9, 2022
…735)

* Bump pyo3 from 0.17.2 to 0.17.3 (#723)

Bumps [pyo3](https://github.com/pyo3/pyo3) from 0.17.2 to 0.17.3.
- [Release notes](https://github.com/pyo3/pyo3/releases)
- [Changelog](https://github.com/PyO3/pyo3/blob/main/CHANGELOG.md)
- [Commits](PyO3/pyo3@v0.17.2...v0.17.3)

---
updated-dependencies:
- dependency-name: pyo3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
(cherry picked from commit d91f488)

# Conflicts:
#	Cargo.lock
#	Cargo.toml

* Fix support of sequence protocol for returned lists (#730)

* Fix support of sequence protocol for returned lists

Add the `sequence` option to PyO3's `pyclass`, so that the `sq_length`
slot is implemented [1]. Implementing this method is required for
sequence types, or Python C API functions like `PySequence_Size` will
fail with an error.

The `reversed` built-in method relies on `PySequence_*` methods. A test
reversing `NodeIndices` is added to guard against future violations of
the sequence protocol.

Fixes #696.

[1]: PyO3/pyo3#2567

* Add release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
(cherry picked from commit 4592bc0)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tino Wagner <ich@tinowagner.com>
Co-authored-by: Ivan Carvalho <ivancarv@student.ubc.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stable-backport-potential This PR or issue is potentially worth backporting for inclusion in a stable branch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants