♻️ Load a replaceable QDMI Client driver - #2229
Conversation
010d76c to
295a166
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
295a166 to
785c041
Compare
785c041 to
7eb542d
Compare
2aa8996 to
cfc4815
Compare
cfc4815 to
3de68c6
Compare
3de68c6 to
32bb143
Compare
burgholzer
left a comment
There was a problem hiding this comment.
Alright. I have quite a few ideas here for how to make this better and improve the overall wording. I really like how this is shaping up though. I did not think that replaceable drivers would feel so natural. Let's see how much of the custom driver extension will actually remain in the end.
There was a problem hiding this comment.
This feels right, but it also feels fairly complex. Are we sure all of this is really needed to properly distribute this? Keep later PRs in the stack in mind for this.
| std::same_as<T, std::vector<std::byte>>; | ||
|
|
||
| namespace detail { | ||
| struct ClientApi { |
There was a problem hiding this comment.
Likely ClientAPI would be better suited here. There may be other places benefitting from the improved spelling.
There was a problem hiding this comment.
This should very closely follow the implementation in the driver so that this has a common look and feel that does not diverge too much. Aligning the two should be a priority here.
| * @param id Stable registered device ID. | ||
| * @param overrides Session values that replace registered defaults. | ||
| * @return An owning device wrapper for the new session. | ||
| * @brief Opens a Client-visible QDMI device in a fresh session. |
There was a problem hiding this comment.
| * @brief Opens a Client-visible QDMI device in a fresh session. | |
| * @brief Opens a client-visible QDMI device in a fresh session. |
Worth establishing consistently
| ClientSession(const ClientSession&) = delete; | ||
| ClientSession& operator=(const ClientSession&) = delete; |
There was a problem hiding this comment.
should this define (defaulted) move constructors and assignment?
There was a problem hiding this comment.
Given how the new code here shares quite a bit of code with the loading logic in the driver, does it make sense to introduce a shared support library (could be header-only) that centralizes a bit of the code and avoids duplication?
I'd only do this if it genuinely saves code.
| {directory / filename, directory / "lib" / filename, | ||
| directory / "bin" / filename, | ||
| directory.parent_path() / "lib" / filename, | ||
| directory.parent_path() / "bin" / filename}) { |
There was a problem hiding this comment.
Should this also cover lib64. IIRC we had problems with that already at some point on manylinux images.
| -> LoadedClient { | ||
| auto* const library = openLibrary(path); | ||
| if (library == nullptr) { | ||
| throw std::runtime_error("Cannot load QDMI Client driver '" + |
There was a problem hiding this comment.
just to annotate this once more: We shouldn't use "QDMI Client driver" but simply "QDMI driver".
| const SessionConfig& config) { | ||
| if (id.empty() || id.find('\0') != std::string_view::npos) { | ||
| throw std::invalid_argument( | ||
| "QDMI device ID must not be empty or contain null bytes"); | ||
| } | ||
| Session session(config); | ||
| auto devices = session.getDevices(); | ||
| std::string available; | ||
| for (const auto& device : devices) { | ||
| const auto candidateId = device.getId(); | ||
| if (candidateId == id) { | ||
| return device; | ||
| } | ||
| if (!available.empty()) { | ||
| available += ", "; | ||
| } | ||
| available += candidateId; | ||
| } | ||
| throw std::out_of_range("QDMI Client session has no device with ID '" + | ||
| std::string(id) + "'; available IDs: " + available); |
There was a problem hiding this comment.
This feels like something that should be improved in the QDMI client interface upstream so that code like this becomes way simpler (and closer to what the custom driver extension actually offered before).
There was a problem hiding this comment.
I get where most of the changes are coming from here and it is quite natural that there are substantial additions given the replaceable driver. However, some of the code, especially around lifetime handling feels more complicated than it necessarily needs to be in my opinion. Maybe some opportunities for simplification arise out of the consequences of the other review comments.
32bb143 to
7504600
Compare
Route Core through one validated standard QDMI 1.4 Client ABI while keeping the packaged Driver as a separate shared library. Select devices by stable Client IDs, retain session-owned handles, support generic authentication and UTF-8 loader paths, and update in-tree consumers and runtime tests. Assisted-by: GPT-5.6 Sol via Codex
Move feature-group coverage to an isolated compiler test that uses the standard fake Client. This preserves public-boundary coverage without linking the Core QDMI target back to private Driver code. Delay Windows discovery for QDMI tests until runtime dependencies have been staged. Assisted-by: GPT-5.6 Sol via Codex
Reject successful or warning allocations that return a null handle before printing a warning or fixing process selection. Cover both the first-selection and selected-driver paths with the standard fake Client. Assisted-by: GPT-5.6 Sol via Codex
Preserve Client driver rejection and device-ID contracts while separating the loader from payload capabilities and batching. Retain current compiler, SDK, optional-device, and session-lifetime behavior. Assisted-by: GPT-5.6 Sol via Codex
7504600 to
04312c3
Compare
🤖 AI text below 🤖
Description
Load a replaceable QDMI Client driver through its validated C ABI while retaining session ownership, stable device IDs, and existing compiler/SDK behavior.
This is the independent foundation of the driver workstream: v4.1 → #2229 → #2230 → #2231. It depends only on QDMI #511, not metadata cleanup, batching, or payload capabilities. The development pin uses that isolated ABI branch. Published artifacts must use released QDMI 1.4.
Target Core 4.1, never v4.0. The optional private discovery/configuration extension is in #2230; standardizing that interface remains QDMI v2 work. Coordination: #2093.
Mechanical adaptations to bindings, SDKs, compiler device opening, Slurm, and fixtures stay here so this intermediate state builds. Current optional-shot APIs, optional-device builds, variadic gates, zero-arity global phase, and fail-closed target inference are preserved.
Local validation
AI-assisted restructuring; human review remains required.
Deployment acceptance
Demonstrate replacing the driver in an installed C++/Python environment without rebuilding the clients or changing SDK application code. Test the packaged default driver and a standards-only replacement without Core's optional discovery symbols. Preserve the default wheel experience and record compatibility with existing provider libraries.
Checklist
If PR contains AI-assisted content:
🤖 *AI text below* 🤖(titles are exempt).