Add dynamic shape explainer - #945
Conversation
|
A POC based on the current explainer is WIP. And it has been validated against some real-world Transformer, LLM and image generation models. |
fdwr
left a comment
fdwr
left a comment
There was a problem hiding this comment.
馃憤 I have some thoughts, but it's 95% 馃憣.
| ## Goals | ||
| - Allow a single compiled `MLGraph` to execute across varying runtime input sizes, without rebuilding. | ||
|
|
||
| - Model dynamism the way the underlying runtimes already do: a dimension is either a **static size**, a **named dynamic** dimension (a symbolic name), or an **unnamed dynamic** dimension (fully unconstrained). |
There was a problem hiding this comment.
Do you think we even need unnamed dimensions? A uniquely named dynamic dimension and an unnamed dynamic dimension are identical after all, and it's preferable to have debuggable symbols. The only reason to support them would be because existing callers may have them (like ORTWeb calling WebNN), but we could always just synthesize a name on the fly like "inputTensorName2_axis3" 馃. I mean, if I was debugging and hit a shape inference error, I'd rather see that than just null for a name. Alternately using "" instead of null could be less problematic (no need to check for null first before trying to use/print the string).
Actually, seeing generated names would probably help you too during WebNN/Chromium debugging, seeing where pass-through fails during shape inference.
There was a problem hiding this comment.
Some history, since we did have derived names early on and removed them deliberately. We generated things like "height-3" and "broadcast_1_hight". The names weren't unique and two unrelated conv/slice calls would collide on the same name and the backend would conflate two independent dimensions. We patched that with a per-builder counter. Additionally, when we created the POC on the ORT backend at the begining, we only called OrtApi::SetDimensions. Regardless of the name, any dim that was dynamic was ultimately set to -1. Then we dropped derived names entirely in the rewrite and narrowed the rule to "a name survives only on 1:1 pass-through".
A synthesized name that is unique and implies no relationship is semantically identical to an anonymous dim, and it's strictly better to look at while debugging. I agree with the direction and will take it.
There was a problem hiding this comment.
Done, the explainer now requires every dynamic dimension to carry a name, and the unnamed state is gone. MLInputOperandDescriptor.shape drops its nullable elements entirely (sequence<MLDimension>), so null is no longer accepted at all and the empty string is rejected too.
@huningxin Do you have any thoughts on this point?
|
anssiko marked as non substantive for IPR from ash-nazg. |
|
I cleared the automatic IPR check for this PR, since explainer documents are consider non-substantive from the W3C Patent Policy point of view. DetailsThis IPR check is to ensure normative portions of the specification come from organizations who participate the WG. Furthermore, in this case, I can attest the authors of this PR are affiliated with Intel, and as such, any normative portions are reusable in the spec PR, as appropriate. |
|
@reillyeon you had good questions comments on our last call for this. Do you have some other Googlers in mind who should review this explainer PR? When this PR lands, the team will start landing the implementation in smaller chunks for further validation of this approach. |
reillyeon
left a comment
reillyeon
left a comment
There was a problem hiding this comment.
Overall I think this approach is reasonable. My main concern is that by making shapes dynamic there are more opportunities for memory safety issues in the implementation.
|
With two approvals, editors are welcome to merge this explainer PR at will. Thank you @miaobin for this contribution and Reilly, Dwayne, everyone for your review. |
|
|
||
| - **Shape computation never reads input data**, as described above, which also bounds what a graph can make the interpreter do. | ||
|
|
||
| - **Bounded constant collection.** The interpreter seeds only from the shape operands of the dynamic operators and walks back along the shape-computation chain, copying only the constants actually on that chain (and skipping oversized constants). Weight tensors are never on a shape chain and are not copied, bounding both memory and shape-computation work (a DoS/OOM guard). |
There was a problem hiding this comment.
Re "(and skipping oversized constants)", what are the oversized constants? Do you mean its length is over maximum rank limit? Should we reject rather than skip it?
There was a problem hiding this comment.
This is actually an implementation level design choice. To prevent the collection of large constants, we limit the maximum number of elements. Here, "skip" refers to the practice of not collecting a constant when an oversized one is encountered during the collection phase.
The description of this passage has been refined.
There was a problem hiding this comment.
we limit the maximum number of elements
What's the limit? Do you mean validate against valid dimension?
There was a problem hiding this comment.
What's the limit? Do you mean validate against valid dimension?
The limit is the rank of a constant. A constant with more elements than the limit is not a shape related vector, it is a weight, and we will not copy it.
6 participants
This is the initial draft to summarize the discussion on dynamic shapes #883 .
The explainer covers named/unnamed dynamic dimensions, deferred (dispatch-time) shape validation,
computeShapes()API, and a new family of shape-as-data (*Dynamic) operators.Open questions and considered alternatives are called out explicitly in the explainer and feedback on this doc would be very welcome.