[FIX][Relax][ONNX] Keep the static shape of a rank-0 Shape input - #20092
[FIX][Relax][ONNX] Keep the static shape of a rank-0 Shape input#20092adityasingh2400 wants to merge 3 commits into
Conversation
Shape._impl_v13 decided between the static shape and a runtime shape_of with a truthiness test on data_info.shape. A rank-0 tensor has a defined but empty shape, and an empty ShapeExpr is falsy, so a scalar input took the runtime path that is meant only for a tensor whose shape is genuinely unknown, where .shape is None. The returned value was then a normalized R.shape_of call rather than a ShapeExpr, so every downstream converter that matches on relax.ShapeExpr lost its static path. Slice is the visible case: importing Shape followed by Slice on a scalar input raised "Slice requires a statically known input rank", because _get_known_tensor_rank cannot give a rank for a shape value. Gather and Reshape carry the same ShapeExpr match. Compare against None so a rank-0 input keeps R.shape([]), which is what every other rank already gets. Shape of a scalar then folds at import time, and Slice over it folds to an empty int64 tensor, matching what ONNX Runtime returns for the same graph. test_shape_start_end_scalar pinned the old runtime fallback for a scalar with start=1. That case now folds to the same empty static shape, checked against ONNX Runtime, so the test asserts the folded module instead. Fixes apache#17770
tlopex
left a comment
There was a problem hiding this comment.
get_info still maps both an absent ONNX shape (unknown rank) and an empty shape (scalar) to []. This change would therefore fold unknown-rank values to R.shape([]). Please preserve the distinction using HasField("shape") and add an unknown-rank test.
get_info started from an empty list and only appended per dim, so a proto with no shape field at all produced the same [] as a rank-0 tensor whose shape field is present with zero dims. Both then became R.Tensor(()), and the is None check in Shape could never be reached for an unknown-rank input. Report None when the shape field is absent, which TensorType already documents as the unknown-rank form alongside ndim -1, and warn about it at the call site the way unknown dimensions are already warned about.
|
You are right, and thanks for catching it. Fixed in dd1477d. I confirmed the collapse you describe.
I checked the premise against real protos rather than assuming:
Added One caveat on verification: I could not run the ONNX frontend tests locally, since that needs a built TVM. The proto behaviour above is measured, the rest is reasoned from |
The new test read params[0].struct_info, which does not exist on this base. Relax vars carry a Type here, so the assertion raised AttributeError instead of checking anything. Read .ty, which exposes the same shape and ndim.
|
The failure was a single test out of the run: It reproduced identically on both test shards, and the Build stage itself passed, so this was the assertion and not the conversion. I wrote the assertion as data_ty = tvm_model["main"].params[0].ty
assert data_ty.shape is None
assert data_ty.ndim == -1That matches how existing tests read it, for example Being clear about what I verified: I do not have a TVM build in this environment, so I could not run the test locally, and I am relying on CI for that. What I did check is that The frontend change from dd1477d is untouched, and |
|
Following up on my last comment, CI has confirmed the fix and all five checks are green: I said there that I had no TVM build in my environment, could not run the test locally, and was relying on CI for that. The cpu run has now finished: against the previous run's @tlopex this is still marked as changes requested from your review, and I believe both points are now covered. dd1477d fixed the collapse you identified, where |
Shape._impl_v13in the Relax ONNX frontend chose between the static shape and a runtimeshape_ofwith a truthiness test:A rank-0 tensor has a defined but empty shape, and an empty
ShapeExpris falsy, so a scalar input took the runtime path that is meant only for a tensor whose shape is genuinely unknown.TensorType.shapeisNonein that case andR.shape([])for a rank-0 tensor, so the two are distinguishable, and only the first should reachshape_of.The value handed back was then a normalized
R.shape_ofcall rather than aShapeExpr, so every downstream converter that matches onrelax.ShapeExprlost its static path.Sliceis the visible case from the report: importingShapefollowed bySliceon a scalar input raisesbecause
_get_known_tensor_rankcannot produce a rank for a shape value.GatherandReshapecarry the sameisinstance(..., relax.ShapeExpr)match, so they are exposed to the same shape.Fix
Compare against
None, so a rank-0 input keeps the staticR.shape([])that every other rank already gets.Shapeof a scalar now folds at import time toR.shape([]), andShapefollowed bySlicefolds to an emptyint64tensor of shape(0,). Checked against ONNX Runtime 1.24 on the same graph:Shapeof a scalar returns an emptyint64array of shape(0,), and slicing it returns the same, so the folded result matches.Effect on an existing test
test_shape_start_end_scalar, added by #20050, pinned the runtime fallback for a rank-0 input withstart=1, asserting the op chainrelax.shape_of, relax.shape_to_tensor, relax.strided_slice, relax.tensor_to_shape. With the static shape preserved, that case folds to the same empty static shape, so the test now asserts the folded module and that no ops remain. ONNX Runtime returns an emptyint64array for that graph too, so the folded answer is the correct one, and the assertion change is the point of the fix rather than a workaround for it.Testing
Two new tests in
tests/python/relax/test_frontend_onnx.py:test_shape_scalar_input, structural equality against the expected module, pins thatShapeof a rank-0 input emitsR.shape([])and notR.shape_of.test_slice_of_scalar_shape, the reported pattern end to end, pins that the import succeeds and yields an emptyint64tensor.Verified fail-before and pass-after against the base ref rather than a stash, over the whole file so any collateral damage would show:
The failure sets differ by exactly three entries, all of them the target tests, and there are no new failures:
This was run on a local build configured with
USE_LLVM OFF, so the tests that go throughcheck_correctnessandtvm.compile(target="llvm")fail identically in both runs withValueError: Cannot find global function target.build.llvm. They are the same 174 entries on both sides and are unrelated to this change. The three tests above and the surroundingtest_shapeandtest_shape_start_endcases need no codegen and were run directly, 16 passed.Lint checked with the pinned
ruff==0.12.3from.pre-commit-config.yaml:ruff format --checkreports already formatted andruff checkpasses on both files.Fixes #17770