Segregate IExecutionProvider optional capabilities into mix-in interfaces - #29087
Segregate IExecutionProvider optional capabilities into mix-in interfaces#29087GopalakrishnanN wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new set of optional-capability mix-in interfaces for IExecutionProvider and adds typed query hooks on IExecutionProvider so callers can explicitly detect and use supported capabilities (graph capture, tuning, compilation, data-layout) without depending on the full base interface.
Changes:
- Add
execution_provider_capabilities.hdefiningIGraphCaptureCapability,ITuningCapability,IDataLayoutCapability, andICompileCapability(build-guarded). - Add
Get*Capability()query hooks toIExecutionProvider(defaulting tonullptr) to expose supported mix-ins. - Add unit tests using lightweight fake EPs to validate the query mechanism and capability independence.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| onnxruntime/test/framework/execution_provider_capabilities_test.cc | Adds unit tests exercising the new capability query hooks with fake EP implementations. |
| include/onnxruntime/core/framework/execution_provider.h | Adds forward declarations and the new Get*Capability() query hooks on IExecutionProvider. |
| include/onnxruntime/core/framework/execution_provider_capabilities.h | Introduces the segregated optional-capability mix-in interface definitions. |
| /** Return this EP's TunableOp tuning capability, or nullptr if unsupported. */ | ||
| virtual ITuningCapability* GetTuningCapability() const noexcept { return nullptr; } | ||
|
|
||
| /** Return this EP's data-layout preference capability, or nullptr if unsupported. */ | ||
| virtual IDataLayoutCapability* GetDataLayoutCapability() const noexcept { return nullptr; } |
There was a problem hiding this comment.
Addressed in d52ecb0: GetDataLayoutCapability() now returns const IDataLayoutCapability*, and the tuning / graph-capture / compile hooks are non-const because those capabilities are on the mutating path. No const_cast remains.
| ITuningCapability* GetTuningCapability() const noexcept override { | ||
| return const_cast<TuningEp*>(this); | ||
| } |
There was a problem hiding this comment.
Fixed in d52ecb0 - the override returns this with no const_cast.
| IDataLayoutCapability* GetDataLayoutCapability() const noexcept override { | ||
| return const_cast<DataLayoutEp*>(this); | ||
| } |
There was a problem hiding this comment.
Fixed in d52ecb0 - GetDataLayoutCapability() returns const IDataLayoutCapability* and the override returns this directly.
|
|
||
| // An EP that supports graph capture, implemented via the segregated mix-in and | ||
| // surfaced through the GetGraphCaptureCapability() query hook. | ||
| class GraphCaptureEp : public IExecutionProvider, public IGraphCaptureCapability { |
There was a problem hiding this comment.
class GraphCaptureEp : public IExecutionProvider, public IGraphCaptureCapability
The goal of this PR is not clearly articulated. What does it gain, memory, performance?
It does make objects binary layout more complicated as this object now has more than one virtual table and casting now has to be done very carefully.
There was a problem hiding this comment.
Fair pushback, and I have rewritten the PR description to state the intent explicitly. Summary:
- This is a structural / interface-segregation refactor, NOT a memory or performance change. Object layout does gain a vtable per implemented mix-in, so it is a (negligible) cost, not a saving. The justification is API design.
- The gain is explicit, typed capability detection. Today an optional capability is a defaulted no-op virtual on the base, so which EP supports X is unanswerable and every caller can call it on every EP. A query hook makes it first-class: if (auto* c = ep->GetGraphCaptureCapability()) use c.
- It lowers EP authoring/review cost: a capability is one narrow interface, and a new/plugin EP author sees a focused per-capability contract instead of the full IExecutionProvider surface.
The multiple-vtable / careful-casting concern is real. The change is deliberately additive: the legacy virtuals stay, the hooks default to nullptr, and the only cast is confined to the opting-in EP returning this. If the ISP gain is not judged worth the extra vtable, I am happy to park this.
| virtual IGraphCaptureCapability* GetGraphCaptureCapability() noexcept { return nullptr; } | ||
|
|
||
| /** Return this EP's TunableOp tuning capability, or nullptr if unsupported. */ | ||
| virtual ITuningCapability* GetTuningCapability() const noexcept { return nullptr; } |
There was a problem hiding this comment.
Now, instead of those default methods we need to deal with default methods that cast to the correct interface by dispatching it to the correct virtual table (it now has more than one), however, this will not compile cleanly as if the method is const.
Now if you return this it has a const qualifier which will have to be cast away which we do not want but the const_cast now is dictated by the interface.
There was a problem hiding this comment.
Addressed in d52ecb0. The hooks are now split by whether acquiring the capability is read-only or on the mutating path:
- GetDataLayoutCapability() const returns const IDataLayoutCapability* (pure query).
- GetGraphCaptureCapability(), GetTuningCapability(), GetCompileCapability() are non-const and return non-const pointers (they expose mutating operations).
Implementers now return this with no const_cast.
|
|
||
| // ITuningCapability. Returns nullptr (no real tuning state); a call counter | ||
| // proves the call is routed to the concrete implementation through the mix-in. | ||
| ITuningContext* GetTuningContext() const override { |
There was a problem hiding this comment.
Agreed - that was the signal that tuning does not belong behind a const query. GetTuningCapability() is now non-const (d52ecb0), so acquiring the tuning capability is honestly on the mutating path rather than reachable through a const EP reference.
| int tuning_query_count() const { return tuning_query_count_; } | ||
|
|
||
| private: | ||
| mutable int tuning_query_count_ = 0; |
There was a problem hiding this comment.
Removed in d52ecb0. ITuningCapability::GetTuningContext() is pure virtual, so reaching the concrete result already proves the call routed through the mix-in - the mutable call-counter was unnecessary and is gone.
…aces Introduce IGraphCaptureCapability, ITuningCapability, and ICompileCapability mix-in interfaces that cluster the optional graph-capture, tuning, and compilation methods currently spread across IExecutionProvider as defaulted virtuals. Add non-breaking capability-query hooks (GetGraphCaptureCapability/GetTuningCapability/GetCompileCapability) that default to nullptr so callers can discover support by capability instead of relying on the large defaulted-virtual surface. Legacy virtuals are retained for compatibility; this is an additive first step toward migrating EPs off them. Add a CPU-only unit test proving unsupported capabilities return null, an exposed capability is queryable and usable, and capabilities are independent.
…erface Add IDataLayoutCapability grouping the coupled GetPreferredLayout() and ShouldConvertDataLayoutForOp() virtuals, with a GetDataLayoutCapability() query hook on IExecutionProvider (defaults to nullptr). Additive and non-breaking; only NHWC-preferring EPs implement it. Includes fake-EP unit tests.
…dispatch Add positive 'queryable and usable' tests for ITuningCapability (call-counted routing) and ICompileCapability (Compile + ValidateCompiledModelCompatibilityInfo), a multi-capability EP test verifying correct mix-in dispatch under multiple inheritance, and compile-null independence checks. 7 tests pass.
Address review feedback on the segregated capability hooks: - GetDataLayoutCapability() now returns a const IDataLayoutCapability* (data-layout preference is a read-only query, so it must not allow mutating the EP through a const reference). - GetTuningCapability() is now non-const, signalling that acquiring the tuning capability is part of the mutating execution path (tuning records state through ITuningContext). - Implementations return 'this' directly; removes the const_cast in every mix-in override and the mutable call-counter in the tuning test.
7da74ea to
d52ecb0
Compare
Review summaryWhat this is: a purely additive interface-segregation refactor (478+/0−, 3 files) — four narrow mix-ins ( Verdict: no blocking issues — this is safe to land as an inert, spec-sound infrastructure step. I verified the linchpin directly: all 11 mix-in signatures mirror the legacy The items below are about whether the PR achieves its stated goal and about future-migration safety, not about the code that lands. Findings
The four MajorsM1 — Discovery/implementation can silently disagree (the central risk). Because the base hooks default to M2 — No drift guard. The "one override, two bases" contract holds only while the signatures stay identical by hand. The dangerous drift case is subtle: if a future edit leaves the non-pure legacy virtual un-overridden (e.g. a M3 — Const asymmetry. M4 — Docs drop safety contracts. Legacy Minor / nits worth folding in
Verification notes / scope
Overall: merge-ready as inert infrastructure. To make it deliver its intended value safely, the cheap nets (M2 |
…ift guard - Restore legacy doc caveats the mix-ins had dropped: ICompileCapability::Compile now warns not to cache the GraphViewer beyond the call, and IGraphCaptureCapability::ReleaseCapturedGraph documents the concurrent-Run thread-safety contract. - Make GetTuningCapability() a const, read-only query returning const ITuningCapability*, matching the const legacy GetTuningContext() and the const data-layout hook. Update the TuningEp fake and its test accordingly. - Add compile-time signature-drift static_asserts (in the unit test) pinning each mix-in method to its legacy IExecutionProvider counterpart, so a future change that would silently split the shared vtable slot fails the build instead. - Strengthen the tuning test with a subobject-identity assertion and note that ReplayGraph's sync default must stay equal across both declarations. - Fix the stale motivation comment to include the data-layout cluster, add legacy->mix-in cross-references, and drop the now-unused <memory> include. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Addressed the review feedback in 945af36. Summary of what changed:
Deferred as follow-ups since this PR is intentionally infra-only: wiring a production EP through a mix-in as a vertical slice (+ parity tests), and version-locking the shared-provider bridge. |
What this PR does
Introduces narrow, optional-capability mix-in interfaces for execution providers in a new header
core/framework/execution_provider_capabilities.h, plus matchingGet*Capability()query hooks onIExecutionProviderthat return the capability pointer when supported andnullptrotherwise.IGraphCaptureCapabilityIsGraphCaptureEnabled,IsGraphCaptured,ReplayGraph,ReleaseCapturedGraph,GetGraphCaptureNodeAssignmentPolicyGetGraphCaptureCapability()ITuningCapabilityGetTuningContextGetTuningCapability()IDataLayoutCapabilityGetPreferredLayout,ShouldConvertDataLayoutForOpGetDataLayoutCapability()ICompileCapability(non-minimal / extended-minimal build)Compile,GetCompiledModelCompatibilityInfo,ValidateCompiledModelCompatibilityInfoGetCompileCapability()What this PR is — and is not
This is a structural / readability refactor, not a runtime optimization. To set expectations honestly:
if (auto* c = ep->GetGraphCaptureCapability()) { ... }.IExecutionProvidersurface.Why bundling on the base is worth addressing
IExecutionProvideris the most widely implemented interface in ORT and has accumulated many defaulted virtuals for capabilities only a subset of EPs implement (CUDA-graph capture, TunableOp tuning, subgraph compilation, non-default data layout). Bundling them on the base couples every EP and every caller to the union of all capabilities. Segregating each cluster behind a queryable mix-in is a step toward anIExecutionProviderwhose base surface is the contract every EP must satisfy, with optional behavior expressed à la carte.Const-correctness (addresses review feedback)
The hooks are now split by whether acquiring the capability is a read-only query or part of the mutating path, which removes the earlier
const_cast/mutableworkarounds:GetDataLayoutCapability() constreturns aconst IDataLayoutCapability*. Data-layout preference is purely read-only (GetPreferredLayout/ShouldConvertDataLayoutForOpareconst), so it must not allow mutating an EP through a const reference. Implementationsreturn this;from a const method with noconst_cast.GetGraphCaptureCapability(),GetTuningCapability(), andGetCompileCapability()are non-const and return non-const pointers, because those capabilities expose mutating operations (graph replay/release, tuning state recorded throughITuningContext, subgraph compilation). Acquiring them is honestly part of the mutating execution path, not a const query.As a result there is no
const_castin any implementation or test, and the test fakes no longer need amutablecall-counter —ITuningCapability::GetTuningContext()is pure virtual, so reaching the concrete result already proves the call routed through the mix-in.Compatibility
Purely additive and non-breaking:
IExecutionProviderremain in place; existing EPs and callers are unaffected.nullptr; an EP opts in only by implementing a mix-in.Follow-up (intentionally not in this PR)
Slimming the base for the cleanest cluster — data layout — is a deliberate, separate change.
GetPreferredLayout()/ShouldConvertDataLayoutForOp()are overridden by ~9 EPs (CUDA, QNN, NNAPI, XNNPACK, WebGPU, WebNN, JS, the plugin bridge, and the internal-testing EP) and called fromgraph_partitioner.ccandlayout_transformation.cc. Migrating all implementers and callers and then deleting the base virtuals is the end-to-end "before/after" that demonstrates the coupling reduction — but it touches several EPs that are not built in a CPU-only configuration, so it is kept out of this infrastructure-only PR to keep this change reviewable and independently verifiable.Tests
onnxruntime/test/framework/execution_provider_capabilities_test.ccexercises the hooks with lightweight CPU-only fake EPs:nullptrfrom every hook;constcapability with no casts.