Skip to content

fix(transpose_optimizer): handle empty node names when synthesizing NodeArg names #28729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Rishi-Dave wants to merge 4 commits into
base: main
Choose a base branch
from
+75 −4
Open

fix(transpose_optimizer): handle empty node names when synthesizing NodeArg names #28729

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
84bc2ac
fix(transpose_optimizer): handle empty node names when synthesizing N…
Rishi-Dave May 31, 2026
6c7e4f2
fix(transpose_optimizer): update test expectations for synthesized no…
Rishi-Dave Jun 3, 2026
5fdcd3f
docs(transpose_optimizer): clarify empty-node-name collision comment
Rishi-Dave Jun 3, 2026
54742ad
test(transpose_optimizer): add regression for empty-named-node NodeAr…
Rishi-Dave Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions onnxruntime/core/optimizer/transpose_optimization/ort_optimizer_api_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,13 @@ static Node& CreateNodeHelper(onnxruntime::Graph& graph, std::string_view op_nam
std::string_view domain, int since_version, std::string_view node_ep) {
const std::string op_type_str(op_type);
const std::string op_name_str(op_name);
std::string name = graph.GenerateNodeName(op_name_str);
// Use op_type as seed when op_name is empty (e.g., after function inlining) to avoid
// output NodeArg name collisions. When the source node's name is empty,
// Graph::GenerateNodeName("") can legally return "" (if no prior empty-named node has been
// registered). That empty base then seeds output arg names as "_out0", "_out1", ..., which
// can collide with existing NodeArgs already present in the graph.
const std::string& name_seed = op_name_str.empty() ? op_type_str : op_name_str;
Comment on lines +651 to +656
std::string name = graph.GenerateNodeName(name_seed);
std::vector<NodeArg*> input_args;
std::vector<NodeArg*> output_args;

Expand Down Expand Up @@ -832,7 +838,9 @@ void ApiGraph::MoveOutput(api::NodeRef& src_node, size_t src_idx, api::NodeRef&

graph_utils::GraphEdge::RemoveGraphEdges(graph_, output_edges);

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();

@tianleiwu tianleiwu Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

std::string new_name = graph_.GenerateNodeArgName(base);
src_output_defs[src_idx] = &graph_.GetOrCreateNodeArg(new_name, nullptr);
graph_.UpdateProducerNode(new_name, src_node_idx);
}
Expand Down
4 changes: 2 additions & 2 deletions onnxruntime/test/internal_testing_ep/internal_testing_tests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ TEST(InternalTestingEP, TestMixOfStaticAndCompiledKernels) {

// Error message should come from the Conv implementation with the statically registered kernel
ASSERT_STATUS_NOT_OK_AND_HAS_SUBSTR(session.Run(feeds, output_names, &fetches),
"Non-zero status code returned while running Conv node. Name:'_token_2' "
"Non-zero status code returned while running Conv node. Name:'Conv' "
"Status Message: TODO: add NHWC implementation here.");
}

Expand Down Expand Up @@ -272,7 +272,7 @@ TEST(InternalTestingEP, TestNhwcConversionOfStaticKernels) {
std::vector<OrtValue> fetches;

ASSERT_STATUS_NOT_OK_AND_HAS_SUBSTR(session.Run(feeds, output_names, &fetches),
"Non-zero status code returned while running Conv node. Name:'_token_2' "
"Non-zero status code returned while running Conv node. Name:'Conv' "
"Status Message: TODO: add NHWC implementation here.");
};

Expand Down
Loading