♻️ Redesign QDMI device management - #1901
Conversation
burgholzer
added
refactor
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
MatthiasReumann
left a comment
MatthiasReumann
left a comment
There was a problem hiding this comment.
This is a great start into the QDMI rewrite! 🚀
I've gone through the code a couple of times now and the following comments are the result of that.
However, currently this can't construct devices at runtime, right? As of now this simply loads dynamic libraries, I think 🤔.
| { | ||
| const std::scoped_lock lock(impl_->mutex); | ||
| library = impl_->libraries[key].lock(); | ||
| if (!library) { | ||
| library = | ||
| detail::makeV1DeviceApi(definition->library, definition->prefix); | ||
| impl_->libraries[key] = library; | ||
| } | ||
| } |
There was a problem hiding this comment.
Something is fishy here.
impl_->libraries[key] is a std::weak_ptr - a non-owning reference to a shared_ptr. The DeviceManager (or the DeviceManager::Impl) class should be owning the device shared_ptr. .lock()ing and assigning the newly created shared_ptr only works (as far as I can tell) because the Device class below holds the same shared_ptr. I am pretty sure this function wouldn't work anymore if the Device takes a const pointer instead of a smart pointer.
There was a problem hiding this comment.
The map in DeviceManager::Impl should probably store the unique_ptr mentioned above (or below).
There was a problem hiding this comment.
I am also not really sure if I like the mutex here. I get where this is coming from - thread safety - but I think one really has to think this through if this would be enough.
There was a problem hiding this comment.
@coderabbitai Could you please evaluate why a weak_ptr is used here and if this is correct?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
fd -HI "DeviceManager.hpp"
fd -HI "Device.hpp" src/qdmi || true
fd -HI "DeviceApi.hpp" || true
echo "---DeviceManager.hpp---"
fd "DeviceManager.hpp" -x cat -n {}
echo "---Device.hpp (qdmi)---"
fd "Device.hpp" -p "qdmi" -x cat -n {}
echo "---DeviceApi.hpp---"
fd "DeviceApi.hpp" -x cat -n {}Length of output: 4767
🏁 Script executed:
#!/bin/bash
set -e
fd "Device.hpp" -x cat -n {}
echo "---DeviceApi.hpp---"
cat -n src/qdmi/DeviceApi.hpp
echo "---DeviceApi.cpp (if exists)---"
fd "DeviceApi.cpp" -x cat -n {}Length of output: 89512
@MatthiasReumann Good catch — let me walk through what's actually happening and where I agree/disagree.
Why weak_ptr is used (the intent)
impl_->libraries is meant to be a non-owning cache keyed by library path + prefix, so that:
- Multiple
open()calls for the same underlying.so/.dll(e.g., two sessions of the same device, or child devices sharing a parent library) reuse a singledlopen'dDeviceApiinstance instead of loading it multiple times. - The library is automatically unloaded (
dlclose) once nothing needs it anymore —DeviceManagerdoesn't want to force-keep every library loaded forever just because it was opened once.
The real owner of the shared_ptr<DeviceApi> is not DeviceManager at all — it's detail::DeviceState::api (see DeviceApi.hpp Line 97), which is reached via Device::state_ → DeviceState. So the chain is:
Device --(shared_ptr)--> DeviceState --(shared_ptr)--> DeviceApi
DeviceManager::Impl::libraries[key] only ever holds a weak_ptr, so as soon as the last Device/Job using that library is destroyed, DeviceState's destructor releases its shared_ptr<DeviceApi>, the refcount hits zero, the library is dlclose'd, and the cache entry silently becomes expired (lazily replaced on the next open()).
Is this correct?
Today, yes — mechanically it works because DeviceState is the sole strong owner. But your instinct is right that this is fragile: correctness here depends entirely on an invariant that isn't enforced or even documented anywhere near DeviceManager.cpp — it just happens to hold because of how DeviceState/Device are implemented elsewhere. If someone later changes Device/DeviceState to not hold the shared_ptr (e.g., a raw pointer optimization, or if DeviceState::api were made a weak_ptr too "for symmetry"), this cache would break with no compile-time signal — library in open() would be constructed, immediately go out of scope with no other owner, and be unloaded from under the returned Device. I'd suggest adding a comment above the libraries member (in DeviceManager::Impl) stating explicitly: "non-owning cache; DeviceState/Device must hold the real shared_ptr<DeviceApi>." That at least makes the contract discoverable for future refactors.
A more concrete, separate bug your mutex comment surfaced
The mutex in DeviceManager::Impl only guards libraries. It does not guard registry (DeviceRegistry::definitions_, a plain std::vector):
DeviceManager::registerDevice/unregisterDevicecall straight intoimpl_->registry.registerDevice/unregisterDevicewith no lock at all.DeviceManager::openreadsimpl_->registry.definitions()unguarded (thescoped_lockis taken only afterward, around thelibrariesmap access).
So if one thread calls open() while another calls registerDevice()/unregisterDevice() on the same DeviceManager, that's an unguarded concurrent read/write on std::vector, which is a real data race/UB, not just a theoretical concern. If DeviceManager is meant to be safely usable from multiple threads (which the presence of a mutex implies), this needs to be fixed too, e.g.:
const std::vector<DeviceDefinition>& DeviceManager::definitions() const {
const std::scoped_lock lock(impl_->mutex);
return impl_->registry.definitions(); // NB: returning a ref out of a locked scope is itself unsafe under mutation
}
void DeviceManager::registerDevice(DeviceDefinition definition, const bool replace) {
const std::scoped_lock lock(impl_->mutex);
impl_->registry.registerDevice(std::move(definition), replace);
}Note that even this isn't quite enough for definitions()/open(), since returning a reference to (or iterating) a mutable vector after releasing the lock is unsafe if another thread can mutate it concurrently — you'd likely want open() to copy out the matching DeviceDefinition while holding the lock, rather than holding an iterator/reference into definitions_ past the lock's scope.
So, to directly answer your question: the weak_ptr choice for the library cache is intentional and works today, but it's an implicit, undocumented invariant rather than a locally-verifiable one — worth a comment at minimum. The mutex, as currently scoped, doesn't make DeviceManager thread-safe overall since it never protects registry/definitions_, which is the concrete issue underlying your third comment.
Want me to open a follow-up issue for the registry/definitions_ locking gap, or would you rather fold it into this PR?
✏️ Learnings added
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1901
File: src/qdmi/DeviceManager.cpp:659-667
Timestamp: 2026-07-15T08:07:16.502Z
Learning: In munich-quantum-toolkit/core, `src/qdmi/DeviceManager.cpp`'s `DeviceManager::Impl::libraries` map intentionally stores `std::weak_ptr<detail::DeviceApi>` as a non-owning cache keyed by (library path, prefix); the true strong owner of the loaded library is `detail::DeviceState::api` (a `std::shared_ptr<DeviceApi>`, defined in `src/qdmi/DeviceApi.hpp`), reached transitively via `Device::state_` -> `DeviceState`. This lets the library auto-unload via `dlclose`/`FreeLibrary` once the last `Device`/`Job` referencing it is destroyed. This is a fragile, non-local invariant: if `DeviceState`/`Device` ever stop holding a strong `shared_ptr<DeviceApi>`, the weak cache in `DeviceManager` would break silently.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@burgholzer I think this is quite interesting. Maybe something to think about whether the DeviceManager should be the only instance managing devices. Personally, I am not 100% sure if I would like to keep the shared_ptr; although the arguments are somewhat convincing. In the end it's a design decision.
There was a problem hiding this comment.
I'll add some more reasoning from my iteration later. Right now, I am rather convinced this is a good design as it is.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai @MatthiasReumann The weak ownership is intentional: the cache is only a reuse index, while live device and job state owns the loaded library. This lets multiple sessions reuse an already loaded provider without forcing the library to remain loaded after the final live object disappears.
I did find a real problem in the original implementation: the cache was manager-local, so separate managers could initialize the same library independently. It is now process-wide, keyed by canonical library path and symbol prefix, and guarded during lookup and construction. If no live object remains, the weak reference expires and reopening the provider is the expected behavior. A regression test covers reuse across simultaneously live managers and equivalent library paths.
Following the lifetime reasoning above, I did not put manager-owned unique_ptrs into this map. Doing so would couple library lifetime to one manager and conflict with the requirement that returned devices and jobs can outlive it.
The map has instead moved out of DeviceManager into the process-wide library loader and contains weak references. Strong ownership remains exclusively with live device and job state, so the final such object controls unloading.
Agreed that the original mutex was not a complete synchronization design. The responsibilities are now separated:
-
a manager-local registry mutex protects definition snapshots, registration, unregistration, and lookup;
-
definitions() returns a value snapshot instead of exposing a reference;
-
a separate process-wide cache mutex protects canonical library lookup and initialization across all managers.
Session creation remains independent after the shared provider library has been obtained. Tests cover concurrent opens, concurrent registry access, independent sessions, and process-wide library reuse. This defines the manager’s guarantees without attempting to serialize all calls made through independent provider sessions.
There was a problem hiding this comment.
I think the rationale is getting clearer for me now, thanks! I have some question / thoughts though:
The weak ownership is intentional: the cache is only a reuse index, while live device and job state owns the loaded library.
Now I get why the DeviceApi is being used as a shared_ptr - this makes totally sense!
I did find a real problem in the original implementation: the cache was manager-local, so separate managers could initialize the same library independently. It is now process-wide, keyed by canonical library path and symbol prefix, and guarded during lookup and construction. If no live object remains, the weak reference expires and reopening the provider is the expected behavior. A regression test covers reuse across simultaneously live managers and equivalent library paths.
Just to make sure that I understand you fully here: Alternatively we could have made the DeviceManager a singleton, right? Probably depends on if we ever want to create DeviceManager's using different registries.
Following the lifetime reasoning above, I did not put manager-owned unique_ptrs into this map. Doing so would couple library lifetime to one manager and conflict with the requirement that returned devices and jobs can outlive it.
I think we should then probably find a more suitable name than DeviceManager. I don't know, something like DeviceExplorer? Following we should document how to best use the DeviceManager. Currently, I imagine something like
auto findAllDevices(....) {
DeviceManager m(...);
return m.openAll(...);
}
66b0be6 to
b050c37
Compare
78f00ef to
330c111
Compare
|
@MatthiasReumann I am through with another iteration here. Feel free to take another look. I may try to split this into two changes, one that is v3.x compatible but adds the configurability and a separate one that contains the breaking change of dropping the client interface and combining driver and FoMaC. Also pinging @marcelwa @flowerthrower @ystade here for awareness. |
MatthiasReumann
left a comment
MatthiasReumann
left a comment
There was a problem hiding this comment.
Solid improvements! 🚀
I've left two minor requests and some questions / thoughts in the open discussion above.
Otherwise, I've nothing to complain than we should probably add a bit more documentation on the caching, shared_ptr, lifetime logic to the respective classes. I am pretty sure Codex can help with that ;)
There was a problem hiding this comment.
.hpp file in src folder
There was a problem hiding this comment.
This file still uses trailing return types.
81dd10e to
fc24a9c
Compare
fc24a9c to
768b78c
Compare
Replace the overlapping FoMaC and client-driver layers with one registry, manager, and object model shared by C++, Python, Qiskit, and the neutral-atom adapter. Assisted-by: GPT-5.6 via Codex
Describe configuration, registry ownership, fresh-session opening, migration, and validation for the consolidated public API. Assisted-by: GPT-5.6 via Codex
768b78c to
466e58e
Compare
Cpp-Linter Report
|
2 participants
🤖 AI text below 🤖
Summary
This PR completes the QDMI object-model redesign on top of the configuration
foundation in #1912. It replaces the overlapping FoMaC and client-driver layers
with one public flow:
DeviceRegistry → DeviceManager → DeviceThe same objects are used by C++, Python, Qiskit, and the neutral-atom adapter.
What changed
DeviceRegistrydiscovers configured definitions and supports idempotentfallback registration without loading device code.
DeviceManagerowns an immutable registry snapshot and opens an independentdevice session for every call.
openAll()isolates failures by stable deviceID.
Device,Site,Operation, andJobshare the session state they need, soderived objects and jobs may safely outlive the manager and their parent
wrappers.
canonical library path and symbol prefix. A library remains initialized while
any live session needs it and unloads after the final session is released.
Reopening the same implementation waits for the prior generation to finish
finalization and unloading.
DeviceDefinitionaccepts path-like libraries and returnspathlib.Path.SessionParametershas one keyword-only constructor for itsoptional fields, including path-like
auth_file.qdmi::DriverC++/Python APIs are removed;UPGRADING.mddocuments the migration.
Relationship to #1912
This branch is based on exact #1912 head
7769bdde0a710b340c5f3a291a509562131f505b. It reuses #1912'sqdmi.json,[tool.qdmi], stable-ID merge semantics, disabled-ID reservations, relocatableCMake metadata, and runtime packaging. It does not duplicate configuration
parsing or add another discovery mechanism.
Integration feedback
The reconstructed API was exercised from local QDMI-on-IQM, Amazon Braket, and
QDMI template/example branches. That iteration removed manager mutation and
singleton state, eliminated separate session-ownership members, preserved
path-like inputs, and added the concise
SessionParametersconstructor.Independently installed device wheels remain explicit: they expose their
library path and register a fallback definition before constructing a manager.
User configuration and disabled definitions retain precedence.
Validation
tests
uvx nox -s lintgit diff --checkDepends on #1912.