Repository navigation
Reject standalone PHIR-JSON execution that generates quantum commands and forward the boxed engine - #977
Merged
Conversation
… and forward the boxed engine
ciaranra
force-pushed
the
phirjson-standalone-rejects-quantum
branch
from
October 2, 2026 08:13
1d9a03d to
597b5a0
Compare
…EADME as doctests, and explain the external engine's reset contract
…ead of dropping data
…tum' into phirjson-standalone-rejects-quantum
…bilizer wrapper, and dispatch QuantumSimulator backends by exact name
…the private dispatch table
This was referenced Oct 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #883. Closes #974.
What was wrong
PhirJsonEngineis a classical-control engine: it generates quantum commands and consumes measurements supplied from outside. Run a program the intended way and it is correct. Callprocess(())standalone and it was not:start(), aStateVecEngine,continue_processing()out = "001"from_program(...).process(())out = "000"The standalone path logged that it could not process quantum operations, said it was "falling back to manual direct execution for integration testing", discarded the command batch, walked the program executing only classical operations, and returned
Ok(get_results()). The0was an export's untouched initial value, never a measurement. A caller received success for a program whose quantum half never ran, and the crate README demonstrated exactly that call.The change
117 lines of manual replay are replaced by a loop over the existing execution cursor. An empty command batch advances; a non-empty one is an error:
Detection is dynamic, not static. A scan of the program for quantum operations would reject a program whose quantum branch is never taken; the cursor selects the branch actually executed. Dynamic detection is less restrictive and equally safe, and it also catches quantum work appearing only after an earlier empty yield. A test covers the untaken-branch case, and the mutation that swaps dynamic for static detection fails it.
Draining empty batches is required rather than incidental:
Resultoperations and successful foreign calls both yield a batch with no quantum operation queued, so rejecting everyNeedsProcessingwould have broken eleven legitimate classical callers.The boxed path, which is why a one-method fix would not have worked
Engine for Box<dyn ClassicalControlEngine>had its ownprocess, looping and answering everyNeedsProcessingwith an emptyByteMessageuntil the engine reported completion. The public factory returns exactly that boxed type, and the box callsstart/continue_processing, never the concreteprocess. Fixing onlyPhirJsonEngine::processwould have left the publicly-returned path fabricating.The box now carries the same policy as the concrete engine:
start(), drain empty batches, reject a non-empty one, with its own error message so a caller can tell the two paths apart.An earlier revision of this branch made the box forward to the concrete implementation instead, and that was a regression. An independent review executed every implementor through the boxed path and found that forwarding reintroduced, in five other engines, the same defect this change exists to remove:
Ok({}), cursor at zeroanswer = 42, cursor 2, finishedc = 1c = 1c = 3c = 0, batch advanced[1,2,3]Ok({result: 0})Err(Input("Message too small for batch header"))Err(Processing)The cause is that the old boxed loop did two things: it drove the program to completion, which is necessary because several concrete
processimplementations returnget_results()without advancing the cursor, and it answered quantum requests with nothing, which is the fabrication. Forwarding removed both. The corrected loop removes only the second, and keepingstart()is what restoresExternalClassicalEngine's batch validation.This still changes behaviour deliberately: a boxed engine whose concrete
processdiscarded a non-empty batch now errors where it returnedOk. Discarding a batch is the defect.Boxed coverage uses real engines in four crates —
pecos-engines,pecos-phir,pecos-qasmandpecos-phir-pliron— rather than mocks, because the first version's two mock tests passed against anExternalClassicalEngine-only mutation and so tested forwarding mechanics rather than cross-crate behaviour. The Pliron cases seed outcomes before processing, since a fresh zero result is otherwise indistinguishable from a retained stale one. Restoring the forwarding now fails three PHIR tests and two engines tests.Removing the fallback also fixes a self-repairing test
tests/wasm_direct_test.rsdetected that its result was wrong and inserted the expected value before asserting, so it could not fail — issue #974. There were two such repairs, not one. Both are removed, and real execution produces10and11: the expected values were correct all along. The test had stopped reporting that the code was not.Two analogous repairs in the WASM integration tests are removed on the same grounds.
Migrated callers
25 call sites were inventoried, 20 in registered Cargo test targets, plus the README.
Kept standalone — the classical-only callers, now requiring exact values rather than mere success: arithmetic
35, comparisons[1,1,1,1], bit operations[1,7,6,12], nested expression60, bit access[1,0,1,5], WASM12,17,10,579, and the missing-function error test unchanged.Migrated to a simulator — the callers that relied on fabricated success: the quantum-operation, angle-unit and machine-operation tests, and the README example. Permissive assertions are gone; one previously accepted either bit value and several tolerated a missing output entirely. Bell outcomes are now asserted as exactly
{0, 3}over 32 shots, and the angle-unit and machine-operation tests assert the exact emitted commands. The machine-operation programs are also run to completion on a state-vector simulator, asserting the classical results after the machine operations (x = 1,a = 42), so a break in the classical path after a machine operation is caught.Two fixtures changed
HtoXso their outcomes are deterministic. Because that thinned Hadamard coverage, a deterministic Hadamard test is added:H; Z; HisX, so it measures1every shot. The obvious alternative would have been worthless —H; H; Measureyields0even ifHwere the identity — whereas this fails for both the identity andX, verified by deleting the Hadamards and watching it fail.The crate README's Rust examples are now compiled and run as doctests (
#[cfg(doctest)]overinclude_str!("../README.md")). Doing so exposed four further stale examples: a nonexistentexamples/bell.phir.json, a wrongDepolarizingNoiseModelpath, a block importing a test-only helper throughcrate::common, and a converter example whose JSON was literally[...]. All are fixed and now run.The five call sites under
crates/pecos-phir/tests/engines/andtests/circuits/are in files Cargo does not register as targets, so they do not compile today. They are untouched; dead test files are tracked separately.ByteMessage readers no longer drop data
The boxed
processdecides "quantum work or not" withByteMessage::is_empty, which inherited the legacy v1 readers' leniency. Those readers returnedOkwhile dropping data:Outcome/ReturnValuepayload yielded nothing, and a long one had its surplus ignored;total_size, were ignored;Two callers also swallowed parse errors:
NoiseUtils::has_measurementsmappedErrtofalse, and PythonByteMessage.is_emptymappedErrtotrue.The readers are now strict:
MessageWalkerreplaces three duplicated helpers. It checks the batch header,total_size == byte_len, header and payload bounds, padding, and trailing bytes, using checked offset arithmetic. Every failure is anInputerror naming the message (orBatch:for batch-wide faults). It boundsmsg_countby what the bytes can hold before anything is reserved. Previously a 16-byte batch claimingu32::MAXmessages madequantum_ops_intoabort the process on allocation failure, and that path is reachable from the C ABI.quantum_opsaccepts only Gate messages,outcomesonly Outcome,return_valueonly ReturnValue (at most one). A zero-message batch reads as empty for every reader. Payload headers are read unaligned, soReturnValueHeader's 8-byte alignment cannot panic on 4-aligned storage.is_emptyis a structural walk plusmsg_count == 0. It no longer decodes every gate.for_quantum_operations()/for_outcomes()after messages of another mode, now panic, as the existing mixing checks already did.Outcome/ReturnValuepayloads must have their exact size. Sizes aboveu32::MAXpanic instead of being clamped into a corrupt header with awarn!.apply_measurement_replyparses outcomes once and propagates errors.has_measurementslost its only caller and is removed. Pythonis_empty()now raises.No in-tree producer emits a batch the new rules reject; all engines, noise models, bridges, Selene plugins and the C ABI build single-type batches through the builder. The wire format and protocol version are unchanged.
docs/development/foreign-plugins.mdnow states the reader contract and the C ABI's-1/ null / zero-length error outputs.Evidence:
u32::MAXpanics, checked-offset overflow on 64-bit, and the Pythonis_emptyerror mapping, because Python has no raw-bytes constructor.Simulator seed streams, and removing CuStabilizer (breaking)
Investigating
QuantumSimulator.init's seed handling found two simulators giving wrong measurement statistics.Run through
HybridEngine, a noiseless Bell circuit should split roughly evenly between its two outcomes:000on all 200 shots, andX; MZreturned0MPS rebuilt pytket's
MPSxGateon everyreset()from the same seededConfig. pytket seeds a fresh RNG in that constructor, so every shot replayed the same draws. The wrapper now owns onerandom.Random(seed), created before the first reset. It sets a freshConfig.seedon each reset, so a fixed seed reproduces the whole run and every shot draws fresh randomness. Unseeded behaviour is unchanged.CudaStabilizer / Rust
CuStabilizerare removed. They wrapped cuStabilizer's Pauli-frame simulation, whose measurement table holds frame flips relative to a noiseless reference rather than outcomes, as if it were a gate-by-gate stabilizer simulator. The wrapper also started a fresh one-shot frame simulator at every measurement, losing all state. Yet it implementedCliffordGateable,StabilizerTableauSimulator,ForcedMeasurementandStabilizerSimulator, and was documented as a GPU stabilizer simulator. Its measurement trait is infallible, so it could not refuse without panicking.This is a breaking change. Removed:
CuStabilizertype and its trait impls;CuStabilizerbinding and stub;pecos_rslib_cuda.is_custabilizer_usable, which now guarded nothing reachable from Python;pecos.simulators.CudaStabilizerpackage;"CudaStabilizer"backend name;CuFrameSimulator, the raw frame-simulation API in the Rust crate, is kept. The docs now say no GPU stabilizer simulator is provided. #1068 tracks a correct GPU stabilizer sampler: frame samples XORed into a CPU tableau reference.QuantumSimulatordispatch is an explicit table. Before:self.backend in "state-vector"was a substring test, so 76 strings selected StateVec, including""and"e".Qulacs, which had no implementation.'NoneType' object is not callable.seedwas dropped by catching aTypeErrorand matching its message.Now:
seed.ValueErrorat construction.ImportErrornaming its dependency.TypeErrorretry is gone.The
seedretry itself never left a backend unseeded. SparseStabPy and CuStateVec draw from PECOS's global RNG, whichHybridEngineseeds. The docstring now says so for direct callers.The seed-determinism tests compared results to
"11"/"00", but results are 3-character strings ("000","011"). So those counts were always zero, and the "noise produces errors" check passed vacuously. They now compareint(x, 2). The different-seeds test also covers MPS, CuStateVec and CudaStateVec.The MPS file's diff is whole-file because it had mixed CRLF/LF line endings. They are normalised to LF.
Evidence:
TypeErrorretry, dropping the unavailable-backend error, marking MPS as not taking a seed, disabling the construction-time name check, and flipping CuStateVec's seed flag.Out of scope, deliberately
The cause is that
ClassicalEnginerequiresEngine<Input = (), Output = Shot>, obliging eight controllers to advertise a capability none has. Five fabricate a shot in five different ways — the box's empty-message loop, PHIR-JSON's manual replay, QASM's "best-effort results", QIS's "empty measurements", and the Python bridge's all-zero outcomes. That is #975 and is an API break. This change fixes the reachable path; QASM, QIS, PHIR, Pliron, the Monte Carlo controller and the Python bridge keep their own fabrications until #975 lands.Coverage
crates/pecos-phir-json/tests/standalone_execution.rsis new. It pins rejection for a plain gate, a measurement, a gate in a taken branch, a machine operation emitting a command, and quantum work after an earlier export or foreign call; success for an untaken quantum branch; classical success across multiple yields; and that a foreign call executes exactly once — the removed fallback restarted its walk, so a stateful foreign call could previously have run twice, and restoring the replay changes that counter to2.31 mutations were applied and every one failed its intended tests before the code was restored.
Verification
On head
ed281025e(dev merged in at72dccd7d3, bringing #1050):just rstest dev: exit 0; 14,077 passed, 0 failed.just python-ci-lint(rustfmt, clippy with all features, pre-commit, cargo check): exit 0.pecos-cuquantumtests on a local RTX 4090: exit 0, 41 passed.cuda_setupdoc tests and the import-laziness test: 95 passed.just pytest-ci-corelocally: 8,918 passed. Of the 28 failures under the lane's parallel workers, 26 are CuStateVec/MPS/CudaStateVec tests hitting CUDA out-of-memory on the shared local GPU, and all 26 pass when re-run serially. The other 2 are the knowntest_simulatorsdoc blocks 9 and 10, which fail withUnrecognized quantum engine builder type; that error predates this change. CI has no GPU, so the GPU tests skip there.Noted, not changed
The box routes
Engine::resetthroughClassicalEngine::reset, whose QIS implementation is a no-op, so a boxed engine reset aborts no dynamic worker where a control reset aborts one. That predates this change. #1050 (now merged) adds theClassicalEngine::resetoverride, and #1057 (#1044) cancels a running shot and reclaims the worker-owned interface, so a QIS shot abandoned by the boxedprocessbecomes recoverable once those merge.Tracing that path found two QIS defects that no existing issue covered:
They are fixed in their own stacked PRs, #1064 and #1066, not here. #1050 has merged and is included via the dev merge. The Python bridge's reset clears
program, so itsprocessalready fails; unchanged here.