Fix aten_amax and aten_amin export when dim is omitted - #3035
Open
om singhal (Om-singhaI) wants to merge 1 commit into
Open
Fix aten_amax and aten_amin export when dim is omitted#3035om singhal (Om-singhaI) wants to merge 1 commit into
om singhal (Om-singhaI) wants to merge 1 commit into
Conversation
The aten schema is amax(Tensor self, int[1] dim=[], bool keepdim=False), so dim defaults to the empty list and means reduce every dimension. torchlib declared dim with no default, which made it required, and torch.export emits aten.amax.default(x) with no dim when the caller leaves it out. The dispatcher then raised ValueError: Required parameter 'dim' is not provided. torch.amax(x, keepdim=True) already worked, because torch.export has to materialize dim=[] positionally to reach keepdim, and the lowering handles an empty axes input correctly. Only the spelling that drops dim failed. aten_amax and aten_amin are now trace_only and default dim to None, which takes ReduceMax and ReduceMin without an axes input. noop_with_empty_axes keeps its default of 0, so that reduces every axis rather than acting as an identity.
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
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.
torch.amax(x)andtorch.amin(x)don't export. The aten schema isamax(Tensor self, int[1] dim=[], bool keepdim=False), sodimdefaults to the empty list, meaning reduce every dimension.aten_amaxandaten_amininonnxscript/function_libs/torch_lib/ops/core.pydeclareddimwith no default, which makes it required, andtorch.exportemitsaten.amax.default(x)with nodimat all when the caller leaves it out.The same reduction exports fine one line over
torch.amax(x, keepdim=True)works on main today.torch.exportcan't reachkeepdimwithout fillingdimin positionally, so it emitsaten.amax.default(x, [], True)and the lowering handles that empty axes input correctly. Same op, same emptydim, one spelling exports and the other raises. The only difference is whetherdimreached the graph at all.The comment already sitting in the source says
ReduceMax reduces all dimensions when dim is empty, so the intent was right. The signature just never let you get there.The fix
aten_amaxandaten_aminbecometrace_onlyand defaultdimtoNone. On that path they callReduceMaxandReduceMinwith no axes input.That last part is the bit worth checking. The ONNX spec for ReduceMax 18 says an empty axes, "either not provided or explicitly empty", reduces over all axes when
noop_with_empty_axesis false and over the empty set of axes when it's true. It defaults to 0, so leaving axes out reduces everything. A 1 there would hand back the input untouched, which is a wrong answer rather than an error, so the test asserts the attribute is 0 on the emitted node as well as comparing against eager.I ran both forms against onnxruntime, opset 18, on a
(2, 3)input whose max is 9.0:The graph for
torch.amax(x)with this change is one node:Tests
test_amax_amin_reduce_every_dimension_when_dim_is_omittedintests/function_libs/torch_lib/e2e_ops_tests.py, four cases: amax and amin, each with and withoutkeepdim. Thekeepdimpair passes on main and is there to pin that both spellings keep agreeing.pytest tests/function_libs/torch_lib/e2e_ops_tests.py -k amax_amin:pytest tests/function_libs/torch_lib/ops_test.py -k "amax or amin":The two extra skips are the function proto validity checks, which skip for traced functions. Same 328 subtests pass either way.
Whole file,
pytest tests/function_libs/torch_lib/e2e_ops_tests.py: 8 failed, 119 passed, 1 skipped, 104 subtests passed. The same 8 fail on main without my change (stft, deform_conv2d, sdpa bool mask, unbind dynamic, convolution complex kernel shape), so they're unrelated.ruff checkandruff format --checkpass on both files with ruff 0.15.1, the lintrunner pinned version.Environment: Python 3.10, torch 2.9.1, onnx 1.22.0, onnxruntime 1.23.2, macOS arm64.