Fix aten_amax and aten_amin export when dim is omitted - #3035
Fix aten_amax and aten_amin export when dim is omitted#3035om 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.
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, matches ONNX Reduce* semantics for omitted axes, and is covered by a targeted end-to-end regression test.
Pull request overview
This PR fixes ONNX export for torch.amax(x) / torch.amin(x) when dim is omitted by aligning the aten::amax / aten::amin lowering signatures with the ATen schema default (dim=[] meaning reduce over all dimensions), and ensuring the lowering can handle the “argument omitted entirely” form emitted by torch.export.
Changes:
- Make
aten_amax/aten_aminaccept an omitteddim(defaulting toNone) and lower toReduceMax/ReduceMinwithout providing anaxesinput in that case. - Add an end-to-end regression test covering
amax/aminwith and withoutkeepdimwhendimis omitted. - Add a graph-level check intended to guard against
noop_with_empty_axes=1on the emitted reduce nodes.
File summaries
| File | Description |
|---|---|
| tests/function_libs/torch_lib/e2e_ops_tests.py | Adds an E2E regression test for amax/amin export when dim is omitted, including a guard against noop_with_empty_axes being set incorrectly. |
| onnxscript/function_libs/torch_lib/ops/core.py | Updates aten::amax / aten::amin signatures and lowering to correctly handle omitted dim by reducing over all axes. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for node in onnx_program.model.graph: | ||
| if node.op_type in ("ReduceMax", "ReduceMin"): | ||
| self.assertEqual(node.attributes.get_int("noop_with_empty_axes", 0), 0) | ||
| _testing.assert_onnx_program(onnx_program) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3035 +/- ##
==========================================
- Coverage 72.70% 72.69% -0.01%
==========================================
Files 265 265
Lines 32298 32302 +4
Branches 3059 3061 +2
==========================================
+ Hits 23481 23483 +2
Misses 7779 7779
- Partials 1038 1040 +2 ☔ View full report in Codecov by Harness. |
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.