✨ QDMI authentication via FoMaC #1355
Conversation
marcelwa
added
enhancement
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR introduces QDMI authentication support to FoMaC by transforming it from a singleton pattern to an instantiable session-based architecture. The implementation enables per-session authentication while maintaining backward compatibility through module-level convenience functions.
Key Changes:
- FoMaC is now instantiable with lazy initialization and session parameter management
- Added SessionParameter enum with authentication options (TOKEN, AUTHFILE, AUTHURL, USERNAME, PASSWORD, PROJECTID, and 5 custom parameters)
- Implemented move semantics to support session transfer while preventing copying
- Python bindings expose FoMaC as
Sessionclass with backward-compatibledevices()function
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| include/mqt-core/fomac/FoMaC.hpp | Adds SessionParameter enum, removes singleton pattern, adds session management members (mutex, initialized flag, pendingParameters map) and move semantics |
| src/fomac/FoMaC.cpp | Implements deferred initialization via ensureInitialized(), session parameter validation, move operations, and module-level convenience functions |
| src/na/fomac/Device.cpp | Updates to use namespace-level getDevices() function instead of static class method |
| test/fomac/test_fomac.cpp | Adds comprehensive authentication tests including parameter validation, URL/file validation, and initialization state tests |
| bindings/fomac/fomac.cpp | Exposes SessionParameter enum and FoMaC class as Python Session with set_session_parameter and get_devices methods |
| python/mqt/core/fomac.pyi | Adds type stubs for Session class, SessionParameter enum, and module-level session functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces the FoMaC singleton with an instantiable fomac::Session (SessionConfig with auth fields), migrates bindings/tests/docs to Session APIs, adds qdmi::DeviceSessionConfig and dynamic device library loading (non-Windows), and removes explicit singleton lifecycle across QDMI device implementations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller (Python / C++)
participant Session as fomac::Session
participant Driver as qdmi::Driver
participant DynLib as DynamicDeviceLibrary
participant Device as QDMI Device Impl
Client->>Session: Session(SessionConfig{token, auth..., custom...})
Note right of Session: validate params, initialize session state
Client->>Driver: addDynamicDeviceLibrary(libPath, prefix, DeviceSessionConfig)
Driver->>DynLib: load library (dlopen / platform loader)
Driver->>Device: construct QDMI_Device_impl_d(lib, DeviceSessionConfig)
Device->>Device: setParameter(baseUrl/token/authFile/...) (skip unsupported)
Device-->>Driver: register devices in driver
Client->>Session: session.get_devices()
Session->>Driver: query loaded devices
Driver-->>Session: return vector<Device>
Session-->>Client: deliver devices list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used🧠 Learnings (13)📓 Common learnings📚 Learning: 2025-10-14T14:37:38.047ZApplied to files:
📚 Learning: 2025-12-05T17:45:37.602ZApplied to files:
📚 Learning: 2025-11-04T14:28:32.371ZApplied to files:
📚 Learning: 2025-12-07T09:10:31.820ZApplied to files:
📚 Learning: 2025-12-01T11:00:40.342ZApplied to files:
📚 Learning: 2025-11-03T23:09:26.881ZApplied to files:
📚 Learning: 2025-11-05T07:42:45.507ZApplied to files:
📚 Learning: 2025-10-17T11:09:50.147ZApplied to files:
📚 Learning: 2025-11-04T14:26:25.420ZApplied to files:
📚 Learning: 2025-11-24T10:19:41.147ZApplied to files:
📚 Learning: 2025-11-04T19:50:14.489ZApplied to files:
📚 Learning: 2025-11-27T08:52:06.428ZApplied to files:
🧬 Code graph analysis (2)include/mqt-core/qdmi/Common.hpp (3)
src/fomac/FoMaC.cpp (3)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (27)
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 |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe PR introduces session parameter management to FoMaC by refactoring it from a singleton pattern to an instance-based architecture with lazy initialization. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (6)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-11-05T07:42:45.507ZApplied to files:
📚 Learning: 2025-11-04T14:28:32.371ZApplied to files:
📚 Learning: 2025-10-17T11:09:50.147ZApplied to files:
🧬 Code graph analysis (5)src/na/fomac/Device.cpp (3)
python/mqt/core/fomac.pyi (2)
bindings/fomac/fomac.cpp (2)
include/mqt-core/fomac/FoMaC.hpp (1)
test/fomac/test_fomac.cpp (2)
🪛 GitHub Check: 🇨 Lint / 🚨 Lintsrc/fomac/FoMaC.cpp[warning] 864-864: src/fomac/FoMaC.cpp:864:9 [clang-diagnostic-ctad-maybe-unsupported] [warning] 844-844: src/fomac/FoMaC.cpp:844:3 [modernize-use-scoped-lock] [warning] 844-844: src/fomac/FoMaC.cpp:844:3 [misc-const-correctness] ⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (17)
Tip ✨ Issue Enrichment is now available for GitHub issues!CodeRabbit can now help you manage issues more effectively:
Disable automatic issue enrichmentTo disable automatic issue enrichment, add the following to your issue_enrichment:
auto_enrich:
enabled: falseThanks 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
This is a very quick review with the goal to dump some thoughts on the general direction here.
I'll try to summarize them again here:
- Breaking changes are entirely fine here
- I would call the entity
Sessionalso in C++. Makes the intent clearer - I would use keyword arguments for the constructor of the session (all optional; all defaulted to
std::nullopt). That might safe exposing the actual enum. - We can drop the default/global
get_devicesin my opinion. In a not too distant future, we may need to provide credentials for some of the devices that we include.
I think that's it from the very direct feedback.
There is one aspect, that we won't be solving as part of this PR.
And that is Munich-Quantum-Software-Stack/QDMI#161
Right now, only one set of authentication credentials can be set.
However, each device might potentially require different authentication criteria. The current way we generally implement drivers and, potentially, the way the client interface is structured don't allow us to set individual parameters per device.
This is something to think about in the not-too-distant future, but not as part of this PR.
ystade
left a comment
ystade
left a comment
There was a problem hiding this comment.
Hi @marcelwa , this is a thoughtful and forward-looking refactoring of the FoMaC. To summarize my comments:
- I am totally fine with dropping the name
FoMaCand call itSession(in aqdminamespace) - I would prefer to align the session handling closer to the original QDMI workflow, i.e., having individual memeber functions to set the indidivual parameters and then calling a function
initialize. Only afterwards, one can for example retrieve the list of devices.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/fomac/test_fomac.cpp (1)
14-27: Remove unused includes.Static analysis indicates that
<filesystem>,<fstream>, and<thread>headers are not directly used in this file. These were likely added for authentication tests that were subsequently removed (as noted in the comment on lines 618-621).#include <cstdio> -#include <filesystem> -#include <fstream> #include <gmock/gmock-matchers.h> #include <gtest/gtest.h> #include <new> #include <numbers> #include <qdmi/client.h> #include <ranges> #include <stdexcept> #include <string> -#include <thread> #include <tuple> #include <vector>bindings/fomac/fomac.cpp (1)
13-21: Add missing<optional>include.Static analysis indicates that
std::optionalis used (line 34) but the header is not directly included.#include <pybind11/cast.h> #include <pybind11/complex.h> // NOLINT(misc-include-cleaner) #include <pybind11/native_enum.h> #include <pybind11/operators.h> #include <pybind11/pybind11.h> #include <pybind11/stl.h> // NOLINT(misc-include-cleaner) #include <qdmi/client.h> +#include <optional> #include <string> #include <vector>src/fomac/FoMaC.cpp (1)
17-28: Remove unused<mutex>include.Static analysis indicates this header is not used directly in this file.
#include <filesystem> #include <iterator> #include <map> -#include <mutex> #include <optional> #include <qdmi/client.h> #include <regex>
♻️ Duplicate comments (2)
src/fomac/FoMaC.cpp (2)
812-884: CheckQDMI_session_allocreturn value.The constructor doesn't verify whether
QDMI_session_allocsucceeded. If allocation fails,session_could be in an invalid state, leading to undefined behavior in subsequentQDMI_session_set_parameterorQDMI_session_initcalls.Session::Session(const SessionConfig& config) { - QDMI_session_alloc(&session_); + throwIfError(QDMI_session_alloc(&session_), "Allocating QDMI session"); // Set and validate parameters from config
839-845: URL validation regex is permissive.The regex allows malformed URLs like
http://(missing domain) orhttp://.com. Consider a stricter pattern that validates basic URL structure.A more robust pattern would ensure at least one valid domain character after the scheme:
// Validate URL format static const std::regex URL_PATTERN( - R"(^https?://[a-zA-Z0-9\-._~:/?#\[\]@!$&'()*+,;=%]+$)"); + R"(^https?://[a-zA-Z0-9][a-zA-Z0-9\-._~:/?#\[\]@!$&'()*+,;=%]*$)");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
bindings/fomac/fomac.cpp(3 hunks)bindings/na/fomac/fomac.cpp(2 hunks)include/mqt-core/fomac/FoMaC.hpp(5 hunks)include/mqt-core/na/fomac/Device.hpp(4 hunks)python/mqt/core/fomac.pyi(2 hunks)python/mqt/core/plugins/qiskit/provider.py(1 hunks)src/fomac/FoMaC.cpp(21 hunks)src/na/fomac/Device.cpp(10 hunks)test/fomac/test_fomac.cpp(7 hunks)test/na/fomac/test_fomac.cpp(2 hunks)test/python/fomac/test_fomac.py(4 hunks)test/python/plugins/qiskit/conftest.py(1 hunks)test/python/plugins/qiskit/test_backend.py(2 hunks)test/python/plugins/qiskit/test_provider.py(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
test/python/plugins/qiskit/conftest.pypython/mqt/core/plugins/qiskit/provider.pytest/python/plugins/qiskit/test_backend.pytest/na/fomac/test_fomac.cppbindings/na/fomac/fomac.cpptest/fomac/test_fomac.cpptest/python/fomac/test_fomac.pyinclude/mqt-core/na/fomac/Device.hppsrc/na/fomac/Device.cpp
📚 Learning: 2025-11-05T07:42:45.507Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
Applied to files:
python/mqt/core/plugins/qiskit/provider.pytest/python/plugins/qiskit/test_backend.pytest/na/fomac/test_fomac.cppbindings/na/fomac/fomac.cpptest/fomac/test_fomac.cppinclude/mqt-core/fomac/FoMaC.hppinclude/mqt-core/na/fomac/Device.hppsrc/na/fomac/Device.cppsrc/fomac/FoMaC.cpp
📚 Learning: 2025-10-17T11:09:50.147Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Applied to files:
test/na/fomac/test_fomac.cpptest/fomac/test_fomac.cpp
📚 Learning: 2025-11-26T10:29:36.435Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/fomac/test_fomac.py:54-66
Timestamp: 2025-11-26T10:29:36.435Z
Learning: In test/python/fomac/test_fomac.py, the `ddsim_device` fixture returns `Device | None` even though it only returns a `Device` or calls `pytest.skip()`. The `| None` annotation is required for the type checker to pass, as type checkers may not recognize `pytest.skip()` as a function that never returns.
Applied to files:
test/python/fomac/test_fomac.py
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
include/mqt-core/fomac/FoMaC.hpp
📚 Learning: 2025-10-09T13:13:51.224Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Applied to files:
src/na/fomac/Device.cpp
🧬 Code graph analysis (9)
python/mqt/core/plugins/qiskit/provider.py (2)
include/mqt-core/fomac/FoMaC.hpp (4)
Session(725-725)Session(730-730)Session(733-733)Session(737-737)python/mqt/core/fomac.pyi (2)
Session(37-95)get_devices(90-95)
python/mqt/core/fomac.pyi (2)
include/mqt-core/fomac/FoMaC.hpp (5)
Session(725-725)Session(730-730)Session(733-733)Session(737-737)Device(636-636)src/fomac/FoMaC.cpp (3)
Session(812-884)Session(886-890)Session(892-894)
test/python/plugins/qiskit/test_backend.py (2)
test/python/plugins/qiskit/test_provider.py (1)
mock_get_devices(83-84)python/mqt/core/fomac.pyi (1)
Session(37-95)
test/na/fomac/test_fomac.cpp (3)
src/fomac/FoMaC.cpp (2)
getDevices(907-917)getDevices(907-907)src/na/fomac/Device.cpp (2)
getDevices(579-588)getDevices(579-579)test/fomac/test_fomac.cpp (2)
getDevices(624-627)getDevices(624-624)
test/python/plugins/qiskit/test_provider.py (3)
test/python/plugins/qiskit/test_backend.py (2)
mock_get_devices(269-270)mock_get_devices(301-302)src/fomac/FoMaC.cpp (3)
Session(812-884)Session(886-890)Session(892-894)python/mqt/core/fomac.pyi (1)
Session(37-95)
test/python/fomac/test_fomac.py (1)
python/mqt/core/fomac.pyi (3)
Device(153-266)Session(37-95)get_devices(90-95)
include/mqt-core/fomac/FoMaC.hpp (2)
src/fomac/FoMaC.cpp (3)
Session(812-884)Session(886-890)Session(892-894)include/mqt-core/na/fomac/Device.hpp (2)
device(123-124)Session(164-164)
bindings/fomac/fomac.cpp (3)
include/mqt-core/na/device/Device.hpp (2)
site(177-179)operation(185-189)src/fomac/FoMaC.cpp (36)
getDevices(907-917)getDevices(907-907)getId(542-552)getId(542-542)getProgramFormat(554-560)getProgramFormat(554-554)getProgram(562-573)getProgram(562-562)getNumShots(575-581)getNumShots(575-575)getName(257-259)getName(257-257)getName(290-295)getName(290-292)getName(390-392)getName(390-390)isZone(269-272)isZone(269-269)getSites(343-357)getSites(343-344)getSites(405-415)getSites(405-405)getVersion(393-395)getVersion(393-393)getStatus(396-398)getStatus(396-396)getLibraryVersion(399-401)getLibraryVersion(399-399)getRegularSites(416-422)getRegularSites(416-416)getZones(423-430)getZones(423-423)getOperations(431-442)getOperations(431-431)getLengthUnit(465-468)getLengthUnit(465-465)include/mqt-core/na/fomac/Device.hpp (1)
device(123-124)
src/fomac/FoMaC.cpp (2)
include/mqt-core/na/device/Device.hpp (7)
param(151-152)param(219-220)prop(118-119)prop(170-171)prop(226-227)prop(306-307)result(257-258)include/mqt-core/fomac/FoMaC.hpp (10)
queryProperty(354-354)queryProperty(438-441)queryProperty(584-584)queryProperty(704-705)throwIfError(172-182)throwIfError(172-172)Session(725-725)Session(730-730)Session(733-733)Session(737-737)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
test/fomac/test_fomac.cpp
[warning] 17-17: test/fomac/test_fomac.cpp:17:1 [misc-include-cleaner]
included header fstream is not used directly
[warning] 16-16: test/fomac/test_fomac.cpp:16:1 [misc-include-cleaner]
included header filesystem is not used directly
[warning] 26-26: test/fomac/test_fomac.cpp:26:1 [misc-include-cleaner]
included header thread is not used directly
[warning] 624-624: test/fomac/test_fomac.cpp:624:13 [misc-use-internal-linkage]
function 'getDevices' can be made static or moved into an anonymous namespace to enforce internal linkage
bindings/fomac/fomac.cpp
[warning] 34-34: bindings/fomac/fomac.cpp:34:25 [misc-include-cleaner]
no header providing "std::optional" is directly included
src/fomac/FoMaC.cpp
[warning] 20-20: src/fomac/FoMaC.cpp:20:1 [misc-include-cleaner]
included header mutex is not used directly
[warning] 81-81: src/fomac/FoMaC.cpp:81:11 [clang-diagnostic-switch]
enumeration value 'QDMI_SESSION_PARAMETER_MAX' not handled in switch
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
🔇 Additional comments (34)
include/mqt-core/fomac/FoMaC.hpp (2)
184-203: LGTM!The
SessionConfigstruct provides a clean, flexible API for authentication parameters. Usingstd::optional<std::string>for all fields allows users to specify only the parameters they need. This approach aligns with the consensus from past review discussions regarding the builder pattern for C++ and kwargs translation to Python.
719-741: LGTM!The
Sessionclass API is well-designed:
- Explicit constructor with default config enables both simple usage (
Session()) and authenticated sessions (Session(config))- Copy semantics correctly deleted to prevent double-free of QDMI session handle
- Move semantics enabled for safe ownership transfer
- Instance-based
getDevices()aligns with the session-per-instance modelThe documentation accurately reflects that the session is initialized in the constructor.
test/na/fomac/test_fomac.cpp (2)
61-61: Consistent with line 45.Same pattern as the first test - uses
Session::getDevices()which should resolve tona::Session::getDevices().
45-45: TheSession::getDevices()call is correct.In this namespace (
na),Sessionrefers tona::Session, which is defined ininclude/mqt-core/na/fomac/Device.hpp(line 26) and inherits fromfomac::Session. Thena::Session::getDevices()method is indeed static (line 167), returningstd::vector<Device>. The callSession::getDevices().front()correctly invokes this static method and retrieves the first device, which is properly assigned tonlohmann::json.test/python/plugins/qiskit/conftest.py (1)
447-455: LGTM!The fixture correctly migrates to the session-based API. Creating a
Sessioninstance and callingget_devices()aligns with the new design where sessions manage device discovery. The session object going out of scope after returning devices is appropriate sinceDeviceobjects are independent of the session lifecycle for query operations.test/python/plugins/qiskit/test_provider.py (1)
79-90: LGTM!The monkeypatch correctly targets
fomac.Session.get_devicesinstead of a module-level function. The mock function's_selfparameter properly accepts the instance argument that will be passed when the method is called, though it's unused since we just want to return an empty list. This pattern is consistent with similar mocks intest_backend.py.test/python/plugins/qiskit/test_backend.py (2)
268-272: LGTM! Correct approach for patching the Session instance method.The mock function signature with
_self: objectparameter correctly matches the bound method pattern forSession.get_devices(). This is consistent with the pattern used intest_provider.py.
300-304: Consistent monkeypatching pattern applied.The second occurrence follows the same correct pattern for mocking
Session.get_devices. Both test functions are now properly aligned with the new Session-based API.python/mqt/core/fomac.pyi (2)
12-17: Exports updated correctly for the new API.The
__all__list properly exportsDevice,Job,ProgramFormat, andSession. This provides a clean public API surface where users create aSessionto access devices.
37-95: Well-documented Session class with comprehensive authentication options.The
Sessionclass API is cleanly designed with:
- Keyword-only parameters preventing positional argument errors
- Appropriate
RuntimeErrordocumentation for validation failures- Helpful usage examples in the docstring
The implementation aligns with the C++
SessionConfigstructure fromsrc/fomac/FoMaC.cppwhich validatesauth_fileexistence andauth_urlformat.test/python/fomac/test_fomac.py (2)
20-27: Helper function correctly implements the Session-based device retrieval.The
_get_devices()function properly creates aSessioninstance and returns devices. Note that this function is called during pytest collection (viaparams=_get_devices()), creating a new Session for each parametrized fixture. This is functionally correct given the test structure.
30-30: Fixture parameterization correctly migrated to Session-based API.All four fixtures (
device,device_and_site,device_and_operation, andddsim_device) now use_get_devices()consistently. Based on learnings, at least one FoMaC device is always available during testing, so this pattern will work reliably.Also applies to: 40-40, 52-52, 71-71
include/mqt-core/na/fomac/Device.hpp (3)
23-26: Class refactoring from FoMaC to Session is correctly implemented.The class inheritance chain is properly updated to
na::Session : public fomac::Session, maintaining the established pattern.
34-34: Device type references consistently updated throughout.All references to
fomac::FoMaC::Devicehave been correctly updated tofomac::Session::Device, including:
- The inheritance declaration
- Constructor parameter
- Factory method parameter and documentation
Also applies to: 82-88, 122-123
164-168: The staticgetDevices()method is correct by design.The
na::Sessionclass intentionally deletes its default constructor and provides a staticgetDevices()method that internally creates afomac::Sessioninstance. This design is appropriate for neutral-atom device discovery, which does not require users to manage session instances directly. The method queriesQDMI_SESSION_PROPERTY_DEVICESthrough an internal session, providing a convenient facade for device enumeration without requiring explicit session management.bindings/na/fomac/fomac.cpp (3)
44-45: Device binding correctly updated to Session-based types.The PyBind11 class binding properly declares
na::Session::Devicewithfomac::Session::Deviceas its base class, enabling proper inheritance in Python.
94-103: Property and method bindings consistently use Session::Device.The
traps,t1,t2properties and__repr__method are all correctly updated to operate onna::Session::Device/fomac::Session::Devicetypes.
107-109: NA module exposes Session-based device access.The
devicesfunction andtry_create_from_devicestatic method are correctly bound to the newna::Sessionnamespace. Note that this NA-specificdevices()function differs from the basefomacmodule'sSession.get_devices()instance method - this static function is appropriate for the neutral-atom extension that filters/wraps generic devices.test/fomac/test_fomac.cpp (3)
31-68: LGTM!The migration of test fixtures from
FoMaC::Device,FoMaC::Device::Site,FoMaC::Device::Operation, andFoMaC::Jobto theirSession::equivalents is consistent and correctly updates all member types and method return types.
604-616: LGTM!The new
AuthenticationTestcomprehensively covers string representations for allQDMI_SESSION_PARAMETER_*constants, which aligns with the newtoStringoverload added insrc/fomac/FoMaC.cpp.
629-669: LGTM!The parameterized test instantiations correctly use the updated
Session::Devicetype forTestParamInfo, maintaining consistency with the refactored API.src/na/fomac/Device.cpp (5)
41-46: LGTM!Documentation and signature correctly updated to reference
Session::Device::Sitetypes.
113-172: LGTM!The device initialization methods (
initNameFromDevice,initMinAtomDistanceFromDevice,initQubitsNumFromDevice,initLengthUnitFromDevice,initDurationUnitFromDevice,initDecoherenceTimesFromDevice) are correctly updated to theSession::Devicenamespace with no logic changes.
173-296: LGTM!The
initTrapsfromDevicemethod correctly migrates toSession::Devicenamespace. The lattice reconstruction logic remains intact.
297-576: LGTM!The
initOperationsFromDevicemethod correctly updates allfomac::Session::Device::Operationandfomac::Session::Device::Sitetype references in loops and lambdas. The shuttling operation parsing and zoned/non-zoned operation handling logic is preserved.
577-588: LGTM!The constructor correctly delegates to the base
fomac::Session::Device, andSession::getDevices()properly creates a localfomac::Sessionto retrieve and filter devices usingDevice::tryCreateFromDevice.bindings/fomac/fomac.cpp (3)
29-55: LGTM!The
Sessionbinding correctly exposes a constructor with optional authentication parameters (token,auth_file,auth_url,username,password,project_id) that map toSessionConfigfields. The use ofdefaultConfigfor default argument values ensures backward compatibility for users who don't need authentication.
57-77: LGTM!The
Jobbindings are correctly migrated toSession::Jobwith all methods and properties properly exposed.
111-214: LGTM!The
Device,Site, andOperationbindings are comprehensively updated to useSession::Device,Session::Device::Site, andSession::Device::Operationtypes respectively. All properties and methods are properly exposed with consistent naming conventions.src/fomac/FoMaC.cpp (5)
248-490: LGTM!All
Site,Operation, andDeviceaccessor methods are correctly migrated to theSession::Devicenamespace. The implementation logic remains unchanged, only the class qualification has been updated.
492-518: LGTM!The
submitJobmethod correctly creates a Job with RAII semantics and properly sets all job parameters before submission.
520-810: LGTM!All
Jobaccessor methods (check,wait,cancel,getId,getProgramFormat,getProgram,getNumShots,getShots,getCounts,getDenseStateVector,getDenseProbabilities,getSparseStateVector,getSparseProbabilities) are correctly migrated toSession::Jobnamespace with robust error handling and size validation.
886-905: LGTM!The destructor correctly handles null session pointers, and the move constructor/assignment operator properly transfer ownership by nullifying the source. Since there's no mutex member in the visible Session class definition, the current implementation is appropriate.
907-917: LGTM!The
getDevicesmethod correctly queries the session property and transforms QDMI devices intoSession::Devicewrappers using theTokenpattern for controlled construction.
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Alright, I looked a little bit more deeply into this the last couple of hours and pushed a bunch of commits that resulted from that investigation.
I tried to keep them as self-contained as possible and added explicit changelog items wherever I saw fit.
I am happy with this for now and hope CI will be happy as well.
Tagging this for auto-merge 🚀
Signed-off-by: burgholzer <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/qdmi/dd/Device.cpp(10 hunks)src/qdmi/na/Device.cpp(10 hunks)src/qdmi/sc/Device.cpp(9 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
📚 Learning: 2025-11-05T07:42:45.507Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
Applied to files:
src/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppsrc/qdmi/dd/Device.cpp
📚 Learning: 2025-10-17T11:09:50.147Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Applied to files:
src/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppsrc/qdmi/dd/Device.cpp
📚 Learning: 2025-11-04T19:50:14.489Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/helpers/test_utils.cpp:103-118
Timestamp: 2025-11-04T19:50:14.489Z
Learning: In the munich-quantum-toolkit/core QDMI device tests, when QDMI APIs return a string result size `ks` that includes a null terminator, the pattern `std::string keys(ks - 1, '\0'); ... device_job_get_results(..., ks, keys.data(), nullptr)` is correct and safe. The string allocates `ks` bytes total (ks-1 content + implicit null), and the API writing `ks` bytes (including trailing null) fits exactly into this space without buffer overrun.
Applied to files:
src/qdmi/sc/Device.cppsrc/qdmi/na/Device.cpp
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
src/qdmi/na/Device.cppsrc/qdmi/dd/Device.cpp
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
src/qdmi/na/Device.cpp
🧬 Code graph analysis (2)
src/qdmi/sc/Device.cpp (2)
src/qdmi/dd/Device.cpp (4)
finalize(239-249)finalize(239-239)get(251-256)get(251-251)src/qdmi/na/Device.cpp (4)
finalize(134-144)finalize(134-134)get(146-151)get(146-146)
src/qdmi/dd/Device.cpp (2)
include/mqt-core/qdmi/dd/Device.hpp (7)
prop(120-121)prop(186-187)prop(297-298)Device(64-64)Device(73-73)Device(76-76)Device(80-80)src/qdmi/na/Device.cpp (7)
Device(107-118)initialize(120-132)initialize(120-120)finalize(134-144)finalize(134-134)get(146-151)get(146-146)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
🔇 Additional comments (12)
src/qdmi/dd/Device.cpp (3)
209-214: IS_INVALID_ARGUMENT macro correctly centralizes validation logic.The macro properly rejects property values
>= prefix##_MAXwhile allowingCUSTOM1throughCUSTOM5extension points. This addresses the previous review feedback about repetitive validation and aligns with the pattern used in the NA device implementation.
218-256: Reference-counted singleton lifecycle is correctly implemented.The implementation matches the pattern in
src/qdmi/na/Device.cppand properly handles concurrent initialization with atomic CAS. Thefinalize()conditionprev > 1correctly ensures the instance is only deleted when the last reference is released.
323-326: LGTM!Using a dedicated
rngMutex_for the RNG state is appropriate - it separates concerns fromsessionsMutex_which guards the sessions map, avoiding unnecessary contention.src/qdmi/sc/Device.cpp (7)
105-151: Reference-counted singleton lifecycle is consistent with other device implementations.The
initialize(),finalize(), andget()implementations match the pattern insrc/qdmi/dd/Device.cppandsrc/qdmi/na/Device.cpp, ensuring uniform behavior across all QDMI device types.
170-175: Validation logic properly extended to support custom properties.The
IS_INVALID_ARGUMENTmacro correctly rejects properties>= QDMI_DEVICE_PROPERTY_MAXwhile explicitly allowingCUSTOM1throughCUSTOM5, enabling device-specific property extensions.
221-232: LGTM!Session parameter validation correctly uses the macro and maintains proper state checking (must be in
ALLOCATEDstate to set parameters, per the learning about QDMI session parameter timing).
288-308: Job parameter and property validation is consistent.Both
setParameterandqueryPropertymethods use the same validation pattern with their respective property type prefixes.
329-339: LGTM!The
getResultsvalidation properly usesQDMI_JOB_RESULTprefix for the macro.
345-355: Site property validation is correct.The validation pattern is consistent with other property query methods.
408-418: Operation property validation is correct.The validation includes the expected checks for
sites,params,value, and the property enum validity.src/qdmi/na/Device.cpp (2)
171-176: LGTM: Consistent validation across property/parameter queries.The refactor to use the
IS_INVALID_ARGUMENTmacro centralizes validation logic across all query and parameter-setting methods. The consistent pattern of checking both(value != nullptr && size == 0)andIS_INVALID_ARGUMENT(...)improves maintainability and reduces code duplication.Also applies to: 231-237, 299-307, 309-318, 340-349, 382-388, 547-559
96-100: Verify that CUSTOM symbol values respect the IS_INVALID_ARGUMENT logic across all prefixes.The
IS_INVALID_ARGUMENTmacro assumes allCUSTOM1–CUSTOM5values are>= PREFIX_MAXfor each of the seven prefixes it validates. If anyCUSTOMvalue falls below its prefix's_MAX, valid custom properties would be incorrectly rejected.Since the enum definitions are externally generated from the
qdmi::qdmipackage and not present in this repository, the assumptions cannot be verified locally. Confirm that the external QDMI enum definitions follow this constraint, or add a static assertion or compile-time check to enforce it.
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.clang-tidy(1 hunks)include/mqt-core/qdmi/dd/Device.hpp(4 hunks)include/mqt-core/qdmi/na/Device.hpp(3 hunks)include/mqt-core/qdmi/sc/Device.hpp(3 hunks)src/qdmi/dd/Device.cpp(10 hunks)src/qdmi/na/Device.cpp(10 hunks)src/qdmi/sc/Device.cpp(10 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1355
File: src/qdmi/sc/Device.cpp:97-102
Timestamp: 2025-12-07T09:10:31.820Z
Learning: In the munich-quantum-toolkit/core repository, duplication of QDMI-related macros (such as IS_INVALID_ARGUMENT) across device implementations (e.g., in src/qdmi/sc/Device.cpp and src/qdmi/dd/Device.cpp) is acceptable as a temporary measure. The preferred long-term solution is to upstream these macros to the QDMI repository rather than creating local shared headers, so they can be reused across all dependent projects.
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
📚 Learning: 2025-12-07T09:10:31.820Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1355
File: src/qdmi/sc/Device.cpp:97-102
Timestamp: 2025-12-07T09:10:31.820Z
Learning: In the munich-quantum-toolkit/core repository, duplication of QDMI-related macros (such as IS_INVALID_ARGUMENT) across device implementations (e.g., in src/qdmi/sc/Device.cpp and src/qdmi/dd/Device.cpp) is acceptable as a temporary measure. The preferred long-term solution is to upstream these macros to the QDMI repository rather than creating local shared headers, so they can be reused across all dependent projects.
Applied to files:
src/qdmi/sc/Device.cppinclude/mqt-core/qdmi/dd/Device.hppsrc/qdmi/na/Device.cppinclude/mqt-core/qdmi/sc/Device.hppinclude/mqt-core/qdmi/na/Device.hppsrc/qdmi/dd/Device.cpp
📚 Learning: 2025-11-05T07:42:45.507Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
Applied to files:
src/qdmi/sc/Device.cppinclude/mqt-core/qdmi/dd/Device.hppsrc/qdmi/na/Device.cppinclude/mqt-core/qdmi/na/Device.hppsrc/qdmi/dd/Device.cpp
📚 Learning: 2025-10-17T11:09:50.147Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Applied to files:
src/qdmi/sc/Device.cppinclude/mqt-core/qdmi/dd/Device.hppsrc/qdmi/na/Device.cppinclude/mqt-core/qdmi/sc/Device.hppinclude/mqt-core/qdmi/na/Device.hppsrc/qdmi/dd/Device.cpp
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
src/qdmi/sc/Device.cppsrc/qdmi/na/Device.cpp
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
src/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppinclude/mqt-core/qdmi/sc/Device.hppinclude/mqt-core/qdmi/na/Device.hpp
📚 Learning: 2025-11-04T19:50:14.489Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/helpers/test_utils.cpp:103-118
Timestamp: 2025-11-04T19:50:14.489Z
Learning: In the munich-quantum-toolkit/core QDMI device tests, when QDMI APIs return a string result size `ks` that includes a null terminator, the pattern `std::string keys(ks - 1, '\0'); ... device_job_get_results(..., ks, keys.data(), nullptr)` is correct and safe. The string allocates `ks` bytes total (ks-1 content + implicit null), and the API writing `ks` bytes (including trailing null) fits exactly into this space without buffer overrun.
Applied to files:
src/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppinclude/mqt-core/qdmi/sc/Device.hppinclude/mqt-core/qdmi/na/Device.hpp
📚 Learning: 2025-12-05T17:45:37.602Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
Applied to files:
include/mqt-core/qdmi/na/Device.hpp
🧬 Code graph analysis (2)
src/qdmi/sc/Device.cpp (1)
src/qdmi/na/Device.cpp (4)
finalize(125-136)finalize(125-125)get(137-142)get(137-137)
src/qdmi/dd/Device.cpp (4)
include/mqt-core/qdmi/dd/Device.hpp (7)
prop(123-124)prop(189-190)prop(300-301)Device(64-64)Device(76-76)Device(79-79)Device(83-83)include/mqt-core/qdmi/sc/Device.hpp (8)
prop(111-112)prop(163-164)prop(224-225)prop(283-284)Device(52-52)Device(64-64)Device(67-67)Device(71-71)include/mqt-core/qdmi/Driver.hpp (3)
prop(243-244)prop(306-307)prop(386-387)src/qdmi/na/Device.cpp (5)
Device(104-115)initialize(116-124)initialize(116-116)finalize(125-136)finalize(125-125)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
🔇 Additional comments (11)
include/mqt-core/qdmi/dd/Device.hpp (2)
67-72: Reference-counted singleton with intentionally leaked mutex looks correct.The pattern using
inline staticmembers with a dynamically allocated mutex that's intentionally never deleted is a well-known approach to avoid the static deinitialization order fiasco. The NOLINT annotation properly documents this design decision.
87-101: Documentation clearly describes the initialize/finalize contract.The updated documentation properly explains the reference-counted lifecycle and the requirement for paired
initialize()/finalize()calls.include/mqt-core/qdmi/sc/Device.hpp (1)
55-60: Consistent singleton pattern with dd Device implementation.The reference-counted singleton with the intentionally leaked mutex follows the same pattern as the dd Device, ensuring consistency across QDMI device implementations.
src/qdmi/na/Device.cpp (2)
96-101: IS_INVALID_ARGUMENT macro correctly validates property/parameter bounds.The macro properly checks that values are within valid ranges, accounting for the
_MAXsentinel and_CUSTOM1through_CUSTOM5extension points. Based on learnings, the duplication across device implementations is acceptable as a temporary measure until these macros are upstreamed to the QDMI repository.
116-142: Mutex-guarded singleton lifecycle is correctly implemented.The
initialize()andfinalize()methods properly usestd::scoped_lockto guard access torefCount_andinstance_. The comment explaining why the mutex is intentionally leaked to avoid static deinitialization issues is helpful.include/mqt-core/qdmi/na/Device.hpp (1)
73-78: Consistent singleton pattern across all QDMI device implementations.The na Device follows the same reference-counted singleton pattern with an intentionally leaked mutex as the dd and sc devices, ensuring consistency across the codebase.
src/qdmi/dd/Device.cpp (4)
209-213: IS_INVALID_ARGUMENT macro correctly centralizes argument validation.The macro properly validates that enum values are within the valid range, checking against
_MAXand allowing_CUSTOM1through_CUSTOM5extension points. This centralized approach reduces code duplication and ensures consistent validation across all query and set parameter paths.
220-246: Mutex-guarded singleton lifecycle implementation is correct.The implementation properly guards both
initialize()andfinalize()withstd::scoped_lock, ensuring thread-safe reference counting. Theget()method correctly asserts thatinstance_is initialized, which aligns with the documented usage contract requiringinitialize()to be called beforeget().
312-314: RNG access correctly guarded by dedicated mutex.The
generateUniqueID()method now usesrngMutex_instead of a more general mutex, which is appropriate for protecting the RNG state. This aligns with the header changes whererngMutex_is declared separately fromsessionsMutex_.
446-455: Program format validation is thorough.The code correctly validates the program format using
IS_INVALID_ARGUMENTfirst, then checks for specifically supported formats (QASM2 and QASM3). This two-step validation ensures both that the input is a valid enum value and that it's a format this device actually supports.src/qdmi/sc/Device.cpp (1)
120-146: Reference-counted singleton pattern is consistent withna/Device.cpp.The mutex-guarded initialization/finalization with reference counting and the unlocked
get()accessor match the established pattern inna/Device.cppexactly. The intentional mutex leak to avoid static deinitialization order issues is correctly documented.The design assumes callers maintain proper reference counting discipline:
initialize()must precedeget(), andget()must not be called after the finalfinalize(). The assert provides debug-time verification of this contract.
This reverts commit 0b74445.
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
include/mqt-core/qdmi/dd/Device.hpp(2 hunks)include/mqt-core/qdmi/na/Device.hpp(1 hunks)include/mqt-core/qdmi/sc/Device.hpp(1 hunks)src/qdmi/dd/Device.cpp(11 hunks)src/qdmi/na/Device.cpp(11 hunks)src/qdmi/sc/Device.cpp(10 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1355
File: src/qdmi/sc/Device.cpp:97-102
Timestamp: 2025-12-07T09:10:31.820Z
Learning: In the munich-quantum-toolkit/core repository, duplication of QDMI-related macros (such as IS_INVALID_ARGUMENT) across device implementations (e.g., in src/qdmi/sc/Device.cpp and src/qdmi/dd/Device.cpp) is acceptable as a temporary measure. The preferred long-term solution is to upstream these macros to the QDMI repository rather than creating local shared headers, so they can be reused across all dependent projects.
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
📚 Learning: 2025-12-07T09:10:31.820Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1355
File: src/qdmi/sc/Device.cpp:97-102
Timestamp: 2025-12-07T09:10:31.820Z
Learning: In the munich-quantum-toolkit/core repository, duplication of QDMI-related macros (such as IS_INVALID_ARGUMENT) across device implementations (e.g., in src/qdmi/sc/Device.cpp and src/qdmi/dd/Device.cpp) is acceptable as a temporary measure. The preferred long-term solution is to upstream these macros to the QDMI repository rather than creating local shared headers, so they can be reused across all dependent projects.
Applied to files:
include/mqt-core/qdmi/na/Device.hppsrc/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppsrc/qdmi/dd/Device.cppinclude/mqt-core/qdmi/dd/Device.hppinclude/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-10-17T11:09:50.147Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Applied to files:
include/mqt-core/qdmi/na/Device.hppsrc/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppsrc/qdmi/dd/Device.cppinclude/mqt-core/qdmi/dd/Device.hppinclude/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-11-05T07:42:45.507Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
Applied to files:
include/mqt-core/qdmi/na/Device.hppsrc/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppsrc/qdmi/dd/Device.cppinclude/mqt-core/qdmi/dd/Device.hpp
📚 Learning: 2025-12-05T17:45:37.602Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
Applied to files:
include/mqt-core/qdmi/na/Device.hpp
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
include/mqt-core/qdmi/na/Device.hppsrc/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppinclude/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-11-04T19:50:14.489Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/helpers/test_utils.cpp:103-118
Timestamp: 2025-11-04T19:50:14.489Z
Learning: In the munich-quantum-toolkit/core QDMI device tests, when QDMI APIs return a string result size `ks` that includes a null terminator, the pattern `std::string keys(ks - 1, '\0'); ... device_job_get_results(..., ks, keys.data(), nullptr)` is correct and safe. The string allocates `ks` bytes total (ks-1 content + implicit null), and the API writing `ks` bytes (including trailing null) fits exactly into this space without buffer overrun.
Applied to files:
include/mqt-core/qdmi/na/Device.hppsrc/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppinclude/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
src/qdmi/sc/Device.cppsrc/qdmi/na/Device.cpp
🧬 Code graph analysis (2)
src/qdmi/sc/Device.cpp (3)
include/mqt-core/qdmi/sc/Device.hpp (4)
prop(84-85)prop(136-137)prop(197-198)prop(256-257)src/qdmi/na/Device.cpp (2)
get(115-121)get(115-115)src/qdmi/dd/Device.cpp (2)
get(220-226)get(220-220)
src/qdmi/na/Device.cpp (4)
include/mqt-core/qdmi/na/Device.hpp (4)
prop(102-103)prop(154-155)prop(210-211)prop(290-291)include/mqt-core/qdmi/Driver.hpp (1)
instance(424-427)src/qdmi/sc/Device.cpp (2)
get(119-125)get(119-119)src/qdmi/dd/Device.cpp (2)
get(220-226)get(220-220)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (8)
include/mqt-core/qdmi/dd/Device.hpp (1)
49-49: Mutex member initialization change is behavior-preserving.Dropping the
{}brace-init onsessionsMutex_,rngMutex_, andjobsMutex_keeps the same default construction semantics forstd::mutex; no functional impact and consistent with usage in the .cpp.Also applies to: 54-54, 130-130
src/qdmi/sc/Device.cpp (2)
96-100: Centralized argument validation viaIS_INVALID_ARGUMENTlooks correct.The new
IS_INVALID_ARGUMENTmacro plus the(value != nullptr && size == 0)guards give consistent validation across device/session/job/site/operation properties and cleanly admit the CUSTOM1..CUSTOM5 ranges while rejecting out-of-range enums. This matches the patterns used in the other QDMI devices and improves robustness without changing supported behavior.Also applies to: 146-147, 198-201, 267-269, 277-280, 308-311, 322-325, 387-392
119-125: Leaked function-local singleton and no-op finalize avoid lifecycle races.Switching
Device::get()to a leaked function-local static pointer and makingMQT_SC_QDMI_device_finalize()a no-op removes the previous explicit destruction path and any related use-after-free risk.MQT_SC_QDMI_device_initialize()now simply ensures the singleton exists, which is consistent with the NA and DDSIM devices and acceptable for a process-lifetime backend.Also applies to: 475-482
src/qdmi/na/Device.cpp (2)
51-56: String/list helper macros andIS_INVALID_ARGUMENTusage are consistent and safe.
ADD_STRING_PROPERTY’s size check plus the adjustedSTRNCPYkeep string writes bounded and explicitly null-terminated, andADD_LIST_PROPERTYworks correctly for both vectors and thestd::span-based flattening. The newIS_INVALID_ARGUMENTchecks (together with(value != nullptr && size == 0)) provide uniform, stricter validation across device/session/job/site/operation enums while still allowing CUSTOM1..CUSTOM5 to flow through as “potentially supported” properties.Also applies to: 58-75, 96-99, 140-145, 200-206, 269-273, 281-285, 313-316, 351-357, 521-528
115-120: NA device singleton and C API lifecycle now match the other backends.
Device::get()now uses a leaked function-local static pointer, and the NA QDMIdevice_initialize/device_finalizeC functions simply ensure initialization and then no-op on teardown. This removes manual destruction from the hot path, avoids lifetime races, and keeps behavior in line with the SC and DDSIM devices.Also applies to: 642-649
src/qdmi/dd/Device.cpp (3)
210-213: Unified argument validation viaIS_INVALID_ARGUMENTis correct and improves robustness.Using
IS_INVALID_ARGUMENTtogether with the(value|data != nullptr && size == 0)checks gives consistent, stricter input validation for device properties, session parameters, site/operation properties, job parameters, and job results. The macro’s whitelist for CUSTOM1..CUSTOM5 keeps those legal while rejecting out-of-range enums, and unsupported-but-valid enums now sensibly fall through toQDMI_ERROR_NOTSUPPORTED.Also applies to: 248-252, 320-325, 367-368, 387-388, 417-420, 457-458, 723-726
292-295: RNG access is now properly synchronized.Wrapping
dis_(rng_)in astd::scoped_lockonrngMutex_eliminates the data race onstd::mt19937_64when IDs are generated concurrently, without impacting observable behavior.
221-225: DDSIM device singleton and C API lifecycle refactor look sound.
Device::get()now uses a leaked function-local static pointer, andMQT_DDSIM_QDMI_device_initialize()simply forces construction while*_device_finalize()is a no-op. This avoids any use-after-free hazards around late jobs referencing the device while still keeping the lifetime bounded to the process, and it aligns the DDSIM backend with the NA/SC implementations.Also applies to: 762-769
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qdmi/na/Device.cpp (1)
54-71: Add mutex protection for session management to prevent data races.The
sessions_map insessionAllocandsessionFreeis unprotected while the equivalentdd::Deviceimplementation usessessionsMutex_to guard these operations. Since both are singleton instances exposed via C API and subject to concurrent access, the lack of synchronization in the NA implementation creates a potential data race. Add amutable std::mutex sessionsMutex_member to the Device class and protect the map operations withstd::scoped_lockas done indd::Device.
♻️ Duplicate comments (1)
include/mqt-core/fomac/FoMaC.hpp (1)
23-23: Unused<mutex>include still present.A past review flagged that
<mutex>is not used in this file or its implementation. This include should be removed to reduce compilation overhead.-#include <mutex>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
CHANGELOG.md(4 hunks)include/mqt-core/fomac/FoMaC.hpp(13 hunks)include/mqt-core/qdmi/Common.hpp(1 hunks)include/mqt-core/qdmi/na/Device.hpp(0 hunks)include/mqt-core/qdmi/sc/Device.hpp(0 hunks)src/fomac/CMakeLists.txt(1 hunks)src/fomac/FoMaC.cpp(13 hunks)src/na/fomac/Device.cpp(17 hunks)src/qdmi/CMakeLists.txt(2 hunks)src/qdmi/Common.cpp(1 hunks)src/qdmi/Driver.cpp(3 hunks)src/qdmi/dd/CMakeLists.txt(1 hunks)src/qdmi/dd/Device.cpp(12 hunks)src/qdmi/na/CMakeLists.txt(4 hunks)src/qdmi/na/Device.cpp(10 hunks)src/qdmi/sc/CMakeLists.txt(2 hunks)src/qdmi/sc/Device.cpp(10 hunks)test/fomac/test_fomac.cpp(8 hunks)test/qdmi/dd/results_sampling_test.cpp(1 hunks)
💤 Files with no reviewable changes (2)
- include/mqt-core/qdmi/sc/Device.hpp
- include/mqt-core/qdmi/na/Device.hpp
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1355
File: src/qdmi/sc/Device.cpp:97-102
Timestamp: 2025-12-07T09:10:31.820Z
Learning: In the munich-quantum-toolkit/core repository, duplication of QDMI-related macros (such as IS_INVALID_ARGUMENT) across device implementations (e.g., in src/qdmi/sc/Device.cpp and src/qdmi/dd/Device.cpp) is acceptable as a temporary measure. The preferred long-term solution is to upstream these macros to the QDMI repository rather than creating local shared headers, so they can be reused across all dependent projects.
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
📚 Learning: 2025-10-14T14:37:38.047Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-12-05T17:45:37.602Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
Applied to files:
CHANGELOG.mdsrc/qdmi/dd/CMakeLists.txttest/fomac/test_fomac.cpp
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
CHANGELOG.mdsrc/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppsrc/qdmi/Driver.cppsrc/qdmi/dd/Device.cpptest/fomac/test_fomac.cppsrc/na/fomac/Device.cppsrc/fomac/FoMaC.cpp
📚 Learning: 2025-12-07T09:10:31.820Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1355
File: src/qdmi/sc/Device.cpp:97-102
Timestamp: 2025-12-07T09:10:31.820Z
Learning: In the munich-quantum-toolkit/core repository, duplication of QDMI-related macros (such as IS_INVALID_ARGUMENT) across device implementations (e.g., in src/qdmi/sc/Device.cpp and src/qdmi/dd/Device.cpp) is acceptable as a temporary measure. The preferred long-term solution is to upstream these macros to the QDMI repository rather than creating local shared headers, so they can be reused across all dependent projects.
Applied to files:
CHANGELOG.mdsrc/qdmi/na/CMakeLists.txtinclude/mqt-core/qdmi/Common.hppsrc/qdmi/dd/CMakeLists.txtsrc/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppsrc/qdmi/Driver.cppsrc/qdmi/CMakeLists.txtsrc/qdmi/sc/CMakeLists.txtsrc/qdmi/dd/Device.cppsrc/qdmi/Common.cppinclude/mqt-core/fomac/FoMaC.hpptest/fomac/test_fomac.cppsrc/na/fomac/Device.cppsrc/fomac/FoMaC.cpp
📚 Learning: 2025-12-01T11:00:40.342Z
Learnt from: flowerthrower
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 1
File: CHANGELOG.md:18-18
Timestamp: 2025-12-01T11:00:40.342Z
Learning: In the munich-quantum-toolkit/core-plugins-catalyst repository, the CHANGELOG.md intentionally references the parent MQT Core repository's release notes (https://github.com/munich-quantum-toolkit/core/releases) because the plugin repository is based on work performed in the parent repository.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
CHANGELOG.mdsrc/fomac/CMakeLists.txtsrc/qdmi/na/CMakeLists.txtinclude/mqt-core/qdmi/Common.hppsrc/qdmi/dd/CMakeLists.txtsrc/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppsrc/qdmi/Driver.cppsrc/qdmi/CMakeLists.txtsrc/qdmi/sc/CMakeLists.txtsrc/qdmi/dd/Device.cppsrc/qdmi/Common.cpptest/fomac/test_fomac.cppsrc/na/fomac/Device.cpp
📚 Learning: 2025-11-04T19:50:14.489Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/helpers/test_utils.cpp:103-118
Timestamp: 2025-11-04T19:50:14.489Z
Learning: In the munich-quantum-toolkit/core QDMI device tests, when QDMI APIs return a string result size `ks` that includes a null terminator, the pattern `std::string keys(ks - 1, '\0'); ... device_job_get_results(..., ks, keys.data(), nullptr)` is correct and safe. The string allocates `ks` bytes total (ks-1 content + implicit null), and the API writing `ks` bytes (including trailing null) fits exactly into this space without buffer overrun.
Applied to files:
test/qdmi/dd/results_sampling_test.cppinclude/mqt-core/qdmi/Common.hppsrc/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppsrc/qdmi/Driver.cppsrc/fomac/FoMaC.cpp
📚 Learning: 2025-10-17T11:09:50.147Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Applied to files:
src/qdmi/na/CMakeLists.txtsrc/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppsrc/qdmi/Driver.cppsrc/qdmi/dd/Device.cpptest/fomac/test_fomac.cpp
📚 Learning: 2025-12-05T15:57:39.701Z
Learnt from: flowerthrower
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 3
File: lib/Conversion/CatalystQuantumToMQTOpt/CMakeLists.txt:22-25
Timestamp: 2025-12-05T15:57:39.701Z
Learning: The munich-quantum-toolkit projects (core and core-plugins-catalyst) use `file(GLOB_RECURSE ...)` patterns in CMakeLists.txt files to collect header files, following an established convention in the parent repository for consistency across the codebase.
Applied to files:
src/qdmi/na/CMakeLists.txt
📚 Learning: 2025-10-09T13:14:10.178Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:219-221
Timestamp: 2025-10-09T13:14:10.178Z
Learning: The MQT Core project (munich-quantum-toolkit/core repository) uses the C++20 standard, not C++17. C++20 features such as abbreviated function templates (e.g., `const auto&` parameters) are supported and valid in this codebase.
Applied to files:
src/qdmi/na/CMakeLists.txtsrc/qdmi/sc/CMakeLists.txt
📚 Learning: 2025-10-10T08:09:54.528Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1246
File: bindings/CMakeLists.txt:0-0
Timestamp: 2025-10-10T08:09:54.528Z
Learning: In the Munich Quantum Toolkit (MQT) Core project, scikit-build-core is configured with `wheel.install-dir = "mqt/core"` in pyproject.toml, which automatically prefixes all CMake `DESTINATION` paths with `mqt/core/` during wheel installation. Therefore, CMake install destinations are relative to the `mqt/core` package namespace, not `site-packages`.
Applied to files:
src/qdmi/na/CMakeLists.txtsrc/qdmi/sc/CMakeLists.txt
📚 Learning: 2025-11-05T07:42:45.507Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
Applied to files:
src/qdmi/sc/Device.cppsrc/qdmi/na/Device.cppsrc/qdmi/Driver.cppsrc/qdmi/dd/Device.cppinclude/mqt-core/fomac/FoMaC.hpptest/fomac/test_fomac.cppsrc/na/fomac/Device.cppsrc/fomac/FoMaC.cpp
📚 Learning: 2025-11-05T09:23:46.540Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: src/qdmi/dd/Device.cpp:492-521
Timestamp: 2025-11-05T09:23:46.540Z
Learning: In the munich-quantum-toolkit/core repository, the `qasm3::Importer::imports()` function is backwards compatible with OpenQASM 2 programs. Therefore, it can be used to parse both QASM2 and QASM3 program formats without requiring separate importers for each version.
Applied to files:
src/qdmi/dd/Device.cpp
📚 Learning: 2025-11-04T14:26:25.420Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/conftest.py:11-19
Timestamp: 2025-11-04T14:26:25.420Z
Learning: In the munich-quantum-toolkit/core repository, Qiskit is always available as a dependency during testing, so import guards for qiskit-dependent imports in test files (e.g., test/python/qdmi/qiskit/*.py) are not necessary.
Applied to files:
src/qdmi/dd/Device.cpptest/fomac/test_fomac.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
include/mqt-core/fomac/FoMaC.hpp
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
test/fomac/test_fomac.cpp
📚 Learning: 2025-10-09T13:13:51.224Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Applied to files:
src/na/fomac/Device.cpp
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
src/fomac/FoMaC.cpp
🧬 Code graph analysis (9)
include/mqt-core/qdmi/Common.hpp (2)
src/qdmi/dd/Device.cpp (9)
ADD_SINGLE_VALUE_PROPERTY(196-197)ADD_SINGLE_VALUE_PROPERTY(199-200)ADD_SINGLE_VALUE_PROPERTY(208-209)ADD_SINGLE_VALUE_PROPERTY(212-213)ADD_SINGLE_VALUE_PROPERTY(304-305)ADD_SINGLE_VALUE_PROPERTY(330-331)ADD_SINGLE_VALUE_PROPERTY(333-334)ADD_SINGLE_VALUE_PROPERTY(335-336)ADD_SINGLE_VALUE_PROPERTY(400-401)src/qdmi/na/Device.cpp (7)
ADD_SINGLE_VALUE_PROPERTY(88-89)ADD_SINGLE_VALUE_PROPERTY(91-92)ADD_SINGLE_VALUE_PROPERTY(99-100)ADD_SINGLE_VALUE_PROPERTY(103-105)ADD_SINGLE_VALUE_PROPERTY(106-107)ADD_SINGLE_VALUE_PROPERTY(290-291)ADD_SINGLE_VALUE_PROPERTY(292-293)
src/qdmi/sc/Device.cpp (3)
include/mqt-core/qdmi/Driver.hpp (1)
instance(424-427)src/qdmi/dd/Device.cpp (2)
get(152-158)get(152-152)src/qdmi/na/Device.cpp (2)
get(47-53)get(47-47)
src/qdmi/na/Device.cpp (2)
src/qdmi/dd/Device.cpp (2)
get(152-158)get(152-152)src/qdmi/sc/Device.cpp (2)
get(50-56)get(50-50)
src/qdmi/Driver.cpp (2)
include/mqt-core/qdmi/Driver.hpp (4)
param(299-300)param(379-380)status(325-325)libName(445-447)include/mqt-core/qdmi/Common.hpp (14)
toString(106-136)toString(106-106)toString(150-178)toString(150-150)toString(181-199)toString(181-181)toString(203-232)toString(203-204)toString(235-277)toString(235-235)toString(280-318)toString(280-280)toString(321-369)toString(321-321)
src/qdmi/dd/Device.cpp (3)
include/mqt-core/qdmi/Driver.hpp (1)
instance(424-427)src/qdmi/na/Device.cpp (2)
get(47-53)get(47-47)src/qdmi/sc/Device.cpp (2)
get(50-56)get(50-50)
src/qdmi/Common.cpp (1)
include/mqt-core/qdmi/Common.hpp (14)
toString(106-136)toString(106-106)toString(150-178)toString(150-150)toString(181-199)toString(181-181)toString(203-232)toString(203-204)toString(235-277)toString(235-235)toString(280-318)toString(280-280)toString(321-369)toString(321-321)
include/mqt-core/fomac/FoMaC.hpp (1)
include/mqt-core/qdmi/Common.hpp (15)
toString(106-136)toString(106-106)toString(150-178)toString(150-150)toString(181-199)toString(181-181)toString(203-232)toString(203-204)toString(235-277)toString(235-235)toString(280-318)toString(280-280)toString(321-369)toString(321-321)throwIfError(147-147)
src/na/fomac/Device.cpp (1)
include/mqt-core/na/fomac/Device.hpp (3)
Device(88-88)device(123-124)d(125-125)
src/fomac/FoMaC.cpp (3)
src/qdmi/dd/Device.cpp (4)
queryProperty(180-222)queryProperty(180-181)queryProperty(384-403)queryProperty(384-387)include/mqt-core/qdmi/Common.hpp (15)
throwIfError(147-147)toString(106-136)toString(106-106)toString(150-178)toString(150-150)toString(181-199)toString(181-181)toString(203-232)toString(203-204)toString(235-277)toString(235-235)toString(280-318)toString(280-280)toString(321-369)toString(321-321)src/qdmi/Common.cpp (2)
throwIfError(25-55)throwIfError(25-25)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (56)
test/qdmi/dd/results_sampling_test.cpp (1)
33-39: Good explicit typing to avoid unintended narrowingUsing
size_tforsumaligns it withshotsand typical histogram count types, avoiding potential narrowing compared toautofrom0U. Loop logic and assertion remain correct.src/qdmi/na/CMakeLists.txt (3)
94-97: Path consistency between header location and directory creation is now correct.The header output path (line 94) and directory creation (line 97) are now aligned at
${CMAKE_CURRENT_BINARY_DIR}/include/qdmi/na, resolving the previous inconsistency. The generated header will be written to a directory that actually exists.
27-27: Path updates to align with qdmi/na directory structure are consistent.The header references have been updated consistently across all target definitions to use the new
qdmi/nanamespace structure:
- Generator.hpp at line 27
- Device.hpp at lines 128 and 178
These align with the directory creation change and the overall PR objective of unifying the directory structure for QDMI devices.
Also applies to: 128-128, 178-178
184-185: This dependency pattern is intentional and consistent across implementations.The static device library and dynamic device library have different PRIVATE dependencies by design. The Generator library (line 33) and dynamic device library (lines 184–185) include
MQT::ProjectOptionsandMQT::ProjectWarningsfor compiler configuration, while the static device library does not directly link them. This same pattern appears consistently in bothsrc/qdmi/na/CMakeLists.txtandsrc/qdmi/sc/CMakeLists.txt, indicating this is intentional rather than an oversight. The separation reflects a deliberate choice to apply compiler options at the dynamic/generator level rather than the static level, which is a valid design.src/qdmi/CMakeLists.txt (2)
28-30: LGTM!The target collection pattern is correct: both the common and driver targets are appended locally, then propagated to the parent scope at the end. This ensures both targets are included in
MQT_CORE_TARGETS.Also applies to: 57-62
50-54: LGTM!The driver's public linkage to
MQT::CoreQDMICommonis appropriate, ensuring consumers have access to common QDMI utilities likethrowIfErrorandtoStringoverloads.src/qdmi/sc/CMakeLists.txt (2)
131-135: LGTM!The private linkage to
MQT::CoreQDMICommoncorrectly provides access to shared QDMI utilities (likeIS_INVALID_ARGUMENTmacro andtoStringhelpers) used in the SC device implementation.
180-186: LGTM!The dynamic SC device library correctly adds
MQT::CoreQDMICommonto its private dependencies while retainingMQT::ProjectOptionsandMQT::ProjectWarningsfor proper build settings in the shared library context.src/qdmi/sc/Device.cpp (3)
50-56: LGTM!The leaked static singleton pattern is consistent with
dd/Device.cppandna/Device.cppimplementations, correctly avoiding static deinitialization order issues. The NOLINT comment documents the intentional memory management choice.
77-79: Validation pattern is consistent across all query/set methods.The
IS_INVALID_ARGUMENTmacro usage follows the established pattern. Based on learnings, duplication across device implementations is acceptable as a temporary measure pending upstream to the QDMI repository.
406-412: LGTM!The initialization pattern correctly ensures the singleton is created via
Device::get(), whiledevice_finalizeappropriately becomes a no-op for the leaked singleton approach. Usingstd::ignoreto explicitly discard the return value is a clean way to suppress warnings.src/fomac/CMakeLists.txt (1)
22-26: LGTM!Adding
MQT::CoreQDMICommonto the public linkage is appropriate since the Session API (formerly FoMaC) exposes types and utilities from the common library to consumers.CHANGELOG.md (1)
14-14: LGTM!The changelog entries accurately document the new authentication support, the FoMaC→Session refactor, NA device relocation, thread-safe reference counting, and the custom property handling fix. All entries follow the established format.
Also applies to: 26-29, 42-42
src/qdmi/dd/CMakeLists.txt (1)
40-44: LGTM!The private linkage to
MQT::CoreQDMICommoncorrectly provides the DD device access to shared QDMI utilities used insrc/qdmi/dd/Device.cpp. This is consistent with the SC device's static target linkage pattern.src/qdmi/Common.cpp (2)
25-55: LGTM!The
throwIfErrorfunction correctly maps QDMI status codes to appropriate C++ exceptions:
QDMI_SUCCESS→ no actionQDMI_WARN_GENERAL→ warning to stderr (non-throwing)QDMI_ERROR_OUTOFMEM→std::bad_alloc(correctly constructed without message per standard)QDMI_ERROR_OUTOFRANGE/QDMI_ERROR_INVALIDARGUMENT→ domain-specific exceptions- Other errors →
std::runtime_errorThe nested switch structure cleanly separates non-error cases from error handling.
11-21: Well-documented with clear intent for future upstream.The file documentation clearly notes this will be upstreamed to the QDMI core library. The includes are minimal and appropriate for the implementation.
src/qdmi/dd/Device.cpp (3)
152-158: LGTM! Singleton lifecycle change addresses previous race condition concerns.The function-local static with intentional leak pattern correctly avoids static deinitialization order fiasco and eliminates the race condition identified in previous reviews. This is now consistent with
src/qdmi/na/Device.cppandsrc/qdmi/sc/Device.cpp.
694-700: LGTM! Simplified lifecycle management.
device_initializenow simply ensures the singleton exists viaDevice::get(), anddevice_finalizeis correctly a no-op since the instance is intentionally leaked to avoid destruction order issues.
182-184: Consistent use of centralized validation macro.The
IS_INVALID_ARGUMENTmacro provides unified validation across device, session, site, operation, job parameters, and results. Based on learnings, this duplication across device implementations is acceptable as a temporary measure until upstreamed to the QDMI repository.src/qdmi/na/Device.cpp (2)
47-53: LGTM! Consistent singleton pattern.The function-local static with intentional leak pattern matches the implementation in
src/qdmi/dd/Device.cppandsrc/qdmi/sc/Device.cpp, ensuring consistency across all QDMI device implementations.
574-580: LGTM! Consistent lifecycle pattern.The
device_initializeanddevice_finalizefunctions follow the same pattern as the DD device implementation.src/qdmi/Driver.cpp (3)
162-185: LGTM! Robust parameter binding with proper error handling.The
setParameterlambda correctly:
- Skips if value is empty or function pointer is null
- Returns silently on
QDMI_SUCCESS- Logs and skips on
QDMI_ERROR_NOTSUPPORTED(graceful degradation for devices without auth support)- Frees the session and throws with detailed error message for actual failures
This addresses the previous review feedback about handling unsupported parameters gracefully.
187-197: LGTM! Complete parameter binding from config.All authentication parameters from
DeviceSessionConfigare bound:baseUrl,token,authFile,authUrl,username,password, andcustom1–custom5. This aligns with the PR objective of propagating session/authentication parameters to devices.
388-394: LGTM! Dynamic library loading with session configuration.The
addDynamicDeviceLibrarymethod now accepts and forwardsDeviceSessionConfigto the device constructor, enabling authentication for dynamically loaded QDMI devices.include/mqt-core/qdmi/Common.hpp (3)
28-34: LGTM! Well-implemented unreachable helper.The
unreachable()function correctly uses compiler-specific intrinsics (__builtin_unreachable()for GCC/Clang,__assume(false)for MSVC) and is properly marked[[noreturn]].
99-103: LGTM! Centralized validation macro.The
IS_INVALID_ARGUMENTmacro correctly validates that a property/parameter value is within the valid range (less than*_MAX) while allowingCUSTOM1–CUSTOM5extension values. This centralizes validation logic previously duplicated across device implementations.
105-136: LGTM! Comprehensive status code stringification.All
QDMI_STATUSvalues are covered with human-readable strings, and theunreachable()fallback ensures compiler warnings for missing cases in future enum extensions.include/mqt-core/fomac/FoMaC.hpp (3)
146-175: LGTM! Complete SessionConfig with custom parameters.The
SessionConfigstruct includes all authentication parameters (token,authFile,authUrl,username,password,projectId) pluscustom1–custom5as requested in previous review discussions. All fields are appropriatelystd::optional<std::string>.
326-358: LGTM! Improved error messages in queryProperty.The
queryPropertymethod now constructs descriptive error messages usingqdmi::toString(prop)before callingqdmi::throwIfError, making debugging easier when property queries fail.
699-718: LGTM! Proper Session lifecycle management.The
Sessionclass correctly:
- Takes
SessionConfigas a constructor parameter for authentication- Deletes copy constructor/assignment to prevent resource duplication
- Implements move semantics for safe ownership transfer
- Has a non-virtual destructor (addressing past review feedback)
test/fomac/test_fomac.cpp (11)
11-28: LGTM! Includes updated appropriately for Session-based API.The new includes for
<filesystem>,<fstream>, and<thread>support the authentication test cases that validate file existence and generate unique temp file names.
31-68: LGTM! Test fixtures correctly migrated to Session types.The test classes (
DeviceTest,SiteTest,OperationTest,DDSimulatorDeviceTest) are properly updated to useSession::Device,Session::Device::Site, andSession::Device::Operationtypes. ThegetDDSimulatorDevice()helper correctly instantiates a localSessionto retrieve devices.
70-103: LGTM! Job-related test fixtures updated correctly.
JobTestandSimulatorJobTestnow useSession::Jobtype, and thecreateTestJob()methods return the correct type.
105-229: LGTM! Status and property toString tests updated to useqdmi::namespace.The test cases correctly use the
qdmi::toStringandqdmi::throwIfErrorfunctions from the centralizedqdmi/Common.hppheader.
745-758: LGTM! SessionParameterToString test provides good coverage.The test covers all standard session parameters including
QDMI_SESSION_PARAMETER_MAXand custom parameters, addressing earlier review feedback.
760-827: Good validation coverage for token and URL authentication parameters.The tests cover valid and invalid URL formats including localhost variations, empty URLs, unsupported protocols, and missing protocols. The empty URL edge case (lines 823-827) was added per earlier review feedback.
829-853: Temporary file uniqueness relies on thread ID hash.The temp file naming uses
std::hash<std::thread::id>which provides reasonable uniqueness within a single process. Per earlier discussion with the maintainer, cross-process collision risk is accepted to avoid platform-specific code.
888-948: Custom parameters smoke tests improved withGTEST_SKIP().The tests now use
GTEST_SKIP()when custom parameters are unsupported, which is cleaner than the previousSUCCEED()in all branches. This addresses earlier review feedback about tests always passing regardless of outcome.
950-976: LGTM! Session instance tests verify basic functionality.
SessionGetDevicesReturnsListandSessionMultipleInstancestests verify that Session objects can be constructed and return devices, confirming the non-singleton behavior works correctly.
978-984: LGTM! Helper function correctly scoped in anonymous namespace.The
getDevices()helper is properly placed in an anonymous namespace without the redundantinlinespecifier, addressing earlier static analysis and review feedback.
986-1026: LGTM! Test suite instantiation updated for Session types.The
INSTANTIATE_TEST_SUITE_Pmacros correctly usegetDevices()andSession::Devicefor parameter info types.src/na/fomac/Device.cpp (6)
15-15: LGTM! Include path updated for reorganized directory structure.The include path change from
"na/device/Generator.hpp"to"qdmi/na/Generator.hpp"aligns with the directory structure unification mentioned in the PR objectives (#1332).
38-90: LGTM! Helper functions updated to accept Session types.Both
calculateExtentFromSitesoverloads correctly acceptfomac::Session::Device::Siteand pairs of sites. The logic remains unchanged.
113-172: LGTM! Device initialization methods migrated to Session::Device namespace.The
initNameFromDevice,initMinAtomDistanceFromDevice,initQubitsNumFromDevice,initLengthUnitFromDevice,initDurationUnitFromDevice, andinitDecoherenceTimesFromDevicemethods are correctly updated to theSession::Devicenamespace while preserving their implementation logic.
173-296: LGTM!initTrapsfromDevicemigrated correctly.The trap initialization logic is unchanged; only the namespace qualification is updated.
297-578: LGTM!initOperationsFromDevicecorrectly uses Session types.The operation loop now uses
fomac::Session::Device::Operationand the local variable is renamed fromnametoopNameto avoid potential shadowing with the outer scope. The regex matching and shuttling unit logic remain intact.
579-590: LGTM! Device constructor andgetDevicesupdated for Session-based API.The
Deviceconstructor now acceptsconst fomac::Session::Device&and forwards to the base class. ThegetDevices()method creates a localfomac::Sessioninstance, retrieves devices, and attempts to create NADeviceobjects from them. This correctly reflects the non-singleton Session design.src/fomac/FoMaC.cpp (9)
13-30: LGTM! Includes updated for Session implementation.New includes for
<filesystem>,<regex>, and<spdlog/spdlog.h>support the authentication file validation, URL pattern matching, and logging respectively.
32-74: LGTM! Site property accessors migrated to Session::Device::Site namespace.All site property getters are correctly updated to the
Session::Device::Sitenamespace with unchanged implementation logic.
75-174: LGTM! Operation property accessors migrated to Session::Device::Operation namespace.All operation property getters including
getName,getQubitsNum,getDuration,getFidelity,getSites,getSitePairs, etc., are correctly updated.
175-275: LGTM! Device property accessors migrated to Session::Device namespace.All device property getters are correctly updated with consistent use of the
Session::Devicenamespace.
277-305: LGTM! Job submission uses centralized error handling.The
submitJobmethod correctly usesqdmi::throwIfErrorfor consistent error handling across all QDMI operations.
307-603: LGTM! Job methods migrated with consistent error handling.All
Session::Jobmethods (check,wait,cancel,getId,getProgramFormat,getProgram,getNumShots,getShots,getCounts,getDenseStateVector,getDenseProbabilities,getSparseStateVector,getSparseProbabilities) correctly useqdmi::throwIfErrorandqdmi::unreachable().
605-675: Session constructor implements robust resource management and validation.The implementation addresses previous review feedback:
- Resource leak prevention: The
cleanuplambda ensuresQDMI_session_freeis called if any exception occurs after allocation.- URL validation: The regex now accepts
localhostURLs per maintainer request.- File validation:
authFileexistence is checked before use.- Optional parameter handling: The
setParameterhelper gracefully skips unsupported parameters withSPDLOG_INFOlogging.
677-696: LGTM! Session lifecycle management is correct.The destructor safely checks for null before freeing, and move semantics correctly transfer ownership while nullifying the source. Thread-safety concerns are deferred to a separate tracking issue per maintainer decision.
698-708: LGTM!getDevicesreturns wrapped Device objects.The implementation correctly queries the session for devices and wraps them in
Deviceobjects using theToken{}pattern for access control.
… test for content equivalence, which might be wonky on Windows in `constexpr` contexts
Description
This PR aims to introduce session authentication via QDMI to FoMaC. I didn't see a clear path to implement this without any API changes.
In essence, in C++ land,
FoMaChas been renamed toSession(as agreed upon by @burgholzer) and is no longer a singleton; it must be instantiated to handle sessions. The implementation sets session parameters during construction. The Python bindings directly follow the new C++ semantics. Furthermore, the extensions have been propagated tobackend.pyandprovider.pyfor Qiskit integration.On the driver side,
Driver::addDynamicDeviceLibraryhas been extended with aqdmi::DeviceSessionConfigparameter to setQDMI_DEVICE_SESSION_PARAMETERs upon loading a library. This function has also been exposed to Python throughmqt.core.qdmi.fomac, which is an inconsistency, but theDriverinterface is currently not available in the bindings. We could clean this up later.Known limitations:
All this will be addressed in future PRs.
Fixes #1350
Fixes #1332
Checklist: