Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[luci/service] Support BMM dynamic shape inferece #13759

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

zetwhite
Copy link
Contributor

@zetwhite zetwhite commented Aug 26, 2024

This PR supports dynamic shpae inference ofr BatchMatMul Op.

ONE-DCO-1.0-Signed-off-by: SeungHui Youn [email protected]

for : #13697

@zetwhite zetwhite force-pushed the 0822/bmm branch 3 times, most recently from 9b84b16 to 856c510 Compare August 27, 2024 09:17
@zetwhite zetwhite changed the title WIP: [luci/service] Support BMM dynamic shape inferece [luci/service] Support BMM dynamic shape inferece Aug 27, 2024
@zetwhite zetwhite marked this pull request as ready for review August 27, 2024 09:20
@zetwhite zetwhite added approval: 2 Require at least 2 approvals PR/ready for review It is ready to review. Please review it. labels Aug 27, 2024
@zetwhite zetwhite requested a review from a team August 27, 2024 09:20
@zetwhite zetwhite requested review from seanshpark and ys44kim August 27, 2024 11:38
@shs-park
Copy link
Contributor

shs-park commented Aug 27, 2024

I made some test cases with tensorflow python code.
I could get exceptions in Test 4 and Test 6 cases.

Please note when you make test cases.

Python Test Code
import tensorflow as tf

# Example 1: Simple 2D Matrices Batch
A = tf.constant([[[1, 2, 3], [4, 5, 6]],
                 [[7, 8, 9], [10, 11, 12]]], dtype=tf.float32)  # Shape: (2, 2, 3)

B = tf.constant([[[1, 4], [2, 5], [3, 6]],
                 [[7, 10], [8, 11], [9, 12]]], dtype=tf.float32)  # Shape: (2, 3, 2)

# Perform batch matrix multiplication using tf.matmul
result = tf.matmul(A, B)
print("Example 1 Result (Shape: {}):\n{}".format(result.shape, result.numpy()))

# Example 2: Broadcasting in Batch Dimensions
A = tf.constant([[[1, 2, 3], [4, 5, 6]],
                 [[7, 8, 9], [10, 11, 12]],
                 [[13, 14, 15], [16, 17, 18]]], dtype=tf.float32)  # Shape: (3, 2, 3)

B = tf.constant([[[1, 4], [2, 5], [3, 6]]], dtype=tf.float32)  # Shape: (1, 3, 2)

# Perform batch matrix multiplication with broadcasting using tf.matmul
result = tf.matmul(A, B)
print("Example 2 Result (Shape: {}):\n{}".format(result.shape, result.numpy()))

# Example 3: Edge case with zero-dimension batch
A = tf.random.normal([0, 2, 3])  # Shape: (0, 2, 3)
B = tf.random.normal([0, 3, 2])  # Shape: (0, 3, 2)

# Perform batch matrix multiplication using tf.matmul
result = tf.matmul(A, B)
print("Example 3 Result (Shape: {}):\n{}".format(result.shape, result.numpy()))

# Example 3-2: Edge case with zero-dimension batch
A = tf.random.normal([3, 2, 3])  # Shape: (3, 2, 3)
B = tf.random.normal([3, 2])  # Shape: (3, 2)

# Perform batch matrix multiplication using tf.matmul
result = tf.matmul(A, B)
print("Example 3-2 Result (Shape: {}):\n{}".format(result.shape, result.numpy()))

# Example 4: Edge case with zero-dimension batch
A = tf.random.normal([2, 2, 3])  # Shape: (2, 2, 3)
B = tf.random.normal([0, 3, 2])  # Shape: (0, 3, 2)

# Perform batch matrix multiplication using tf.matmul
#result = tf.matmul(A, B)
print("Example 4 Result EXCEPTION")

# Example 5: Edge case with rank-2 batch
A = tf.random.normal([2, 3])  # Shape: (2, 3)
B = tf.random.normal([3, 2])  # Shape: (3, 2)

# Perform batch matrix multiplication using tf.matmul
result = tf.matmul(A, B)
print("Example 5 Result (Shape: {}):\n{}".format(result.shape, result.numpy()))

# Example 6: Edge case with rank-2 and rank-1 atch
A = tf.random.normal([2, 3])  # Shape: (2, 3)
B = tf.random.normal([3])  # Shape: (3)

# Perform batch matrix multiplication using tf.matmul
#result = tf.matmul(A, B)
print("Example 6 Result EXCEPTION")

Copy link
Contributor

@shs-park shs-park left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments, PTAL
=)

zetwhite and others added 2 commits August 28, 2024 10:22
This PR supports dynamic shpae inference ofr BatchMatMul Op.

ONE-DCO-1.0-Signed-off-by: SeungHui Youn <[email protected]>
@jinevening
Copy link
Contributor

LGTM with minor comment https://github.com/Samsung/ONE/pull/13759/files#r1733773189

@zetwhite zetwhite force-pushed the 0822/bmm branch 2 times, most recently from 262d0fe to 72a02d3 Compare August 28, 2024 02:31
seanshpark
seanshpark previously approved these changes Aug 28, 2024
Copy link
Contributor

@seanshpark seanshpark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +95 to +96
throw_unless((not contain_zero(x_shape)), "x_shape should NOT have 0");
throw_unless((not contain_zero(y_shape)), "y_shape should NOT have 0");
Copy link
Contributor Author

@zetwhite zetwhite Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +382 to +404
TEST(ShapeRuleTest, bmm_empty_input_NEG)
{
luci::CircleInput input_x;
luci::CircleInput input_y;
luci::CircleBatchMatMul bmm;

input_x.shape({0, 2});
input_x.shape_status(luci::ShapeStatus::VALID);

input_y.shape({2, 4});
input_y.shape_status(luci::ShapeStatus::VALID);

bmm.x(&input_x);
bmm.y(&input_y);

loco::TensorShape shape;
luci::sinf::Rule shape_inf_rule;

// (0, 2) x (2, 4)
// ^
// => error, x should not be empty
ASSERT_ANY_THROW(shape_inf_rule.infer(&bmm, shape));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(newly added) to handle empty input

Comment on lines +71 to +100
TEST(ShapeRuleTest, bmm_broadcast_known_dim_2)
{
luci::CircleInput input_x;
luci::CircleInput input_y;
luci::CircleBatchMatMul bmm;

input_x.shape({5, 4, 3});
input_x.shape_status(luci::ShapeStatus::VALID);

input_y.shape({3, 8});
input_y.shape_status(luci::ShapeStatus::VALID);

bmm.x(&input_x);
bmm.y(&input_y);

loco::TensorShape shape;
luci::sinf::Rule shape_inf_rule;

ASSERT_TRUE(shape_inf_rule.infer(&bmm, shape));

// (5, 4, 3) x (3, 8) -> (5, 3, 8)
// output shape should be (5, 4, 8)
ASSERT_EQ(3, shape.rank());
ASSERT_TRUE(shape.dim(0).known());
ASSERT_TRUE(shape.dim(1).known());
ASSERT_TRUE(shape.dim(2).known());
ASSERT_EQ(5, shape.dim(0).value());
ASSERT_EQ(4, shape.dim(1).value());
ASSERT_EQ(8, shape.dim(2).value());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ys44kim I added the TC for the case you asked about. :)

Copy link
Contributor

@seanshpark seanshpark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@shs-park shs-park left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, LGTM!
=)

Copy link
Contributor

@jinevening jinevening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jinevening
Copy link
Contributor

Ping @ys44kim

Copy link
Contributor

@ys44kim ys44kim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -30,4 +76,72 @@ luci::CircleNode *CloneNodeLet<CN::ABC>::visit(const luci::CircleBatchMatMul *no
return cloned;
}

// BatchMatMulV2 supports broadcasting in the batch dimensions(BatchMatMul doesn't)
// TODO Distinguish BatchMatMul and BatchMatMulV2
loco::TensorShape sinf::Algorithm::visit(const luci::CircleBatchMatMul *node)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zetwhite , please check #13780 (review)

can you apply here too? or as getting approvals takes time and you've go all ACKs,
apply in another PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if go with another PR, you can apply changes to all Ops (I think there aren't too many)

Copy link
Contributor Author

@zetwhite zetwhite Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to merge this PR as is. (not for waiting approvals again)
If fine, I'll make another PR to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can apply changes to all Ops (I think there aren't too many)

Oh, I see. I apply it at once.

@seanshpark seanshpark merged commit 36c4001 into Samsung:master Aug 28, 2024
10 checks passed
@seanshpark
Copy link
Contributor

seanshpark commented Aug 28, 2024

ah... commit title message had typo ... -_-;

This PR supports dynamic shpae inference ofr BatchMatMul Op.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: 2 Require at least 2 approvals PR/ready for review It is ready to review. Please review it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants