Op Builder Tests 1. #4242

gchinora · 2025-08-19T11:10:01Z

During the code review of the op-builder-PRs the demand of creating tests for the op-builders came up.
This is the first PR that covers those op-builders that has already been merged in #4005

pfultz2 · 2025-08-20T03:48:11Z

test/op/builder/einsum_test.cpp

+                              "batch axes that are not the outer-most axes, is not implemented",
+                              {3, 3, 3},
+                              {}));
+}


This is missing tests for the valid inputs, are you going to add tests for those?

I've just added a few positive test cases. Pls. let me know if you'd like to see some specific special case that needs to be tested.

pfultz2 · 2025-08-20T03:48:58Z

test/op/builder/main.cpp

+ */
+#include "test.hpp"
+
+int main(int argc, const char* argv[]) { test::run(argc, argv); }


I think this file can be removed as it doesnt have any tests.

The purpose of this file is equivalent with the main.cpp inside "./test/onnx/parse/" or "./test/onnx/verify". This serves merely for the "test_op_builder_test" binary to have an entry point that launches the test cases.

I see the other tests dont have a main function.

gchinora · 2025-08-22T09:14:47Z

test/op/include/op_builder_test_utils.hpp

+                                       const migraphx::value& options,
+                                       const std::vector<migraphx::instruction_ref>& params)
+{
+    migraphx::module mm_op_built;


Paul! I recall that you've mentioned something about this implementation in the weekly meeting. Could you pls. describe here how would you like to see this to be done?

…in the code layout between my vscode and the pipeline

Copilot

Pull Request Overview

This PR adds comprehensive test coverage for op-builders in AMDMIGraphX, implementing tests for operators that were merged in PR #4005. The tests validate both successful operation building and error handling for invalid inputs.

Introduces a utility module for testing op-builders with standardized test patterns
Implements comprehensive test suites for GEMM, GELU, einsum, CELU, and other op-builders
Adds CMake build configuration to compile and run the op-builder tests

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
test/op/include/op_builder_test_utils.hpp	Utility header providing helper functions for testing op-builders
test/op/builder/gemm_test.cpp	Test cases for GEMM op-builder with various parameter configurations
test/op/builder/gelu_test.cpp	Test cases for GELU variants (quick, erf, tanh, split)
test/op/builder/einsum_test.cpp	Extensive test suite for einsum operations covering matrix operations and error cases
test/op/builder/celu_test.cpp	Test cases for CELU op-builder including error handling
test/op/builder/batchnorm_test.cpp	Test cases for batch normalization op-builder
test/op/builder/mean_variance_normalization_test.cpp	Test cases for mean variance normalization
test/op/builder/main.cpp	Test runner entry point
test/op/CMakeLists.txt	Build configuration for op-builder tests
test/CMakeLists.txt	Updated to include op-builder test directory

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-03T13:07:40Z

test/op/include/op_builder_test_utils.hpp

+    const std::vector<migraphx::instruction_ref>& args{params.rbegin(), params.rend()};
+    mm_op_built.add_instructions(args);
+
+    const auto& params2 = mm_op_built.get_parameters();
+    const std::vector<migraphx::instruction_ref>& args2{params2.rbegin(), params2.rend()};
+
+    migraphx::op::builder::add(op_builder_name, mm_op_built, args2, options);


[nitpick] The variable name args is confusing since it represents a reversed copy of params. Consider renaming to reversed_params or adding a comment explaining why the reversal is necessary.

Suggested change

const std::vector<migraphx::instruction_ref>& args{params.rbegin(), params.rend()};

mm_op_built.add_instructions(args);

const auto& params2 = mm_op_built.get_parameters();

const std::vector<migraphx::instruction_ref>& args2{params2.rbegin(), params2.rend()};

migraphx::op::builder::add(op_builder_name, mm_op_built, args2, options);

const std::vector<migraphx::instruction_ref> reversed_params{params.rbegin(), params.rend()};

mm_op_built.add_instructions(reversed_params);

const auto& params2 = mm_op_built.get_parameters();

const std::vector<migraphx::instruction_ref> reversed_params2{params2.rbegin(), params2.rend()};

migraphx::op::builder::add(op_builder_name, mm_op_built, reversed_params2, options);

Copilot · 2025-09-03T13:07:40Z

test/op/builder/mean_variance_normalization_test.cpp

+{
+    migraphx::module mm;
+
+    const auto axes = {2, 2, 2};


The axes vector contains duplicate values (2, 2, 2) which doesn't match the test comment indicating 'axes attribute needs to be equal to input tensor rank - 1'. For a 4D tensor {2, 2, 2, 2}, this should likely be {1, 2, 3} or similar non-duplicate axis indices.

Suggested change

const auto axes = {2, 2, 2};

const auto axes = {1, 2, 3};

Copilot · 2025-09-03T13:07:41Z

test/op/builder/gelu_test.cpp

+     auto split_left            = mm.add_instruction(
+         migraphx::make_op("slice",
+                                     {{"axes", {-1}}, {"starts", {0}}, {"ends", {last_dim_size / 2}}}),
+         x);


[nitpick] The indentation of the migraphx::make_op call is inconsistent. The parameters should be aligned properly within the function call.

Suggested change

auto split_left = mm.add_instruction(

migraphx::make_op("slice",

{{"axes", {-1}}, {"starts", {0}}, {"ends", {last_dim_size / 2}}}),

x);

auto split_left = mm.add_instruction(

migraphx::make_op(

"slice", {{"axes", {-1}}, {"starts", {0}}, {"ends", {last_dim_size / 2}}}),

x);

codecov · 2025-09-10T17:37:51Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop    #4242    +/-   ##
=========================================
  Coverage    92.23%   92.23%            
=========================================
  Files          555      557     +2     
  Lines        25682    25924   +242     
=========================================
+ Hits         23686    23910   +224     
- Misses        1996     2014    +18

see 27 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gchinora added 25 commits July 29, 2025 12:42

adding unit test basic files for op builders

80bcc2b

first test for batchnorm

d2858f3

2. test for batchnorm

5734d5e

remove unnecessary .at(.) call

edcfc8c

test for celu

4ceac20

add a comment

6319bc5

rename one file

7246287

new batchnorm tests

2dddb15

new celu tests

9cee884

modify failing tests to check error messages

c013fcf

introducing a helper builder to reduce test sizes

79c6dc4

modify to inline in order to avoid linker errors

62c05b3

making use of the new helper builder function to reduce test size

6a29d2d

document formatting

54823ed

adding negative unit tests for einsum op-builder

abf4084

remove unnecessary include

9afad70

first tests for gelu op-builders

cee08f7

extending gelu tests

e5f14bb

introducing the first two gemm tests

085decb

remove unnecessray include

d624e6e

the rest of the gemm tests

4f613f9

change copyright dates

d8deaf1

introducing tests for mean variance normalization op builder

2b1019b

format-checking fixes

74286c1

fixing tidy check errors

ba2ffcf

gchinora requested a review from causten as a code owner August 19, 2025 11:10

pfultz2 reviewed Aug 20, 2025

View reviewed changes

adding positive einsum test cases

e2572e7

gchinora requested a review from pfultz2 August 21, 2025 15:50

remove unnecessray include

81277d1

gchinora commented Aug 22, 2025

View reviewed changes

gchinora added 6 commits August 22, 2025 16:05

fixing copyright date

0f544be

fixing cppcheck warning

38e343b

the pipeline complained about formatting error; there was a mismatch …

7607695

…in the code layout between my vscode and the pipeline

Merge branch 'develop' into upstream/feature/op_bldr_unit_tests_1

07179b2

adding positive einsum test cases

b89f989

fixing input params for literal ctor

b601fa7

causten requested review from TedThemistokleous and Copilot September 3, 2025 13:05

Copilot AI reviewed Sep 3, 2025

View reviewed changes

TedThemistokleous added the roadmap Tasks to finish for a release label Sep 3, 2025

CharlieL7 self-requested a review September 17, 2025 13:06

fixing format check error

c09d4d0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Op Builder Tests 1. #4242

Op Builder Tests 1. #4242

gchinora commented Aug 19, 2025

Uh oh!

pfultz2 Aug 20, 2025

Uh oh!

gchinora Aug 21, 2025 •

edited

Loading

Uh oh!

pfultz2 Aug 20, 2025

Uh oh!

gchinora Aug 21, 2025 •

edited

Loading

Uh oh!

pfultz2 Aug 27, 2025

Uh oh!

gchinora Aug 22, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 3, 2025

Uh oh!

Copilot AI Sep 3, 2025

Uh oh!

Copilot AI Sep 3, 2025

Uh oh!

codecov bot commented Sep 10, 2025 •

edited

Loading

Uh oh!

Uh oh!

Op Builder Tests 1. #4242

Are you sure you want to change the base?

Op Builder Tests 1. #4242

Conversation

gchinora commented Aug 19, 2025

Uh oh!

pfultz2 Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

gchinora Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfultz2 Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

gchinora Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfultz2 Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

gchinora Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

gchinora Aug 21, 2025 •

edited

Loading

gchinora Aug 21, 2025 •

edited

Loading

codecov bot commented Sep 10, 2025 •

edited

Loading