Fix SequenceEmpty dtype parameter ignored in eager mode - #2949
Fix SequenceEmpty dtype parameter ignored in eager mode#2949pratik wayase (PratikWayase) wants to merge 2 commits into
Conversation
| from typing import Any, Iterable | ||
|
|
||
|
|
||
| class _TypedSequence(list): |
There was a problem hiding this comment.
Pull request overview
This PR fixes eager/trace-mode handling of SequenceEmpty(dtype=...) by introducing a typed runtime sequence representation so empty sequences retain an ONNX element dtype and can safely flow into later sequence ops (e.g., SequenceInsert).
Changes:
- Added
_TypedSequence(alistsubclass carrying anonnx_dtype) as an internal runtime representation for sequences with explicit element dtype. - Updated eager-mode evaluation to wrap
SequenceEmptyoutputs in_TypedSequence, and to preserve_TypedSequencewhen converting values for runtime execution and type inference. - Added an eager-mode regression test covering
FLOAT16,DOUBLE, andINT64dtypeforSequenceEmpty+SequenceInsert.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/eager_mode_test.py |
Adds regression coverage ensuring SequenceEmpty(dtype=...) prevents SequenceInsert elem-type mismatches in eager mode. |
onnxscript/_internal/utils.py |
Updates type inference to extract ONNX element dtype from _TypedSequence instead of defaulting empty lists to float. |
onnxscript/_internal/typed_sequence.py |
Introduces _TypedSequence wrapper that carries onnx_dtype for empty (and subsequent) sequence values. |
onnxscript/_internal/evaluator.py |
Wraps SequenceEmpty outputs in _TypedSequence and preserves _TypedSequence during conversion for runtime execution. |
| if op.name == "SequenceEmpty": | ||
| dtype = attributes.get("dtype") or onnx.TensorProto.FLOAT | ||
| return _TypedSequence(dtype, result) |
| # Handle SequenceEmpty: wrap result with _TypedSequence to preserve dtype | ||
| # for empty sequences where type cannot be inferred from elements. | ||
| if op.name == "SequenceEmpty": | ||
| dtype = attributes.get("dtype") or onnx.TensorProto.FLOAT | ||
| return _TypedSequence(dtype, result) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2949 +/- ##
==========================================
+ Coverage 72.61% 72.63% +0.01%
==========================================
Files 263 264 +1
Lines 32034 32057 +23
Branches 3013 3016 +3
==========================================
+ Hits 23263 23283 +20
- Misses 7748 7750 +2
- Partials 1023 1024 +1 ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
onnxscript/_internal/evaluator.py:270
dtype = attributes.get("dtype") or onnx.TensorProto.FLOATwill treatdtype=0(TensorProto.UNDEFINED) as “missing” and silently default to FLOAT. This changes semantics vs ONNX/ORT (which would treat 0 as invalid) and can hide user errors. Prefer an explicitis Nonecheck and coerce tointwhen storing on the wrapper.
if op.name == "SequenceEmpty":
dtype = attributes.get("dtype") or onnx.TensorProto.FLOAT
return _TypedSequence(dtype, result)
| # Handle _TypedSequence BEFORE generic list check | ||
| if isinstance(val, _TypedSequence): | ||
| return onnx.helper.make_sequence_type_proto( # noqa: TID251 | ||
| onnx.helper.make_tensor_type_proto(val.onnx_dtype, None) # noqa: TID251 | ||
| ) |
Summary
Fix
SequenceEmptydtype parameter being ignored in eager/trace mode by introducing a typed runtime representation for sequences. Fixes #1562.Changes
As noted by G. Ramalingam (@gramalingam) in the issue, the root cause is that we reuse Python lists for runtime sequence values, and when the list is empty, we cannot infer a suitable ONNX type. This PR implements the first proposed fix: using a runtime representation that combines a type with the value.
_TypedSequence, alistsubclass inonnxscript/_internal/typed_sequence.pythat preserves ONNX dtype information for empty sequencesBaseEvaluator.eval_opinonnxscript/_internal/evaluator.pyto wrapSequenceEmptyresults with_TypedSequenceusing the provideddtypeattribute (defaults toFLOATifNone)_onnxscript_to_numpy_valueinonnxscript/_internal/evaluator.pyto preserve the_TypedSequencewrapper (instead of downcasting to a plainlist) when preparing inputs for the ONNX runtimevalue_to_type_protoinonnxscript/_internal/utils.pyto extract the dtype from_TypedSequencewhen inferring the ONNX TypeProtoTests
Added OpInfo-style regression test
test_sequence_empty_preserves_dtypecovering:FLOAT16dtypeDOUBLEdtypeINT64dtypeAdditionally verified edge cases:
dtypeattribute (correctly defaults toFLOAT)SequenceInsertoperations on the same typed sequenceSequenceAton a typed sequenceFiles:
onnxscript/_internal/typed_sequence.py(new)onnxscript/_internal/evaluator.pyonnxscript/_internal/utils.pytests/eager_mode_test.pyValidation
python -m pytest tests/eager_mode_test.py -k "test_sequence_empty_preserves_dtype" -vPassed: