fix(ONNX): avoids resizing unsupported dimensions #3945

bjacobgordon · 2025-01-07T22:21:34Z

Based on #3975

zjgarvey

I think the main structural question is about the need for adding the BaseTensorType method. If it were useful elsewhere (I have some doubts, since we would need to know too much about the two tensor shapes prior to using it- namely that they are present, and they have the same rank), I would consider keeping it; however, the code is simplified here by not using it, and I suspect that the same would be true in other circumstances where it might be used.

lib/Dialect/Torch/IR/TorchTypes.cpp

include/torch-mlir/Dialect/Torch/IR/TorchTypes.h

lib/Conversion/TorchOnnxToTorch/DefaultDomainQtoZ.cpp

zjgarvey

Sorry for the misdirect earlier, we need to perform the runtime asserts on the scales or sizes values instead of sizes, since we will not have access to the correct output sizes ahead of time.

lib/Conversion/TorchOnnxToTorch/DefaultDomainQtoZ.cpp

IanWood1 · 2025-01-14T17:07:07Z

I think the renaming of loc -> opLocation should probably be split into a different PR so it can be reviewed separately, it adds ~600 lines of changes that aren't directly related to the functional changes. It would help to keep this PR focused and easier to review.

lib/Conversion/TorchOnnxToTorch/DefaultDomainQtoZ.cpp

bjacobgordon · 2025-01-17T21:59:05Z

Okay, @zjgarvey, I think we're in business! Got green on the CI a few hours ago. Just wrapped up self-review.

I'm guessing now we'll:

discuss how the diffs might need to change
finalize those changes
discuss which chunks of commits need to be in the PR stack
Something like that?

I kept the commits atomic and ordered such that it's easier to propagate changes to the head of the branch in case an earlier commit needs to be inserted/tweaked/excised.

Let me know what you think!

zjgarvey · 2025-01-17T23:14:54Z

Okay, @zjgarvey, I think we're in business! Got green on the CI a few hours ago. Just wrapped up self-review.

I'm guessing now we'll:
* discuss how the diffs might need to change

* finalize those changes

* discuss which chunks of commits need to be in the PR stack
  Something like that?
I kept the commits atomic and ordered such that it's easier to propagate changes to the head of the branch in case an earlier commit needs to be inserted/tweaked/excised.

Let me know what you think!

Nice!

At least for now, please exclude any commits which involve style changes not directly related to the fix content. E.g. widespread enforcement of naming preferences like the loc -> opLocation should be factored out into a separate PR as @IanWood1 suggested.

… transforms filter

…elper

…n transforms filter

…ilter

…ms filter

…nsforms filter

…lper

…n `getValueList`

- Before: - "get": implies retrieval of some private property - "Value": restatement of the return type `Value` - "List": assumed result of casting the returned instance - After: - "create": contextualizes the need to pass in `rewriter` - "Scalar": contextualizes the opaque return type - "Sublist": the relationship between the first parameter and the returned result

…s filter

…forms filter

- helper isn't concerned with the role of the tensor at the call site, only its shape

- easier to read - allows for cleaner diffs if they ever change

bjacobgordon force-pushed the fix-onnx-adds-exceptions-enforcing-convention-in-resize-op branch 3 times, most recently from 6baa8d5 to ab7e021 Compare January 8, 2025 23:02

bjacobgordon changed the title ~~fix(ONNX): protects against mismatched dynamic meta dimensions~~ fix(ONNX): avoids resizing fixed dimensions Jan 9, 2025

bjacobgordon mentioned this pull request Jan 9, 2025

convert-torch-onnx-to-torch generates invalid IR for onnx.Resize where scaling is in the first two dimensions #3453

Open

bjacobgordon force-pushed the fix-onnx-adds-exceptions-enforcing-convention-in-resize-op branch from ab7e021 to 7aec80b Compare January 9, 2025 17:17

zjgarvey reviewed Jan 9, 2025

View reviewed changes

bjacobgordon force-pushed the fix-onnx-adds-exceptions-enforcing-convention-in-resize-op branch from 7aec80b to a20ee29 Compare January 10, 2025 23:03

bjacobgordon changed the title ~~fix(ONNX): avoids resizing fixed dimensions~~ fix(ONNX): avoids non-scalable dimensions in onnx.resize Jan 13, 2025

bjacobgordon force-pushed the fix-onnx-adds-exceptions-enforcing-convention-in-resize-op branch from a20ee29 to 574f4fe Compare January 13, 2025 15:22

bjacobgordon marked this pull request as ready for review January 13, 2025 17:06

bjacobgordon requested a review from zjgarvey January 13, 2025 17:06

zjgarvey requested changes Jan 13, 2025

View reviewed changes

bjacobgordon force-pushed the fix-onnx-adds-exceptions-enforcing-convention-in-resize-op branch from 574f4fe to cb371ea Compare January 14, 2025 15:04

bjacobgordon marked this pull request as draft January 15, 2025 14:49

bjacobgordon force-pushed the fix-onnx-adds-exceptions-enforcing-convention-in-resize-op branch 3 times, most recently from 05ea165 to cb20894 Compare January 15, 2025 23:19

zjgarvey reviewed Jan 16, 2025

View reviewed changes

lib/Conversion/TorchOnnxToTorch/DefaultDomainQtoZ.cpp Outdated Show resolved Hide resolved

bjacobgordon force-pushed the fix-onnx-adds-exceptions-enforcing-convention-in-resize-op branch 4 times, most recently from 8a78147 to 54cf76e Compare January 17, 2025 21:50

bjacobgordon requested a review from zjgarvey January 17, 2025 21:59

bjacobgordon marked this pull request as ready for review January 17, 2025 21:59

bjacobgordon changed the title ~~fix(ONNX): avoids non-scalable dimensions in onnx.resize~~ fix(ONNX): avoids resizing unsupported dimensions Jan 17, 2025

bjacobgordon force-pushed the fix-onnx-adds-exceptions-enforcing-convention-in-resize-op branch from 54cf76e to 052404a Compare January 20, 2025 14:51

bjacobgordon added 28 commits January 24, 2025 17:11

refactor(ONNX): enforces declaration-usage adjacency for context in…

710cd11

… transforms filter

refactor(ONNX): gets context via location vs op in getValueList h…

3ad853e

…elper

refactor(ONNX): enforces declaration-usage adjacency for itemList i…

edbb35c

…n transforms filter

refactor(ONNX): extracts FilteredTransformation within transforms f…

ce66201

…ilter

refactor(ONNX): consolidates duplicate conditional fragments

f9bd42c

refactor(ONNX): prefers direct return over declaration and assignment

b9853be

refactor(ONNX): adds type parameter to transforms filter

20003e7

refactor(ONNX): extracts createScalarAs helper method from transfor…

0858e53

…ms filter

refactor(ONNX): removes redundant xTy declaration in transforms filter

8e8a809

refactor(ONNX): leverages operandType when declaring sizes in tra…

2de97b5

…nsforms filter

refactor(ONNX): removes redundant auto annotation in transforms filter

4cb5128

refactor(ONNX): extracts loc within transforms filter

c9a22d1

refactor(ONNX): adds anchoring labels to parameters getValueList he…

7a068ef

…lper

refactor(ONNX): narrows first parameter from Binder to Location i…

e6a6734

…n `getValueList`

refactor(ONNX): reorders params in getValueList helper method

5a932a7

refactor(ONNX): exposes start index as parameter getValueList helper

ed51947

refactor(ONNX): extracts listLength within transforms filter

7c37637

refactor(ONNX): renames sizes to sizesOfGiven1DTensor

e569b44

refactor(ONNX): renames operandType to Given1DTensor in transform…

d9fa3b9

…s filter

refactor(ONNX): renames itemList to runningScalarSublist in trans…

596e906

…forms filter

refactor(ONNX): renames item to eachScalar in transforms filter

f07b9cb

refactor(ONNX): renames i to indexOfEachScalar in transforms filter

218c897

refactor(ONNX): renames operand to given1DTensor in sublist helper

617980a

- helper isn't concerned with the role of the tensor at the call site, only its shape

refactor(ONNX): prefers multiline attributes in onnx.resize tests

7203364

- easier to read - allows for cleaner diffs if they ever change

refactor(ONNX): distills checks in lit tests for onnx.resize

062339d

fix(ONNX): differentiates names of lit tests for onnx.resize

0dc88e0

fix(ONNX): avoids resizing unsupported dimensions

73fa3b2

bjacobgordon force-pushed the fix-onnx-adds-exceptions-enforcing-convention-in-resize-op branch from a3daf2a to 73fa3b2 Compare January 24, 2025 18:17

bjacobgordon marked this pull request as draft January 24, 2025 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ONNX): avoids resizing unsupported dimensions #3945

fix(ONNX): avoids resizing unsupported dimensions #3945

bjacobgordon commented Jan 7, 2025 •

edited

Loading

zjgarvey left a comment

zjgarvey left a comment

IanWood1 commented Jan 14, 2025 •

edited

Loading

bjacobgordon commented Jan 17, 2025 •

edited

Loading

zjgarvey commented Jan 17, 2025

fix(ONNX): avoids resizing unsupported dimensions #3945

Are you sure you want to change the base?

fix(ONNX): avoids resizing unsupported dimensions #3945

Conversation

bjacobgordon commented Jan 7, 2025 • edited Loading

zjgarvey left a comment

Choose a reason for hiding this comment

zjgarvey left a comment

Choose a reason for hiding this comment

IanWood1 commented Jan 14, 2025 • edited Loading

bjacobgordon commented Jan 17, 2025 • edited Loading

zjgarvey commented Jan 17, 2025

bjacobgordon commented Jan 7, 2025 •

edited

Loading

IanWood1 commented Jan 14, 2025 •

edited

Loading

bjacobgordon commented Jan 17, 2025 •

edited

Loading