Fix ModelEditorValueInfoToOnnx dropping symbolic dim names#28307
Open
f-dy wants to merge 3 commits intomicrosoft:mainfrom
Open
Fix ModelEditorValueInfoToOnnx dropping symbolic dim names#28307f-dy wants to merge 3 commits intomicrosoft:mainfrom
f-dy wants to merge 3 commits intomicrosoft:mainfrom
Conversation
Author
|
@microsoft-github-policy-service agree company="Amazon" |
4c34c9d to
2d88e77
Compare
added 2 commits
May 1, 2026 10:37
ModelEditorValueInfoToOnnx had a missing increment bug: `size_t idx = 0;`
was declared before the dim loop but never incremented, so `dim_params[idx]`
always read the first entry. Every dynamic dimension in the generated
ValueInfoProto was assigned the same name — the name of the first dim —
instead of its own.
For a ValueInfo built with shape `[-1, -1, -1]` and symbolic dim names
`["N", "H", "W"]`, the resulting session exposed dims
`["N", "N", "N"]` via `SessionGetInputTypeInfo` →
`GetSymbolicDimensions`.
Downstream, this causes the TensorRT EP to reject valid dynamic shape
profiles. When `trt_profile_{min,opt,max}_shapes` bind distinct ranges
to each dim, TRT (correctly) sees all three ranges attached to the same
symbolic name and errors with:
"N: Input dimensions with this name have different constant values
or have contradictory IOptimizationProfile kMIN/kMAX constraints."
The fix indexes dim_params by the dim position, restoring the invariant
that `dim_params[i]` corresponds to `dims[i]`, consistent with:
- OrtApis::SetSymbolicDimensions (stores names by position)
- OrtApis::GetSymbolicDimensions (reads names by position)
- OrtTypeInfo::FromTypeProto (reconstructs by position)
The doc comment on Basic_CApi already documents the expected behavior:
"call SetDimensions with {-1, 3, 2} and SetSymbolicDimensions with
{\"N\", nullptr, nullptr} to create a shape of {\"N\", 3, 2}".
Adds a regression test SymbolicDimensions_DistinctNamesPreserved that
builds a minimal Identity model via the C++ Model Editor API with three
named dynamic dims, finalizes the session, and asserts the names
round-trip correctly via SessionGetInputTypeInfo. The existing
Basic_CxxApi test happens to use only one non-empty symbolic name
("multiple_of_3", empty on the other dim), which masked this bug:
with only one name, always reading dim_params[0] happens to produce
the correct result.
Adds ModelEditorAPITest.SymbolicDimensions_PartiallyNamedDimensions
which covers the case where only some dimensions carry a symbolic
name and others are either static or dynamic-but-unnamed. Matches
the example given in the Basic_CApi test's doc comment:
"call SetDimensions with {-1, 3, 2} and SetSymbolicDimensions
with {\"N\", nullptr, nullptr} to create a shape of
{\"N\", 3, 2}"
2d88e77 to
0881d46
Compare
Adds ModelEditorAPITest.SymbolicDimensions_CApi_NullNames which
mirrors the literal example from the Basic_CApi test's doc comment:
'call SetDimensions with {-1, 3, 2} and SetSymbolicDimensions
with {"N", nullptr, nullptr} to create a shape of {"N", 3, 2}'
The existing C++ TensorTypeAndShapeInfo wrapper takes
std::vector<std::string>* and represents 'no name' as an empty
string, so the C++ tests cannot exercise the nullptr path directly.
This test drops to the raw C API to cover the nullptr case.
Also updates the partial-naming test's comment to reference this
companion test and clarify the empty-string-vs-nullptr distinction.
2043db2 to
f359ec6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
ModelEditorValueInfoToOnnxinonnxruntime/core/graph/graph.cchad a missing increment bug:size_t idx = 0;was declared before the dim loop but never incremented, sodim_params[idx]always read the first entry. Every dynamic dimension in the generatedValueInfoProtowas assigned the same name — the name of the first dim — instead of its own.For a
ValueInfobuilt with shape[-1, -1, -1]and symbolic dim names["N", "H", "W"], the resulting session exposes dims["N", "N", "N"]viaSessionGetInputTypeInfo→GetSymbolicDimensions.Why this wasn't caught earlier
The only existing
SetSymbolicDimensionscoverage is indirect (ModelEditorAPITest.Basic_CxxApi), and it only passes one non-empty symbolic name ("multiple_of_3"on dim 0, empty on dim 1). With exactly one name, the bug's "always readdim_params[0]" behavior happens to produce the correct result, so the test passes despite the bug.The bug is only observable with ≥2 distinct non-empty symbolic names.
Notably, the comment in
ModelEditorAPITest.Basic_CApialready documents the expected behavior:The comment is correct; the implementation just didn't match.
Impact
Downstream, this causes the TensorRT EP to reject valid dynamic shape profiles. When
trt_profile_{min,opt,max}_shapesbind distinct ranges to each dim, TRT (correctly) sees all ranges attached to the same symbolic name and errors with:I tripped over this in a local project that uses the Model Editor API to bake letterbox preprocessing onto a pose-estimation ONNX model. Python works fine (Python builds the
ModelProtodirectly viaonnx.helperand bypasses the Model Editor API); the equivalent C++ code hits this issue.The fix
Index
dim_paramsby the same position asdims:if (tensor_info.HasShape()) { auto& shape = *tensor->mutable_shape(); - size_t idx = 0; const auto dims = tensor_info.GetShape()->GetDims(); const auto& dim_params = *tensor_info.GetDimParams(); - for (auto dim : dims) { + for (size_t idx = 0; idx < dims.size(); ++idx) { + const auto dim = dims[idx]; auto& dim_proto = *shape.add_dim(); if (dim >= 0) { dim_proto.set_dim_value(dim); } else { const std::string& dim_param = dim_params[idx]; // if empty leave the new dim_proto with neither dim_value nor dim_param set. this represents an 'unknown' dim if (!dim_param.empty()) { dim_proto.set_dim_param(dim_param); } } } }This restores the invariant that
dim_params[i]corresponds todims[i], which is already how:OrtApis::SetSymbolicDimensionsstores names (tensor_type_and_shape.cc)OrtApis::GetSymbolicDimensionsreads names (tensor_type_and_shape.cc)OrtTypeInfo::FromTypeProtoreconstructs the info in the reverse direction (onnxruntime_typeinfo.cc)Regression tests
Two new tests in
onnxruntime/test/shared_lib/test_model_builder_api.cc:SymbolicDimensions_DistinctNamesPreserved— builds a minimal Identity model via the C++ Model Editor API with three named dynamic dims{"N", "H", "W"}, finalizes the session, and assertsGetInputTypeInfo(0)→GetSymbolicDimensions()returns{"N", "H", "W"}. Without the fix, the second and third names would both be"N"(the first name).SymbolicDimensions_PartiallyNamedDimensions— covers the mixed case where some dims are named, some are static, and some are dynamic-but-unnamed. Shape[-1, 3, -1]with names{"N", "", ""}. This matches the exact example given in the existingBasic_CApitest's doc comment.The bug was first observed in a separate C++ application using the Model Editor API to bake preprocessing onto a YOLO-pose model;
GetInputTypeInfoafterFinalizeModelEditorSessionconsistently returned['N','N','N',3]instead of the['N','H','W',3]that was set viaSetSymbolicDimensions. These regression tests exercise the same code path within ORT's own test suite so CI will catch any re-introduction.Motivation and context
This blocks using the C++ Model Editor API to build models with multi-named dynamic shapes that must then run through the TensorRT EP with dynamic shape profiles — a reasonable use case for any application that bakes preprocessing on top of a dynamic-input model (e.g. letterboxing for detection / pose networks).