RFC1: Fix dt-unit Delay handling in the C API (minimal patch)#16226
RFC1: Fix dt-unit Delay handling in the C API (minimal patch)#16226ihincks wants to merge 1 commit into
Conversation
A `QuantumCircuit` with `Delay(n, "dt")` stores its integer duration as `Param::Obj(PyAny)` in Rust, since Param has no integer variant. Two consequences: 1. `qk_circuit_get_instruction` panics in `from_packed_instruction_with_numeric` when it encounters a non-Float/non-ParameterExpression param. 2. `CDelayUnit` has no DT variant, so dt-delays can't be constructed from C at all. A. Coerce Python int to f64 only on the Delay extraction path. B. Add `CDelayUnit::DT` and a new `qk_circuit_delay_dt(u64)` C function. C. Add a `Param::Int(i64)` variant. Rejected: every match over Param across the codebase would need updating. D. Have the C readback path extract ints from `Param::Obj`. Rejected: papers over the type confusion at every call site instead of fixing it at ingestion. E. Make qk_circuit_get_instruction return `QkExitCode` instead of void so it can fail gracefully. Rejected: would break the C ABI. After A, the panic site is unreachable for valid circuits anyway. F. Force Python to store dt durations as float. Rejected: user-visible API change requiring a deprecation cycle. This PR implements A and B. Changes A. In `crates/circuit/src/circuit_instruction.rs`, the Delay branch now uses the standard coercing `params.extract::<SmallVec<[Param; 3]>>()`. Python ints land as Param::Float. Round-trip back to Python is preserved by `Delay.validate_parameter`, which already converts fractional-free floats back to int for the dt unit, so delay.duration is still an int for users. B. `CDelayUnit::DT = 5` plus a `new qk_circuit_delay_dt(circuit, qubit, duration: u64) -> QkExitCode` that stores the duration as `Param::Float(duration as f64)`.
jakelishman
left a comment
jakelishman
left a comment
There was a problem hiding this comment.
This is a much more invasive change than it looks like - it's saying that we're not going to use int at all to represent dt-unit delays any more, and are just going to lie about that on exposure to Python space (and hope that nothing in Rust space ever used the ability to represent something as 1.5dt, since that'd raise a Python-space exception. Obviously that wouldn't happen right now because clearly nothing in Rust space significantly reasons about the values (as they're all Python objects), but in theory we should be doing it in Rust space, and using f64 means we've lost all our type safety.
I don't think we can safely do this: there's only pain ahead when we deliberate make two of our data models diverge.
| // `Delay.validate_parameter`. | ||
| let params: SmallVec<[Param; 3]> = params.extract()?; | ||
| Some(Parameters::Params(params)) | ||
| } |
There was a problem hiding this comment.
Woah boy that's one long range assumption.
That's fair, and no harm done seeing what it looks like this way. It was obviously taking the "don't be invasive" clause seriously, because I expect all other solutions would be bigger changes. |
ihincks
changed the title
ihincks
changed the title
2 participants
This PR is one of three RFCs proposing a fix for the same problem: a
QuantumCircuitcontainingDelay(n, "dt")panics when read back throughqk_circuit_get_instruction, and there is no way to construct a dt-Delay from C in the first place. The root cause is that an integernfrom Python lands inParam::Obj(PyAny), which the C readback path does not accept, andCDelayUnithas noDTvariant.This branch takes the smallest possible patch. The Python-side Delay extraction now coerces its integer duration to
f64so it stores asParam::Float, which the C readback already handles. It also addsCDelayUnit::DTand a newqk_circuit_delay_dt(circuit, qubit, duration: u64)entry point that stores the duration asParam::Float(duration as f64). Round-tripping back to Python is unchanged becauseDelay.validate_parameteralready converts a fractional-free float back to int for the dt unit.The trade-off is that integer durations are no longer integers internally — they are floats that happen to be whole. That's enough to fix the panic and unblock C callers without touching anything else, but it does not address the underlying type confusion.
The other two RFCs (#16241, #16242) explore deeper fixes that preserve integer-ness in Rust.
Partially addresses #16121.
AI/LLM disclosure