Skip to content

♻️ Load a replaceable QDMI Client driver - #2229

Draft
burgholzer wants to merge 4 commits into
mainfrom
codex/qdmi-client-runtime-c1
Draft

♻️ Load a replaceable QDMI Client driver#2229
burgholzer wants to merge 4 commits into
mainfrom
codex/qdmi-client-runtime-c1

Conversation

@burgholzer

@burgholzer burgholzer commented Aug 24, 2026

Copy link
Copy Markdown
Member

🤖 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: main → #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

  • Full release build and CTest: 3,869 passed, one existing skip.
  • Selected QDMI, SDK, and MLIR Python tests: 455 passed.
  • Stub generation, repository lint, and full changed-file C++ lint passed.
  • Hosted CI is separate and must be checked on the final pushed head.

AI-assisted restructuring; human review remains required.

Checklist

  • The pull request only contains commits that are focused and relevant to this change.
  • I have added appropriate tests that cover the new/changed functionality.
  • I have updated the documentation to reflect these changes.
  • I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals.
  • I have added migration instructions to the upgrade guide (if needed).
  • The changes follow the project's style guidelines and introduce no new warnings.
  • The changes are fully tested and pass the CI checks.
  • I have reviewed my own code changes.

If PR contains AI-assisted content:

  • Any agent that created, edited, or submitted GitHub content was explicitly authorized for that scope, as required by our AI Usage Guidelines.
  • Every agent-authored or agent-edited public text body begins with the visible disclosure 🤖 *AI text below* 🤖 (titles are exempt).
  • I have disclosed AI assistance in the PR description.
  • I confirm that I have personally reviewed and understood all AI-generated content, and accept full responsibility for it.

@burgholzer burgholzer added dependencies Pull requests that update a dependency file feature New feature or request c++ Anything related to C++ code python Anything related to Python code packaging Anything related to Python packaging QDMI Anything related to QDMI labels Aug 24, 2026
@burgholzer burgholzer self-assigned this Aug 24, 2026
@burgholzer burgholzer added dependencies Pull requests that update a dependency file feature New feature or request c++ Anything related to C++ code python Anything related to Python code packaging Anything related to Python packaging QDMI Anything related to QDMI labels Aug 24, 2026
@burgholzer
burgholzer force-pushed the codex/qdmi-client-runtime-c1 branch 2 times, most recently from 010d76c to 295a166 Compare August 24, 2026 16:50
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.33333% with 26 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/qdmi/Client.cpp 93.2% 17 Missing ⚠️
src/qdmi/driver/Driver.cpp 73.3% 8 Missing ⚠️
mlir/lib/Compiler/QDMIAdapter.cpp 87.5% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@mergify mergify Bot added the conflict label Aug 24, 2026
@burgholzer
burgholzer force-pushed the codex/qdmi-client-runtime-c1 branch from 295a166 to 785c041 Compare August 24, 2026 18:13
@mergify mergify Bot removed the conflict label Aug 24, 2026
@burgholzer
burgholzer force-pushed the codex/qdmi-client-runtime-c1 branch from 785c041 to 7eb542d Compare August 24, 2026 18:59
@burgholzer
burgholzer force-pushed the codex/qdmi-client-runtime-c1 branch 2 times, most recently from 2aa8996 to cfc4815 Compare August 24, 2026 19:06
@burgholzer
burgholzer force-pushed the codex/qdmi-client-runtime-c1 branch from cfc4815 to 3de68c6 Compare August 24, 2026 20:10
@burgholzer
burgholzer force-pushed the codex/qdmi-client-runtime-c1 branch from 3de68c6 to 32bb143 Compare August 24, 2026 23:47
@burgholzer burgholzer added this to the QDMI Support milestone Aug 25, 2026

@burgholzer burgholzer left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely ClientAPI would be better suited here. There may be other places benefitting from the improved spelling.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @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

Comment on lines +103 to +104
ClientSession(const ClientSession&) = delete;
ClientSession& operator=(const ClientSession&) = delete;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this define (defaulted) move constructors and assignment?

Comment thread src/qdmi/Client.cpp

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/qdmi/Client.cpp
Comment on lines +172 to +175
{directory / filename, directory / "lib" / filename,
directory / "bin" / filename,
directory.parent_path() / "lib" / filename,
directory.parent_path() / "bin" / filename}) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also cover lib64. IIRC we had problems with that already at some point on manylinux images.

Comment thread src/qdmi/Client.cpp
-> LoadedClient {
auto* const library = openLibrary(path);
if (library == nullptr) {
throw std::runtime_error("Cannot load QDMI Client driver '" +

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to annotate this once more: We shouldn't use "QDMI Client driver" but simply "QDMI driver".

Comment thread src/qdmi/Client.cpp
Comment on lines +1263 to +1282
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);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread src/qdmi/Client.cpp

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
@burgholzer
burgholzer force-pushed the codex/qdmi-client-runtime-c1 branch from 7504600 to 04312c3 Compare September 4, 2026 10:19
@burgholzer
burgholzer changed the base branch from codex/classical-control-support to main September 4, 2026 10:19
@mergify mergify Bot added the conflict label Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Anything related to C++ code conflict dependencies Pull requests that update a dependency file feature New feature or request packaging Anything related to Python packaging python Anything related to Python code QDMI Anything related to QDMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant