Conversation
…type dtype_to_onnx mapped ml_dtypes.float8_e4m3fnuz to "FLOAT8E4M3FN", so a Constant holding float8_e4m3fnuz values was exported with the wrong ONNX data type. FLOAT8E4M3FN and FLOAT8E4M3FNUZ have the same storage width, so this failed silently: the exported raw_data was byte-identical and only the type label was wrong, which makes any consumer that decodes the bytes according to that label read different values. The mismatch also made the import/export round trip asymmetric. Import already handles FLOAT8E4M3FNUZ correctly, but materializing a lazy initializer and exporting it again relabelled the tensor from 18 to 17. float8_e5m2fnuz is mapped correctly in the same table, so only the e4m3fnuz entry was affected. The new tests assert the exported ONNX data_type for each ml_dtypes float8 variant, plus the round trip, rather than relying on raw_data: since both e4m3 variants share a byte width, a bytes-only assertion cannot tell a correct label from an incorrect one. Reverting only the source change makes the three new regression tests fail, which confirms they detect the bug. Signed-off-by: luxing <luicarus@users.noreply.github.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.
Description
dtype_to_onnxincorrectly mappedml_dtypes.float8_e4m3fnuzto"FLOAT8E4M3FN", causing constants withfloat8_e4m3fnuzvalues to be exported with the wrong ONNX data type.Because
FLOAT8E4M3FNandFLOAT8E4M3FNUZhave the same storage width, this failed silently: the exportedraw_datawas unchanged, but the tensor was labeled with the wrong type.Fixes #4856.
What changed
onnx_graphsurgeon/exporters/onnx_exporter.pyfloat8_e4m3fnuzto"FLOAT8E4M3FNUZ".tests/test_exporters.pyCHANGELOG.mdFixedentry underv0.6.2.Tests
I verified that the new regression tests detect the original bug by reverting only the source fix and re-running them:
test_should_export_constant_tensor_with_each_ml_float8_dtype[float8_e4m3fnuz-18]test_should_distinguish_e4m3fn_from_e4m3fnuztest_should_preserve_e4m3fnuz_across_import_and_export_round_tripFull
tools/onnx-graphsurgeontest suite:The 12 remaining failures are unchanged and unrelated to this patch. They come from
tests/test_examples.py, which invokes examples through:In my Windows environment,
python3is not available under that command name. No test that passed before this change fails afterward.Notes
The regression tests assert the exported ONNX
data_typeexplicitly rather than relying only onraw_data. SinceFLOAT8E4M3FNandFLOAT8E4M3FNUZuse the same byte width, a bytes-only assertion would not detect this bug.Relationship to #4843
#4843 fixes the same defect and is technically correct. It has been open since 2026-09-01 with no linked issue and no maintainer response, and its DCO check fails because its commit carries no
Signed-off-byline, which I believe is the only thing blocking it.I am opening this separately rather than waiting because it adds two things #4843 does not have: a
CHANGELOG.mdentry (following the precedent of the merged #4836) and a regression test for the import/export round trip described in #4856, which #4843's PR description mentions but does not cover. Its existing test also relies on araw_dataround-trip assertion that cannot distinguish a correct label from an incorrect one.This commit is signed off. If the maintainers would rather land #4843 once it is signed off, that is entirely fine by me — please close this one.