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

Regression: instruction label must now be a string #12581

Closed
garrison opened this issue Jun 14, 2024 · 6 comments
Closed

Regression: instruction label must now be a string #12581

garrison opened this issue Jun 14, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@garrison
Copy link
Member

Environment

  • Qiskit version: main
  • Python version:
  • Operating system:

What is happening?

I am witnessing a CI failure in the circuit-knitting-toolbox that very likely has something to do with the merging of #12459. (We run CI against Qiskit main on a cron job every day.)

Here is one such error:

    def _split_barriers(circuit: QuantumCircuit):
        """Mutate an input circuit to split barriers into single qubit barriers."""
        for i, inst in enumerate(circuit):
            num_qubits = len(inst.qubits)
            if num_qubits == 1 or inst.operation.name != "barrier":
                continue
            barrier_uuid = uuid4()
    
            # Replace the N-qubit barrier with a single-qubit barrier
>           circuit.data[i] = CircuitInstruction(
                Barrier(1, label=barrier_uuid), qubits=[inst.qubits[0]]
            )
E           TypeError: 'UUID' object cannot be converted to 'PyString'

How can we reproduce the issue?

from uuid import uuid4
from qiskit.circuit import Barrier, CircuitInstruction

CircuitInstruction(Barrier(1, label=uuid4()), (0,))

What should happen?

It should work like before.

Any suggestions?

It is probably sufficient to call str(...) on any object before passing to Rust, and to add a test.

@garrison garrison added the bug Something isn't working label Jun 14, 2024
@jakelishman
Copy link
Member

An instruction label should always have been a string, and certainly for Barrier it's been documented that way since 2022.

@jakelishman
Copy link
Member

(btw, you're quite likely to see a lot more problems in the near future, including performance and memory regressions on main while we make large-scale changes to the internals. Everything should be fast and memory-efficient by the time of 1.2, but the intermediate states will very likely temporarily regress.)

@garrison
Copy link
Member Author

garrison commented Jun 14, 2024

An instruction label should always have been a string, and certainly for Barrier it's been documented that way since 2022.

Thanks for the reply, Jake. We can work around this pretty easily in ckt, obviously, and this is probably something I should have noticed in code review, except I was unfamiliar with uuid4() so didn't realize it returns an object, not a string. The reason I raised an issue here is because I suspect there is a chance this specific change may break other code in the wild, so perhaps it is worth converting the label to a str, and making a DeprecationWarning if you really don't want users to ever do it. But it's up to you as Qiskit maintainers. I am just trying to do my part to contain the fallout across the ecosystem from these fundamental changes.

@garrison
Copy link
Member Author

Jake, it looks like if qiskit were modified as I suggest to convert the label to str, that would still have broken us, since ckt later checks isinstance(label, UUID). So I think you have it correct right now: the code I provided should just lead to an error. I am going to close this as "won't fix."

@garrison garrison closed this as not planned Won't fix, can't repro, duplicate, stale Jun 14, 2024
@garrison
Copy link
Member Author

(Also, sorry I was in such a rush to open this issue. I wanted to do so before I forgot, but it turns out my mental model of what was happening was wrong still.)

garrison added a commit to Qiskit/qiskit-addon-cutting that referenced this issue Jun 15, 2024
* transforms.py: Convert the barrier uuid label into a string

More info at Qiskit/qiskit#12581

* Fix `isinstance(label, UUID)` check
mergify bot pushed a commit to Qiskit/qiskit-addon-cutting that referenced this issue Jun 15, 2024
* transforms.py: Convert the barrier uuid label into a string

More info at Qiskit/qiskit#12581

* Fix `isinstance(label, UUID)` check

(cherry picked from commit 654f02f)
garrison added a commit to Qiskit/qiskit-addon-cutting that referenced this issue Jun 15, 2024
…627)

* transforms.py: Convert the barrier uuid label into a string

More info at Qiskit/qiskit#12581

* Fix `isinstance(label, UUID)` check

(cherry picked from commit 654f02f)

Co-authored-by: Jim Garrison <garrison@ibm.com>
@jakelishman
Copy link
Member

Sorry, I dropped responding to this at the time.

I am just trying to do my part to contain the fallout across the ecosystem from these fundamental changes.

I meant to say: thank you very much for this - it is super helpful. Sorry for being curt before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants