fix: unify LINSPACE alpha/beta dtype in the arange converter - #4626
fix: unify LINSPACE alpha/beta dtype in the arange converter#4626SrivastavaKshitij wants to merge 1 commit into
Conversation
github-actions
Bot
added
component: tests
`arange` resolves a common `value_dtype` for the sequence and passes it to each
`get_trt_tensor` call. `get_trt_tensor` applies that dtype when it constructs a constant,
and returns a value that is already an ITensor unchanged:
elif isinstance(input_val, TRTTensor):
return input_val
So a dynamic `start` keeps the dtype of its incoming ITensor, while a literal `step`
becomes a constant of `value_dtype`. TensorRT asks that the LINSPACE `alpha` (input 1) and
`beta` (input 2) have the same type, and reports:
IFillLayer `alpha` and `beta` must have the same type.
`alpha` is of type Int32 but `beta` is of type Int64.
This change routes every operand through a small helper that casts after
`get_trt_tensor`, so the resolved dtype holds for all of them. `cast_trt_tensor` returns
the tensor unchanged when the dtype already matches, so paths that were already consistent
are unaffected.
The tests cover three cases with a dynamic bound: an int32 `start`, an int64 `start`, and
both bounds dynamic. Of those, only the int64 `start` works today, because the sequence
dtype for integer operands is already int64 and matches. The other two pass with this
change, and the full arange converter suite stays green.
e3eeb41 to
91b21b9
Compare
|
@narendasan @apbose for viz |
| Three cases with a dynamic bound are covered here: an int32 `start`, an int64 | ||
| `start`, and both bounds dynamic. Of those, only the int64 `start` works today, | ||
| because the sequence dtype for integer operands is already int64 and matches. | ||
| """ |
There was a problem hiding this comment.
Minor comment: this looks to be the pre-fix behavior. _operand_as() now force-casts start_rank_0/start_rank_1 to value_dtype via cast_trt_tensor regardless of the incoming ITensor's own dtype, so start no longer "keeps the dtype of its incoming ITensor." And the "only int64 works today" claim is directly contradicted by the test right below it: test_arange_dynamic_start[int32] passes along with [int64]. Could you please update the docstring
| @@ -41,6 +41,28 @@ def _sequence_dtype( | |||
| return trt.DataType.INT64 | |||
|
|
|||
There was a problem hiding this comment.
this is unrelated to your changes. _sequence_dtype()(same file, lines 29-41) has a bug that affects this PR's own scenario, the returntrt.DataType.INT64fallback is inside the for loop, so it only ever looks at the first operand and ignores the rest. For arange(dynamic_int_start, 10.0, 1.5), it returns INT64 (wrong) instead of FLOAT, because it stops checking after start and never sees that end/step are floats. Since your _operand_as() now enforces whatever dtype this function returns on every operand, this silently produces wrong integer output instead of the correct float sequence.
You could include the fix in this PR, with a minimal test scenario
2 participants
Description
Issue #4625 has two parts.
arangeresolves a commonvalue_dtypefor the sequence and passes it to eachget_trt_tensorcall.get_trt_tensorapplies that dtype when it constructs a constant, andreturns a value that is already an ITensor unchanged:
So a dynamic
startkeeps the dtype of its incoming ITensor, while a literalstepbecomes aconstant of
value_dtype. TensorRT asks that the LINSPACEalpha(input 1) andbeta(input 2) have the same type, and reports:
This routes every operand through a small helper that casts after
get_trt_tensor, so theresolved dtype holds for all of them.
cast_trt_tensorreturns the tensor unchanged when thedtype already matches, so paths that were already consistent are unaffected.
Issues
Closes #4625
Type of change
Testing
Three cases with a dynamic bound are covered: an int32
start, an int64start, and bothbounds dynamic. Of those, only the int64
startworks today, because the sequence dtype forinteger operands is already int64 and matches.
Checklist: