✨ Adopt exact QDMI 1.4 payload contracts across Core - #2226
Conversation
ee2c0b4 to
f19a838
Compare
b9a5363 to
31d6f2d
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
bb0240b to
28284fa
Compare
31d6f2d to
b7d1f2a
Compare
burgholzer
left a comment
There was a problem hiding this comment.
This one took quite a bit of time to review. and there are still several areas that I would like to see changed. However, I have the feeling this is going to be the last iteration on this before it is ready.
@denialhaag @simon1hofmann do you have any feedback on top of the above?
| [[nodiscard]] inline std::string | ||
| decodeText(std::string value, const std::string_view description) { | ||
| if (value.empty() || value.back() != '\0') { | ||
| throw std::invalid_argument(std::string(description) + | ||
| " is not null-terminated"); | ||
| } | ||
| if (value.find('\0') != value.size() - 1U) { | ||
| throw std::invalid_argument(std::string(description) + | ||
| " contains an embedded null byte"); | ||
| } | ||
| value.pop_back(); | ||
| return value; | ||
| } |
There was a problem hiding this comment.
I am likely the one being too dense here, but it feels like this may be introducing off-by-one errors or false requirements.
an std::string is always null-terminated and IIRC the null terminator is not accounted in size(). I want to make absolutely sure that this doesn't introduce an architecture where strings are essentially <chars>\0\0 in memory.
| if (value.empty()) { | ||
| throw std::invalid_argument(std::string(description) + | ||
| " is not null-terminated"); | ||
| } |
There was a problem hiding this comment.
This error message feels wrong. are we sure this is useful, needed, and correct?
| std::string value(size - 1, '\0'); | ||
| std::string value(size, '\0'); | ||
| result = QDMI_device_query_device_property(device_.get(), prop, size, | ||
| value.data(), nullptr); | ||
| qdmi::throwIfError(result, msg); | ||
| return value; | ||
| return detail::decodeText(std::move(value), msg); |
There was a problem hiding this comment.
This is another case where I would want to make absolutely sure this is correct and does not introduce off-by-one errors. There's more cases below.
| job.def( | ||
| "get_results", | ||
| [](const qdmi::Job& self, const size_t programIndex, | ||
| const QDMI_Job_Result result) { | ||
| const auto value = self.getResults(programIndex, result); | ||
| return nb::bytes(reinterpret_cast<const char*>(value.data()), | ||
| value.size()); | ||
| }, | ||
| "program_index"_a, "result"_a, | ||
| "Returns one indexed result as exact bytes."); |
There was a problem hiding this comment.
would it makes sense to flip the order of the arguments here so that the index could receive a default argument? Which would allow to simplify a few call sites.
| job.def_prop_ro("programs_num", &qdmi::Job::getProgramsNum, | ||
| "The number of programs in the job."); |
There was a problem hiding this comment.
Related to another comment from a PR as part of this stack: should this be called num_programs to align better with the rest of the bindings and Python exposure. May also be worth to already rename that on the C++ side before binding to Python.
|
|
||
|
|
||
| # MQT Core owns the two OpenQASM formats and registers them through the same | ||
| # registry as everyone else, so the backend walks one ordered list of formats | ||
| # with no format-specific branch. `mqt.core.plugins.qiskit.__init__` imports this | ||
| # module whenever Qiskit is installed, so both formats are available as soon as | ||
| # the adapter is. | ||
| register_program_serializer(ProgramFormat.QASM3, _serialize_to_qasm3) | ||
| register_program_serializer(ProgramFormat.QASM2, _serialize_to_qasm2) |
There was a problem hiding this comment.
This changes how program serialization for Qiskit is handled from the 3.9.1 release. Make sure all documentation is updated accordingly and there are no remaining fragments of the old integration style.
| self._capabilities = DeviceCapabilities( | ||
| qjit_compatible=False, | ||
| runtime_code_generation=False, | ||
| dynamic_qubit_management=False, |
There was a problem hiding this comment.
feels like the capability management could be better here. dynamic qubit management is an explicit payload capability.
Carefully check for a suitable mapping between feature of the payload and how that maps to PennyLane.
| if len(dependencies) > 16: | ||
| msg = "A conditional depends on more than 16 measurements." | ||
| raise ValidationError(msg) |
There was a problem hiding this comment.
Is this an artificial limitation or an actual PennyLane limitation?
There was a problem hiding this comment.
QDMI v1.4 also introduces multi-program job submission.
Qiskit backend run natively supports passing QuantumCircuit | Sequence[QuantumCircuit]. the latter case can now be natively be handled by QDMI instead of in the QDMI backend code itself. This should clean up the implementation quite a bit.
With the planned changes to the DDSIM QDMI backend, this can also be properly tested.
There was a problem hiding this comment.
I am not familiar enough with PennyLane to judge, but I would guess it also supports the submission of multiple circuits to a device as part of one job. This should now work natively with QDMI v1.4
Explore whether the device itself can be improved in that regard.
simon1hofmann
left a comment
There was a problem hiding this comment.
Went through the changes and have some comments that should be double-checked.
| @@ -505,15 +613,16 @@ | |||
| // Update the measurement counts. | |||
| ++counts_[runtime.getMeasurements()]; | |||
There was a problem hiding this comment.
getMeasurements() stores slot 0 first, but QDMI requires the highest slot first. Values [1,0,0] therefore produce "100" instead of "001".
There was a problem hiding this comment.
If this is true, then we may want to adjust the QIR runtime function itself so that it follows the same convention. We should generally try to stick to the same convention throughout MQT Core wherever we can; even if this means changes now.
| switch (result) { | ||
| case QDMI_JOB_RESULT_HIST_KEYS: | ||
| case QDMI_JOB_RESULT_HIST_VALUES: | ||
| if (numShots_ == 0) { | ||
| return QDMI_ERROR_INVALIDARGUMENT; | ||
| } | ||
| return getHistogram(result, size, data, sizeRet); |
There was a problem hiding this comment.
Sampled jobs always expose histograms even when bool, integer, floating-point, or container outputs are omitted. QDMI requires QDMI_ERROR_NOTSUPPORTED when output cannot be represented losslessly.
There was a problem hiding this comment.
Hm. This feels a bit wrong to me. Shouldn't it be up to getHistogram to report the error? Maybe I am misinterpreting this though?
There was a problem hiding this comment.
Yes that should be reported by getHistogram, but that function currently never reports that error 🤔
🤖 AI text below 🤖
Yes, getHistogram is the better place to report this. My concern was about the current behavior, not that the check must live in this switch. getHistogram currently always succeeds, while counts_ only contains RESULT/RESULT_ARRAY bits. For AdaptiveRecordOutputs.ll, that omits the recorded integer and double values, so the three-bit histogram is only a partial representation. Under the pinned QDMI contract, the histogram queries must return QDMI_ERROR_NOTSUPPORTED in that case. Container records alone do not invalidate the histogram; that part of my wording was too broad.
There was a problem hiding this comment.
Hm. We may want to adjust the QDMI spec here. This feels overly restrictive. It may still make sense to export a histogram, even though there is further output in the proprietary results.
Good time to make changes to the open QDMI PRs so this becomes less of a hassle.
| capabilities = self._program_capabilities or set() | ||
| for capability, instruction, name in ( | ||
| ("forward-branching", IfElseOp, "if_else"), | ||
| ("counted-iteration", ForLoopOp, "for_loop"), | ||
| ("conditional-loop", WhileLoopOp, "while_loop"), | ||
| ("multiway-branching", SwitchCaseOp, "switch_case"), | ||
| ): | ||
| if capability in capabilities: | ||
| target.add_instruction(instruction, name=name) |
There was a problem hiding this comment.
Circuits using measurement-driven branches and measured-qubit reuse are accepted without the separately required capabilities.
There was a problem hiding this comment.
Is that something that we can reasonably express in Qiskit? without going entirely out of our way checking the program?
This may become a little easier once we can actually wedge the compiler collection between the input program and the required output program.
There was a problem hiding this comment.
Probably not..
Just noticed that for example in this test: test_mock_backend.py:547-566 mid-circuit-measurement should be required due to the circuit.measure(0, 0), but boolean-computation is not needed.
| pending = list(bound_circuit.data) | ||
| while pending: | ||
| instruction = pending.pop() | ||
| op_name = instruction.operation.name | ||
| for block in getattr(instruction.operation, "blocks", ()): | ||
| pending.extend(block.data) | ||
| if op_name in {"if_else", "for_loop", "while_loop", "switch_case"}: | ||
| if op_name not in self._target.operation_names: | ||
| msg = f"Unsupported control flow operation: '{op_name}'" | ||
| raise UnsupportedOperationError(msg) | ||
| continue |
There was a problem hiding this comment.
Circuits using measurement-driven branches and measured-qubit reuse are accepted without the separately required capabilities.
| std::ostringstream output; | ||
| runtime.setOstream(output); | ||
| if (const auto rc = jitSession.run(); rc != 0) { | ||
| throw std::runtime_error( | ||
| llvm::formatv("QIR program failed with error: {}", rc)); | ||
| } | ||
| auto state = runtime.takeState(); | ||
| dd_ = std::move(state.dd); | ||
| stateVecDD_ = state.edge; | ||
| }); |
There was a problem hiding this comment.
State extraction neither emits the required QIR header nor assigns programOutput_ (observed at :916-918).
There was a problem hiding this comment.
State extraction itself should likely not do this at all. But we should definitely be exposing the output stream from QIR through the new result type, which should already be captured by the QIR runner.
| [[nodiscard]] constexpr bool | ||
| isValidProgramFormat(const QDMI_Program_Format& format) noexcept { | ||
| return format.version != 0U && format.id[0] != '\0' && | ||
| (format.encoding == QDMI_PROGRAM_ENCODING_TEXT || | ||
| format.encoding == QDMI_PROGRAM_ENCODING_BINARY) && | ||
| detail::isCanonicalFixedString(format.id) && | ||
| detail::isCanonicalFixedString(format.profile); | ||
| } |
There was a problem hiding this comment.
It accepts binary OpenQASM, unsupported QIR profiles and unqualified vendor IDs, causing NOTSUPPORTED where QDMI requires INVALIDARGUMENT.
There was a problem hiding this comment.
Fair enough. Validation could be more strict here.
| -> QDMI_STATUS { | ||
| if ((data != nullptr && size == 0) || | ||
| IS_INVALID_ARGUMENT(result, QDMI_JOB_RESULT)) { | ||
| IS_INVALID_ARGUMENT(result, QDMI_JOB_RESULT) || programIndex != 0U) { |
There was a problem hiding this comment.
Index 1 on a completed single-program job returns INVALIDARGUMENT; QDMI requires OUTOFRANGE.
There was a problem hiding this comment.
This is true, but the actual implementation will likely change a bit based on the other review comments. It is still good to make use of OUTOFRANGE for out of range access to the result list.
| auto QDMI_Session_impl_d::setParameter(QDMI_Session_Parameter param, | ||
| const size_t size, | ||
| const void* value) const -> int { | ||
| if ((value != nullptr && size == 0) || param >= QDMI_SESSION_PARAMETER_MAX) { | ||
| return QDMI_ERROR_INVALIDARGUMENT; | ||
| } | ||
| if (status_ != qdmi::SessionStatus::ALLOCATED) { | ||
| return QDMI_ERROR_BADSTATE; | ||
| } | ||
| return QDMI_ERROR_NOTSUPPORTED; | ||
| } | ||
|
|
||
| auto QDMI_Session_impl_d::querySessionProperty(QDMI_Session_Property prop, | ||
| size_t size, void* value, | ||
| size_t* sizeRet) const -> int { | ||
| if ((value != nullptr && size == 0) || prop >= QDMI_SESSION_PROPERTY_MAX) { | ||
| return QDMI_ERROR_INVALIDARGUMENT; |
There was a problem hiding this comment.
The >= *_MAX checks reject QDMI’s valid custom range 999999995..INT32_MAX instead of returning NOTSUPPORTED.
There was a problem hiding this comment.
I believe this is fixed in a later PR down the line in this PR stack. Might be worth pulling the changes in here already.
There was a problem hiding this comment.
After checking again, this might not actually be fixed yet. And it definitely should be fixed. Here would be a good place to do so.
| @overload | ||
| def get_custom_result(self, custom_property: CustomProperty, value_type: type[str]) -> str | None: ... | ||
| @overload | ||
| def get_custom_result(self, custom_property: CustomProperty, value_type: type[bool]) -> bool | None: ... | ||
| @overload | ||
| def get_custom_result(self, custom_property: CustomProperty, value_type: type[int]) -> int | None: ... | ||
| @overload | ||
| def get_custom_result(self, custom_property: CustomProperty, value_type: type[float]) -> float | None: ... | ||
| @overload | ||
| def get_custom_result(self, custom_property: CustomProperty, value_type: type[bytes]) -> bytes | None: ... | ||
| @overload | ||
| def get_custom_result( | ||
| self, custom_property: CustomProperty, value_type: type[str | bool | int | float | bytes] | ||
| ) -> str | bool | int | float | bytes | None: | ||
| """Return an implementation-defined custom job result. | ||
|
|
||
| The caller must provide the type documented by the device implementation. | ||
| Use ``bytes`` to retrieve the value without interpretation. Returns ``None`` | ||
| when the custom slot is unsupported. | ||
| """ |
There was a problem hiding this comment.
The runtime binding accepts program_index: int = 0, but every overload omits it, so valid multi-program calls fail static type checking.
There was a problem hiding this comment.
This is definitely an oversight and needs to be fixed either in the bindings themselves or in the pattern file.
denialhaag
left a comment
There was a problem hiding this comment.
Some additional comments from my side, none of which I'm super sure about myself:
|
|
||
|
|
||
| cpp_api_tagfile = ("_build/doxygen/mqt-core.tag", "cpp/", "_build/doxygen/xml") | ||
| _qdmi_api_base = "https://munich-quantum-software-stack.github.io/QDMI/pr-preview/pr-509/" |
There was a problem hiding this comment.
Just noting that this likely needs to be changed before merging.
There was a problem hiding this comment.
Yeah. It needs to at least point to a merged version on the develop branch.
Before the eventual v4 release it needs to be pinned to a released version (the v1.4.0 tag).
| # run, you must also specify the path to the tagfile here. | ||
|
|
||
| TAGFILES = _build/qdmi.tag=https://munich-quantum-software-stack.github.io/QDMI/v1.3.2/ | ||
| TAGFILES = _build/qdmi.tag=https://munich-quantum-software-stack.github.io/QDMI/pr-preview/pr-509/ |
|
Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
28284fa to
be4c87a
Compare
Retain the design-gated exact-format and optional-feature prototype with its mechanical SDK consumers. Extract native multi-program jobs and indexed results into their own enum-based workstream; preserve current optional shots, binary transport, target inference, and simulator concurrency. Assisted-by: GPT-5.6 Sol via Codex
b7d1f2a to
2ff9471
Compare
🤖 AI text below 🤖
Lead: @simon1hofmann. Cross-repository coordination: @burgholzer.
Description
Keep the runtime portion of the experimental QDMI program-capability contract independently reviewable. This branch targets
v4.1, without compiler-only #2219, replaceable drivers, metadata cleanup, or native multi-program adoption.Native multi-program jobs and indexed results were extracted into #2373 using QDMI #509's existing format enum. This prototype instead uses QDMI #508's exact format descriptors, optional feature records, single-program submission, and format-defined output bytes. Mechanical Qiskit/PennyLane adaptations remain here so this runtime builds and its SDK consumers import and run coherently.
Current mainline optional shots, binary transport, DDSIM QCO simulation, session ownership, concurrent single-program execution, and fail-closed target inference are preserved. It does not replace concurrent submissions with synthetic aggregate jobs.
Design and release gate
This is a non-blocking Core 4.1 / QDMI 1.4 candidate, not Core 4.0 scope or a settled public contract. QDMI #523 and Core #2365 must record the format/capability decisions before this becomes merge-ready. In particular, provider-neutral execution guarantees, opaque payload semantics, and supported versus native operations remain design work.
Refs #2365, #2373, Munich-Quantum-Software-Stack/QDMI#508, Munich-Quantum-Software-Stack/QDMI#523.
Core #2227 is the separate adapter between this runtime and #2219's compiler model. Neither foundation depends on that adapter or on the other foundation. The development QDMI pin must be replaced with a released dependency before publishing release artifacts.
Validation
AI assistance was used for extraction, rebasing, implementation adaptations, tests, documentation and this description.
Interface acceptance
Describe the device-and-format-specific contract through #2365 and QDMI #523. Do not assume full OpenQASM/QIR language support or a universal native/verbatim compilation boundary. The prototype remains a priority for Core 4.1, not a release blocker.
Before the interface change merges, link a working Core consumer and a demonstration with at least one existing provider, preferably both where applicable. Compatibility evidence suffices when a provider needs no changes. Development revisions may support these tests; published artifacts must use released dependencies.
Checklist
If PR contains AI-assisted content:
🤖 *AI text below* 🤖(titles are exempt).