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 Operation::blocks method. #13056

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Add Operation::blocks method. #13056

merged 3 commits into from
Sep 19, 2024

Conversation

kevinhartman
Copy link
Contributor

@kevinhartman kevinhartman commented Aug 29, 2024

Summary

This gives us a way to get the blocks of a control flow operation as an iterator of CircuitData. If called on a non-control-flow instruction, it returns an empty Vec.

Details and comments

This is not intended as a final API for this, which will likely instead return an iterator of &CircuitData once control flow operations have been fully ported to Rust and own their blocks without requiring conversion.

This gives us a way to get the blocks of a control flow
operation as an iterator of CircuitData. If called on
a non-control-flow instruction, returns None.

This is not intended as a final API for this, which
will likely instead return an iterator of &CircuitData
once control flow operations have been fully ported to
Rust and own their blocks without requiring conversion.
@kevinhartman kevinhartman requested a review from a team as a code owner August 29, 2024 20:22
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core
  • @kevinhartman
  • @mtreinish

@coveralls
Copy link

coveralls commented Aug 29, 2024

Pull Request Test Coverage Report for Build 10942872876

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 8 of 29 (27.59%) changed or added relevant lines in 2 files are covered.
  • 1890 unchanged lines in 64 files lost coverage.
  • Overall coverage decreased (-2.1%) to 87.14%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/packed_instruction.rs 0 3 0.0%
crates/circuit/src/operations.rs 8 26 30.77%
Files with Coverage Reduction New Missed Lines %
qiskit/quantum_info/operators/mixins/multiply.py 1 93.75%
qiskit/pulse/instructions/directives.py 1 97.22%
qiskit/synthesis/one_qubit/one_qubit_decompose.py 1 70.67%
qiskit/quantum_info/operators/mixins/group.py 1 92.59%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 1 88.61%
qiskit/quantum_info/operators/channel/quantum_channel.py 1 72.14%
qiskit/pulse/transforms/alignments.py 1 96.45%
qiskit/quantum_info/operators/mixins/adjoint.py 1 90.0%
qiskit/transpiler/passes/layout/sabre_pre_layout.py 1 98.88%
crates/qasm2/src/lex.rs 1 92.48%
Totals Coverage Status
Change from base Build 10598013395: -2.1%
Covered Lines: 25505
Relevant Lines: 29269

💛 - Coveralls

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 seems generally fine to me, though given that control_flow is a part of the Operation, does it make sense just to similarly define this somewhere higher up as well, so we need to do less unpacking of the internal objects to get here?

Comment on lines 2092 to 2093
pub fn blocks(&self) -> Option<impl Iterator<Item = CircuitData>> {
Python::with_gil(|py| -> Option<vec::IntoIter<CircuitData>> {
Copy link
Member

Choose a reason for hiding this comment

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

If this needs the GIL, can we just accept the GIL token in the function call? The PR comment says this is unlikely to be the final interface, and removing the GIL token from all the call sites won't be hard when it comes to it.

I'm concerned about how much we're hiding with_gil calls in Qiskit at the moment - given that the move to Rust is in part meant to make threaded parallelism easier for us, I think hiding implicit serialisation is possibly working against us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved blocks to the Operation trait, and it seems like that interface doesn't accept a Python token for any of the methods. I agree with your concern, but keeping it out of the signature feels more consistent with everything else, for now.

crates/circuit/src/operations.rs Outdated Show resolved Hide resolved
@kevinhartman
Copy link
Contributor Author

This seems generally fine to me, though given that control_flow is a part of the Operation, does it make sense just to similarly define this somewhere higher up as well, so we need to do less unpacking of the internal objects to get here?

It may. Do you have any thoughts on adding a new variant to OperationRef for control flow? That might not be the right choice for this PR, but I wonder if it'd be a better API long-term to be able to unpack a control flow operation and access its blocks with something like:

if let OperationRef::ControlFlow(op) = op {
    match op.op {
        ControlFlowOp::ForLoop | ControlFlowOp::WhileLoop => { todo!() },
        _ => (),
    }

    // No need to unwrap an option because we know we have a control flow op.
    for block in op.blocks() {
        todo!()
    }
}

I don't think we'd need any support from PackedOperation for this.

@jakelishman
Copy link
Member

I'm not sure what you mean by not needing support from PackedOperation - OperationRef is PackedOperation for all intents and purposes; it's just "proper" enum form of a reference to it. (There's space for 4 more variants in PackedOperation without any trouble at all, and the existing three Python forms could be compressed into a single one if we needed to anyway.)

I guess "longer term" depends a lot on the degree of length here. Long long term, I think control-flow operations may want to move out of being in a PackedInstruction at all, and be first-class in the data description (especially in DAGCircuit, less clearly in CircuitData). Shorter term, it could be fine to separate it off, but if the primary reason to do that is to avoid an Option here, maybe we just get rid of the Option in the method and leave the PR as it is? If it isn't a control-flow operation, we can just return an empty iterator, which is an equally valid return value; the caller of this function is presumably already calling Operation::control_flow before anyway.

Also addresses other review comments.
@kevinhartman
Copy link
Contributor Author

Long long term, I think control-flow operations may want to move out of being in a PackedInstruction at all, and be first-class in the data description (especially in DAGCircuit, less clearly in CircuitData). Shorter term, it could be fine to separate it off, but if the primary reason to do that is to avoid an Option here, maybe we just get rid of the Option in the method and leave the PR as it is? If it isn't a control-flow operation, we can just return an empty iterator, which is an equally valid return value; the caller of this function is presumably already calling Operation::control_flow before anyway.

Heh, that's all fair. I ended up moving blocks to Operation to make it easy to access in the meantime (hopefully justified in the same way that Operation::control_flow is part of that interface). The return type of blocks is now Vec<CircuitData> (mostly because I think returning an abstract Iterator from a trait method is not stable in Rust), and the caller will get an empty Vec if the operation does not have any blocks for any valid reason.

Copy link
Contributor

@raynelfss raynelfss left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @kevinhartman, I'm sure I'm not the only one in desperate need of this method existing in Rust.

Everything looks fine but you might want to reconsider the usage of Python::with_gil() here, specially since this method will only work with PyInstruction and the user will most likely hold a py token when calling this method.

@@ -145,6 +146,7 @@ pub trait Operation {
fn num_clbits(&self) -> u32;
fn num_params(&self) -> u32;
fn control_flow(&self) -> bool;
fn blocks(&self) -> Vec<CircuitData>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since blocks will only return a populated Vec when dealing with a PyInstruction, could we make this accept a py token regardless? It is very likely that the user here will have obtained the GIL whenever they want to play with an instance of PyInstruction, it's all within the name after all.

I saw your previous comments when discussing with Jake about keeping this consistent. However, in this case, this method is only supposed to work with one type of operation, one that specifically holds a PyObject and is tied quite closely with python, and even then it is not guaranteed to be an instance of ControlFlowOp. The function itself will also perform callbacks to python and obtaining the GIL manually might prove itself costly here (it would be nice to benchmark it beforehand to see if this holds true).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the case that right now blocks comes from Python, but once we've ported control flow operations to Rust that goes away. I don't see a compelling reason to ask for a Python token at the Operation level for blocks but not for its other methods, so I'd rather be consistent to avoid implying to code readers that there's some logical difference between Operation::blocks and say, Operation::matrix.

In the case of performance, having the GIL already means that with_gil will return fastest (as opposed to not holding it). And, we don't even try to acquire it unless this is a control flow operation, so the penalty to calling with_gil within Operation::blocks would scale with respect to the number of control flow operations in the circuit, rather than the number of op nodes.

@kevinhartman kevinhartman changed the title Add PyInstruction::blocks method. Add Operation::blocks method. Sep 13, 2024
Copy link
Contributor

@raynelfss raynelfss 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 sensible to me, and about ready to merge. I only have one inline comment here that you might want to look into 👀

Comment on lines 2125 to 2137
Python::with_gil(|py| -> Vec<CircuitData> {
let raw_blocks = self.instruction.getattr(py, "blocks").unwrap();
let blocks: &Bound<PyTuple> = raw_blocks.downcast_bound::<PyTuple>(py).unwrap();
blocks
.iter()
.map(|b| {
b.getattr(intern!(py, "_data"))
.unwrap()
.extract::<CircuitData>()
.unwrap()
})
.collect()
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Python::with_gil(|py| -> Vec<CircuitData> {
let raw_blocks = self.instruction.getattr(py, "blocks").unwrap();
let blocks: &Bound<PyTuple> = raw_blocks.downcast_bound::<PyTuple>(py).unwrap();
blocks
.iter()
.map(|b| {
b.getattr(intern!(py, "_data"))
.unwrap()
.extract::<CircuitData>()
.unwrap()
})
.collect()
})
Python::with_gil(|py| -> PyResult<Vec<CircuitData>> {
let raw_blocks = self.instruction.getattr(py, "blocks")?;
let blocks: &Bound<PyTuple> = raw_blocks.downcast_bound::<PyTuple>(py)?;
blocks
.iter()
.map(|b| -> PyResult<_> {
b.getattr(intern!(py, "_data"))?.extract::<CircuitData>()
})
.collect::<PyResult<_>>()
})
.unwrap_or_default()

You might find this option more attractive. We let the iterator and the Python::with_gil() do their own result handling and we only unwrap at the end and use the Default option were it to ever fail. This option is more stable as it has zero chance of ever panicking, if it ever were to fail an empty list will be returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually very similar to what I had originally in 622509e 🙂 (the difference being that I was returning an Option instead of an empty Vec if something went wrong).

In the current version, I actually purposefully made this code less flexible. The reason being that if one of these expectations is not true (e.g. if self.instruction does not have a blocks attribute), then it means that there is a bug in Qiskit. In that case, returning an empty Vec would only mask the underlying issue and prevent us from finding out about it / fixing it.

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw I'm fairly sure I've done the same thing as Kevin elsewhere and taken the control_flow attribute to mean that it's an instance of ControlFlowOp and therefore blocks must exist (which matches the Python expectations).

Copy link
Contributor

Choose a reason for hiding this comment

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

That's good then, can you add a comment explaining this? I think it's a good idea to mention that all of this unwrapping is being done with the purpose of catching bad behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done in 2a0951f.

@raynelfss raynelfss self-assigned this Sep 18, 2024
jakelishman
jakelishman previously approved these changes Sep 18, 2024
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 also seems fine to me now, and Ray can merge if he's happy.

@raynelfss raynelfss enabled auto-merge September 19, 2024 14:21
@raynelfss raynelfss added this pull request to the merge queue Sep 19, 2024
Merged via the queue into Qiskit:main with commit 4c64a64 Sep 19, 2024
15 checks passed
@raynelfss raynelfss mentioned this pull request Sep 30, 2024
4 tasks
@raynelfss raynelfss added the Changelog: None Do not include in changelog label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants