onnx-graphsurgeon - fold value_float constants as float32 in fold_constants - #4834
Open
VaggelisGian wants to merge 1 commit into
Open
onnx-graphsurgeon - fold value_float constants as float32 in fold_constants#4834VaggelisGian wants to merge 1 commit into
VaggelisGian wants to merge 1 commit into
Conversation
Constant nodes carrying value_float or value_floats attributes were lowered
by fold_constants() into float64 tensors because np.array defaults to double
for Python floats. ONNX defines both attributes as float32, so the folded
initializers had the wrong type and models produced from them were rejected
by ONNX Runtime with a type error on consumers such as Add.
Lower the two attributes explicitly as float32 and cover both with a
regression test asserting dtype, shape and values of the folded constant.
value_int and value_ints already land on int64 through the default path,
which matches their ONNX types, so they are left alone.
Test Plan:
cd tools/onnx-graphsurgeon
PYTHONPATH=$PWD python -m pytest "tests/ir/test_graph.py::TestFoldConstants::test_value_float_attrs_fold_as_float32"
fails before the fix (initializer dtype DOUBLE), passes after
PYTHONPATH=$PWD python -m pytest tests --ignore=tests/test_examples.py
281 passed, 1 skipped (test_examples excluded: its harness spawns
bash with python3, which does not exist on Windows)
Signed-off-by: Vaggelis <baggelis100@gmail.com>
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
1 participant
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.
What does this PR do?
Graph.fold_constants()loweredConstantnodes carrying thevalue_floatorvalue_floatsattributes into float64 (DOUBLE) tensors, becausenp.arraydefaults to double precision for Python floats. ONNX defines both attributes as float32, so the folded initializers had the wrong type and models produced from them were rejected by ONNX Runtime with a type error on consumers such asAdd.Reproduction before this fix: build a graph with a
Constant(value_float=...)node, runfold_constants(), export. The folded initializer comes out withdata_type = 11(DOUBLE) and ONNX Runtime refuses to load the model. After the fix it isdata_type = 1(FLOAT) and loads cleanly.The fix lowers both attributes explicitly as float32.
value_intandvalue_intsalready land on int64 through the default path, which matches their ONNX types, so they are left alone.Testing
New regression test
tests/ir/test_graph.py::TestFoldConstants::test_value_float_attrs_fold_as_float32, parametrized over both attributes; asserts dtype, shape and values of the folded constant. It fails on unpatched main (folded initializer dtypeDOUBLE) and passes after.Commands and output on this machine:
test_examples.pyis excluded only because its harness spawnsbash -c python3, unavailable on Windows; unrelated to this change.Environment
TensorRT OSS main (10d15ae), onnx-graphsurgeon sources run from the repo.
Python 3.12, onnx 1.20.0, numpy 2.5.2, Windows 11 Pro.
Issue
Per CONTRIBUTING.md, bugfixes start as an issue approved by TensorRT engineers before code review. I could not find an existing issue covering this; happy to file one and link it here if maintainers prefer that flow, or please advise whether this PR can proceed directly.