fix(transpose_optimizer): handle empty node names when synthesizing NodeArg names#28729
Conversation
…odeArg names When ONNX function inlining or QDQPropagationTransformer leaves nodes with empty names, CreateNodeHelper and MoveOutput were seeding GenerateNodeName / GenerateNodeArgName with an empty string, producing colliding names such as "_out0". If two such nodes had different element types (e.g. uint8 vs int8), Graph::Resolve() would fail with a TypeError. Fix both call sites to fall back to the node's op type as the seed when the node name is empty, ensuring unique, type-descriptive generated names. Fixes microsoft#28716
|
That would be great to have a unit test checking this case. |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a failure mode in the TransposeOptimizer’s ORT Graph adapter where empty Node::Name() values could be used as the seed for Graph::GenerateNodeName() / Graph::GenerateNodeArgName(), leading to non-unique or otherwise problematic synthesized names and ultimately Graph::Resolve() type errors during session initialization with graph optimizations enabled.
Changes:
- In
CreateNodeHelper, fall back to using the node’s op type as the node-name seed when the providedop_nameis empty. - In
ApiGraph::MoveOutput, fall back to using the source node’s op type as the NodeArg-name seed when the source node name is empty.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Use op_type as seed when op_name is empty (e.g., after function inlining) to avoid | ||
| // colliding generated arg names like '_out0' when two empty-named nodes exist in the graph. | ||
| const std::string& name_seed = op_name_str.empty() ? op_type_str : op_name_str; |
|
A couple of tests are failing due to the naming changes. 2026-06-02T16:43:39.6801876Z [ FAILED ] InternalTestingEP.TestMixOfStaticAndCompiledKernels |
…de names
The internal_testing_ep tests TestMixOfStaticAndCompiledKernels and
TestNhwcConversionOfStaticKernels asserted on the previous synthesized
name ('_token_2'). Now that CreateNodeHelper seeds GenerateNodeName with
the op type for empty-named nodes, the resulting name is 'Conv'. Update
the expected substrings to match.
Also clarify the inline comment in CreateNodeHelper to describe the
actual collision mechanism: GenerateNodeName("") can return "" on its
first call, which then seeds output arg names like "_out0" and collides
when a second empty-named node is processed later.
Reword the comment in CreateNodeHelper to accurately describe the
failure mode flagged in PR review: Graph::GenerateNodeName("") can
legally return an empty string on the first empty-named node (no prior
empty-named node registered), and that empty base then seeds output
NodeArg names as _out0, _out1, ..., which can collide with existing
NodeArgs already present in the graph. The previous wording implied
two empty-named nodes were required, which was inaccurate.
|
Reworded the comment in |
tianleiwu
left a comment
tianleiwu
left a comment
There was a problem hiding this comment.
Reviewed the empty-node-name fix. The change is correct, minimal, and well-scoped.
Why it matters: In MoveOutput, when src_ort_node.Name() was empty, GenerateNodeArgName("") could return the empty string, and GetOrCreateNodeArg("", nullptr) then synthesizes a NodeArg whose empty name carries ONNX "missing/optional output" semantics — that is the real correctness bug being fixed. Seeding with OpType() guarantees a non-empty, unique base. In CreateNodeHelper, the op-type seed similarly keeps the downstream name + "_out" + i arg names unique and descriptive.
Lifetime check: Both const std::string& ternaries bind to long-lived objects (the op_type_str/op_name_str locals, and the Node::OpType()/Node::Name() member references), so there is no dangling-reference issue. The non-empty path is unchanged.
Suggestion (non-blocking): Per repo conventions, a behavioral fix should carry a regression test. The empty-name path is reachable by constructing a graph via Graph::AddNode("", ...) directly (bypassing ModelTestBuilder, which always assigns a non-empty name), running the transpose optimizer, and asserting Graph::Resolve() succeeds with no empty NodeArg names. That would lock in the fix end-to-end rather than relying on incidental CI coverage of inlined-function graphs.
|
|
||
| std::string new_name = graph_.GenerateNodeArgName(src_ort_node.Name()); | ||
| // Use op type as seed when node name is empty to avoid colliding generated arg names. | ||
| const std::string& base = src_ort_node.Name().empty() ? src_ort_node.OpType() : src_ort_node.Name(); |
There was a problem hiding this comment.
This is the higher-impact of the two fixes. Before this change, an empty src_ort_node.Name() could cause GenerateNodeArgName("") to return "", and the subsequent GetOrCreateNodeArg("", nullptr) would create a NodeArg whose empty name is interpreted as a missing/optional output by ONNX — corrupting the producer wiring. Seeding with OpType() avoids that. A focused regression test that builds a graph with an empty-named source node and asserts the synthesized new_name is non-empty/unique would be worth adding.
…g collision Constructs a graph with two Transpose nodes whose Node::Name() is empty, runs the transpose optimizer, and asserts Graph::Resolve() still succeeds and that no NodeArg has an empty or colliding name. Addresses review feedback on PR microsoft#28729.
|
Thanks for the review. The regression test you suggested is already on this branch — commit 54742ad ( It constructs a graph with two Transpose nodes whose
That exercises the |
Description
When ONNX function inlining (or
QDQPropagationTransformer) leaves nodes with empty names, theTransposeOptimizerinort_optimizer_api_impl.ccwas seedingGenerateNodeName/GenerateNodeArgNamewith an empty string. Two empty-named nodes of different element types (e.g. uint8 vs int8QuantizeLinear) would then produce colliding generated NodeArg names such as_out0, causingGraph::Resolve()to fail with a TypeError during session initialization whenever graph optimization was enabled atORT_ENABLE_BASICor higher.This change makes both call sites fall back to the node's op type as the seed when the node name is empty, ensuring unique, descriptive generated names while leaving the non-empty path byte-for-byte identical to before.
Motivation and Context
Fixes #28716.
Changes
onnxruntime/core/optimizer/transpose_optimization/ort_optimizer_api_impl.ccCreateNodeHelper: whenop_nameis empty, seedGenerateNodeNamewithop_typeinstead of the empty string. DownstreamGenerateNodeArgName(name + "_out" + i)calls inherit the fix automatically becausenameis now globally unique.MoveOutput: when the source node's name is empty, seedGenerateNodeArgNamewithNode::OpType()instead ofNode::Name().The only other
GenerateNodeArgNamecall in the file (AddInitializer) uses a hard-coded literal seed and is unaffected.Test Plan
TestQuantizeLinearScalar,TestQuantizeLinearVector, etc.) continue to exercise both modified call paths with non-empty node names; their behavior is unchanged by construction (the ternary short-circuits to the original path).QDQPropagationTransformer. CI integration coverage of inlined-function graphs will exercise the fix end-to-end. A targeted regression unit test was considered but would require constructing a graph with empty-named nodes via directGraph::AddNode(bypassingModelTestBuilder, which always assigns a non-empty name); happy to add one if reviewers prefer.