Add support for axes input to reduce operators #2120

music-dino · 2023-08-24T08:31:20Z

Add parsing support for axes as a non-constant input
Update the reference implementation to handle this case
Resolves Reduce operators axes input parse error migraphx-benchmark/AMDMIGraphX#68

…18_compat

pfultz2 · 2023-08-29T18:45:02Z

src/include/migraphx/op/reduce_op.hpp

@@ -80,11 +80,12 @@ template <class Derived>
 struct reduce_op : op_name<Derived>
 {
    std::vector<std::int64_t> axes{};
+    bool noop_with_empty_axes = false;


We want to make reduce_op to always be a noop with empty axes. For this PR, just skip the handling of empty axes for now. In a future PR, we can update the onnx parser to insert extra operators for empy axes that is not a noop.

Do you want the empty axes handling to be skipped, or changed even in the case when axes are constant or an attribute?
In the current code, that is handled according to ONNX spec.

This field should be removed. And we should by default be a noop wit empty axes and the parser can insert extra operators to handle the empty case for the ONNX spec.

In the constant axes case the parser already handles that, by not inserting an operator at all, and just passing the input back as the output.
In the variable axes case it cannot be known until runtime whether a noop will occur until the runtime value of axes is checked.
So which operators could the parser insert in the variable axes case to handle it. The If operator?

This field needs to be removed from the operator.

In the variable axes case it cannot be known until runtime whether a noop will occur until the runtime value of axes is checked. So which operators could the parser insert in the variable axes case to handle it. The If operator?

We can insert operators such as equal to detect this and then insert a where operator to pick between the reduce and an empty reduce(or a reduce with all axes for the noop_with_empty_axes=false case).

The Where would only have to be inserted when noop_with_empty_axes is true, and be predicated on the size of the variable axes input. For empty axes the input tensor would need to be returned, while for non-empty axes a reduce would be performed.
This leads to an issue with Where due to the difference of output dimensions of the two branches, given that one would have the shape of the input, and the other would have whichever shape the variable input axes dictate.

The Where would only have to be inserted when noop_with_empty_axes is true

reduce_op should be a noop with empty axes, by default. So then when noop_with_empty_axes=false the where would take two different reduces: is_empty(axes) ? reduce_op(x, computed_axes) : reduce_op(x, axes). For both branches the axes will be passed as a variable input(even though the empty case will be taking a literal) so they produce the same output shape.

Has this issue been resolved yet?

Hi @CharlieL7, not yet, I'd been on vacation for most of December so the PR was left unattended.

…18_compat

umangyadav · 2023-09-18T16:02:53Z

@music-dino can you fix tidy ? and resolve merge issues ?

…18_compat

codecov · 2023-09-26T15:43:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.78%. Comparing base (effedcd) to head (bbdc7ac).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2120      +/-   ##
===========================================
+ Coverage    91.75%   91.78%   +0.02%     
===========================================
  Files          473      473              
  Lines        17958    18011      +53     
===========================================
+ Hits         16478    16531      +53     
  Misses        1480     1480

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ests

…18_compat

CharlieL7

Not sure what's the status on this PR.

CharlieL7 · 2023-12-19T19:56:17Z

src/include/migraphx/op/reduce_op.hpp

+    argument compute(const shape& computed_shape,
+                     const std::vector<int64_t>& reduce_axes,
+                     argument& data_arg) const


Can we name this function something else so we're not getting into possible future trouble with how operation.hpp handles the compute function?

CharlieL7 · 2023-12-19T19:57:05Z

src/include/migraphx/op/reduce_op.hpp

@@ -80,11 +80,12 @@ template <class Derived>
 struct reduce_op : op_name<Derived>
 {
    std::vector<std::int64_t> axes{};
+    bool noop_with_empty_axes = false;


Has this issue been resolved yet?

…18_compat

…implementation

music-dino · 2024-01-29T16:32:24Z

src/onnx/parse_reduce_op.cpp

+            auto is_axes_empty_bc =
+                info.add_instruction(make_op("multibroadcast"), is_axes_empty, reduce_all_axes);
+
+            return info.add_instruction(


@pfultz2 I've implemented operator parsing in the fashion you recommended. The where here will work during parsing, due to dynamic output shape equivalence between branches. However, when evaluation time comes around, the output shapes will differ which causes where to throw an exception(demonstrated by this test).

music-dino · 2024-02-05T16:10:18Z

test/onnx/verify/reducesum_variable_dynamic_axes_test.cpp

+    std::vector<int64_t> axes{1};
+    pm["axes"] = argument(shape{shape::int64_type, {1}}, axes.data());
+
+    auto result = p.eval(pm);


@pfultz2 @CharlieL7

This test demonstrates the Where issue that crops up(here is the code path it tests) we discussed last week.

All three input shapes of Where being the same({1, 2}, {1, 2}, {1, 2}) parsing will succeed.
However, when attempting to evaluate the graph issues pop up.
The shapes being reified, the all axes reduction will have an output shape of {1, 1, 1}, the other one {2, 1, 2}, causing this check to fail.

The compute method to where needs to be updated to remove the dyn_output usage so the compute shape can happen lazily. For scalar condition we can pick either branch regardless of the shape difference:

argument compute(shape output_shape, std::vector<argument> args) const { if(args[0].scalar()) return args[args[0].at<bool>() ? 1 : 2].copy(); if(output_shape.dynamic()) output_shape = compute_shape(to_shapes(args)); argument result{output_shape}; visit_all(result, args[1], args[2])([&](auto output, const auto x, const auto y) { args[0].visit([&](const auto condition) { par_for(dyn_out.computed_shape.elements(), [&](auto i) { output[i] = condition[i] ? x[i] : y[i]; }); }); }); return result; }

…18_compat

lakhinderwalia

Thanks for all your commits/responses so far. I have submitted some additional comments. PTAL.

src/include/migraphx/op/reduce_op.hpp

src/onnx/parse_reduce_op.cpp

test/ref/reduce_mean.cpp

lakhinderwalia · 2024-02-23T04:34:11Z

test/ref/reduce_mean.cpp

+
+    std::vector<int32_t> gold{2, 6, 10};
+    EXPECT(results_vector == gold);
+}


If there is not a test for default axes, with keepdims=1, the following (copied from Netron) would be a good test. Thanks.

shape = [3, 2, 2] axes = np.array([], dtype=np.int64) keepdims = 1 node = onnx.helper.make_node( "ReduceMean", inputs=["data", "axes"], outputs=["reduced"], keepdims=keepdims, ) data = np.array( [[[5, 1], [20, 2]], [[30, 1], [40, 2]], [[55, 1], [60, 2]]], dtype=np.float32, ) reduced = np.mean(data, axis=None, keepdims=keepdims == 1) # print(reduced) # [[[18.25]]] expect( node, inputs=[data, axes], outputs=[reduced], name="test_reduce_mean_default_axes_keepdims_example", )

Please note, the dimension of the output tensor has to be verified as well as the value.

…18_compat

src/onnx/parse_reduce_op.cpp

lakhinderwalia · 2024-02-27T20:19:05Z

src/onnx/parse_reduce_op.cpp

+        // Empty axes attribute indicates to the operator to look for axes in the inputs
+        auto reduce_op = make_op(op_name, {{"axes", {}}});
+
+        if(noop_with_empty_axes != 0)


This clause also needs to check that the axes is indeed empty.

I'm sorry but I still fail to see why it should. Can you demonstrate with a particular situation for which it would be necessary, and how you see the code path for that situation, including the compute method of the operator?

We cant check that the axes is empty because we dont know how many axes there are yet until runtime.

(Had a discussion with Paul. The reason this logic works is because, our reduce_op has some default behavior when the axis is empty: to be an identity operator. Hence it is taken care of somewhere else. This code doesn't explain that.)
So please add a comment here to give a hint:
'If axes is empty, the default behavior of reduce_op is to be an identity operator"

I added that default behavior, and that's what I was trying to explain. I've added the suggested comment. Thanks for reviewing.

… variable dynamic axes

…erify test cases

…18_compat

pfultz2 · 2024-03-04T17:45:04Z

src/include/migraphx/op/reduce_op.hpp

+    // Depending on how axes is passed to the operator the output shape can be either dynamic or
+    // static.
+    //
+    // If the axes are passed as an attribute, or a constant input, we can precisely


We cant produce a static shape with a constant input because the input to compute_shape is just the shapes.

pfultz2 · 2024-03-04T17:45:39Z

src/include/migraphx/op/reduce_op.hpp

+    // static.
+    //
+    // If the axes are passed as an attribute, or a constant input, we can precisely
+    // determine which axes need to be collapsed even during parsing.


Remove even during parsing. There is no parsing here. The onnx parser has already constructed the operator.

Removed. I had intended to state something different, but I worded it poorly.

lakhinderwalia

Please update with comments, as I have explained above.. Approving this PR.

…18_compat

music-dino added 6 commits August 18, 2023 07:21

Implement parsing support for onnx opset 18 versions of reduce operators

e7243d9

Implement codepath for handling a static data shape for reduce operators

714b10e

Implement dynamic shape computation and refactor existing code

ae024f2

Merge remote-tracking branch 'upstream/develop' into reduce_op_opset_…

44be9e5

…18_compat

Add reference operator tests for ReduceMin, ReduceMax, and ReduceMean

904de2a

Merge remote-tracking branch 'upstream/develop' into reduce_op_opset_…

a0f8e52

…18_compat

causten requested review from pfultz2 and umangyadav August 24, 2023 14:14

CharlieL7 self-requested a review August 24, 2023 14:30

attila-dusnoki-htec mentioned this pull request Aug 24, 2023

Bump onnx from 1.10.2 to 1.14.0 #2121

Closed

pfultz2 reviewed Aug 29, 2023

View reviewed changes

music-dino added 4 commits August 31, 2023 09:23

Disable failing onnx backend tests

4316e48

Remove noop_with_empty_axes_attribute, fix code style issues

b024436

Restore ref_ops_test.cpp

5b2f4c3

Merge remote-tracking branch 'upstream/develop' into reduce_op_opset_…

8b91ef0

…18_compat

music-dino added 5 commits September 24, 2023 09:20

Restore noop_with_empty_axes attribute

4b200ca

Merge remote-tracking branch 'upstream/develop' into reduce_op_opset_…

844e358

…18_compat

Add op shape tests for variable axes case

075f663

Implement onnx parsing tests for variable axes

2919f1d

Add reference operator tests

2fbd708

music-dino marked this pull request as ready for review September 26, 2023 17:03

causten and others added 2 commits October 1, 2023 15:32

Merge branch 'develop' into reduce_op_opset_18_compat

5762d6c

Update verify_range uses to verify_rms_range in reduce op reference t…

af06175

…ests

music-dino requested a review from pfultz2 November 1, 2023 12:37

Merge remote-tracking branch 'upstream/develop' into reduce_op_opset_…

7d8bede

…18_compat

CharlieL7 reviewed Dec 19, 2023

View reviewed changes

music-dino added 2 commits January 24, 2024 08:48

Merge remote-tracking branch 'upstream/develop' into reduce_op_opset_…

53877e8

…18_compat

Rejig reduce operator parsing to remove complexity from the operator …

0cd9b14

…implementation

music-dino added 3 commits January 29, 2024 08:16

Implement parsing tests for ReduceL1

0bcca87

Implement parse tests for ReduceSum

71d00e6

Delete ReduceL1 onnx test files

2e324f8

music-dino commented Jan 29, 2024

View reviewed changes

Update test name

05338d4

music-dino commented Feb 5, 2024

View reviewed changes

music-dino added 4 commits February 7, 2024 11:23

Update Where operator to handle scalar predicate

399cde5

Merge remote-tracking branch 'upstream/develop' into reduce_op_opset_…

c6122d8

…18_compat

Implement additional tests, fix parsing and licensing

95fa52e

Fix formatting, cppcheck, and tidy issues

5f7b702

music-dino requested a review from CharlieL7 February 7, 2024 13:58

attila-dusnoki-htec requested a review from lakhinderwalia February 22, 2024 07:49

lakhinderwalia reviewed Feb 23, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into reduce_op_opset_…

2784e31

…18_compat

lakhinderwalia reviewed Feb 27, 2024

View reviewed changes

src/onnx/parse_reduce_op.cpp Show resolved Hide resolved

lakhinderwalia requested changes Feb 27, 2024

View reviewed changes

Remove superfluous onnx test files, and add additional test cases for…

2e6e5e8

… variable dynamic axes

music-dino requested a review from causten as a code owner February 28, 2024 10:01

music-dino added 4 commits February 28, 2024 10:47

Add some documentation for reduce_op, and implement additional onnx_v…

c350c5f

…erify test cases

Merge remote-tracking branch 'upstream/develop' into reduce_op_opset_…

9efb952

…18_compat

Fix formatting issue in gen_onnx.py

13364ec

Fixed formatting issue

d92a664

pfultz2 approved these changes Mar 4, 2024

View reviewed changes

lakhinderwalia approved these changes Mar 4, 2024

View reviewed changes

music-dino added 2 commits March 5, 2024 14:25

Update code comments

766ecfe

Merge remote-tracking branch 'upstream/develop' into reduce_op_opset_…

bbdc7ac

…18_compat

causten merged commit 11f6abc into ROCm:develop Mar 6, 2024
17 of 18 checks passed

causten deleted the reduce_op_opset_18_compat branch March 6, 2024 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for axes input to reduce operators #2120

Add support for axes input to reduce operators #2120

music-dino commented Aug 24, 2023

pfultz2 Aug 29, 2023

music-dino Aug 31, 2023

pfultz2 Oct 6, 2023

music-dino Oct 9, 2023

pfultz2 Nov 29, 2023 •

edited

Loading

music-dino Dec 5, 2023

pfultz2 Dec 6, 2023

CharlieL7 Dec 19, 2023

music-dino Jan 3, 2024

umangyadav commented Sep 18, 2023

codecov bot commented Sep 26, 2023 •

edited

Loading

CharlieL7 left a comment

CharlieL7 Dec 19, 2023

music-dino Jan 3, 2024

CharlieL7 Dec 19, 2023

music-dino Jan 29, 2024

music-dino Feb 5, 2024

pfultz2 Feb 5, 2024

lakhinderwalia left a comment

lakhinderwalia Feb 23, 2024

lakhinderwalia Feb 23, 2024

lakhinderwalia Feb 27, 2024

music-dino Feb 27, 2024 •

edited

Loading

pfultz2 Mar 4, 2024

lakhinderwalia Mar 4, 2024

music-dino Mar 5, 2024

pfultz2 Mar 4, 2024

pfultz2 Mar 4, 2024

music-dino Mar 5, 2024

lakhinderwalia left a comment

Add support for axes input to reduce operators #2120

Add support for axes input to reduce operators #2120

Conversation

music-dino commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfultz2 Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umangyadav commented Sep 18, 2023

codecov bot commented Sep 26, 2023 • edited Loading

Codecov Report

CharlieL7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakhinderwalia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

music-dino Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakhinderwalia left a comment

Choose a reason for hiding this comment

pfultz2 Nov 29, 2023 •

edited

Loading

codecov bot commented Sep 26, 2023 •

edited

Loading

music-dino Feb 27, 2024 •

edited

Loading