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

[res] Add GRU in CircleSchema #12597

Merged
merged 4 commits into from
Feb 19, 2024
Merged

Conversation

BalyshevArtem
Copy link
Contributor

This pr adds CirGru operation in circle_schema.fbs

for issue: #12320
from draft: #12319

ONE-DCO-1.0-Signed-off-by: Artem Balyshev [email protected]

This pr adds CirGru operation in circle_schema.fbs

ONE-DCO-1.0-Signed-off-by: Artem Balyshev <[email protected]>
@BalyshevArtem BalyshevArtem added the approval: 2 Require at least 2 approvals label Feb 2, 2024
@@ -1431,6 +1433,12 @@ table GeluOptions {
approximate: bool;
}

table CirGruOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this above BCQGatherOptions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@seanshpark seanshpark requested a review from hseok-oh February 4, 2024 21:45
@hseok-oh
Copy link
Contributor

hseok-oh commented Feb 5, 2024

Do you have any reason to use Gru, not GRU?
I recommended GRU because it is comes from abbreviation (Gated Recurrent Unit), such as RNN, LSTM (RNNOptions, LSTMOptions).

@glistening
Copy link
Contributor

In addition, it is the 1st operation which has the prefix Cir. Do we need this prefix?

@BalyshevArtem
Copy link
Contributor Author

In addition, it is the 1st operation which has the prefix Cir. Do we need this prefix?

@glistening, @hseok-oh
In this comment #12320 (comment) we discussed the reason for this decision. The main reason is to avoid conflict with tflite naming in future. I think it is good idea to emphasize that this is a circle operation.

Do you have any reason to use Gru, not GRU?

I can change CirGru to CirGRU, for it is okay both variants, If @seanshpark doesn't mind :)

@seanshpark
Copy link
Contributor

CirGru to CirGRU

OK with me :)

@BalyshevArtem
Copy link
Contributor Author

CirGru to CirGRU

OK with me :)

Done)

seanshpark
seanshpark previously approved these changes Feb 13, 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 thank you!

@glistening
Copy link
Contributor

@BalyshevArtem

@glistening, @hseok-oh In this comment #12320 (comment) we discussed the reason for this decision.

Basically @hseok-oh and I are working for runtime. I (and perhaps @hseok-oh) usually don't see CompilerFE issues and PRs. I see this PR because it is about schema (which is common to compiler and runtime).

The main reason is to avoid conflict with tflite naming in future. I think it is good idea to emphasize that this is a circle operation.

I don't think we need to add prefix Cir to GRU:

  1. tflite has not provided GRU, and it is not likely to happen in my opinion. (GRU is old-fashined op, ...)
  2. It is circle schema, we don't need to add Cir.

But I don't want to block this PR. It may be merged if it gets @hseok-oh's approval.
(For your information, @hseok-oh is off this week.)

@seanshpark
Copy link
Contributor

@BalyshevArtem , as we are adding new Op in current 0.7 version with some circumstances that there is no particular users (other tools or repos), and behind this, frontend has to do some laborious tasks to provide new mio-circleXX module and related changes.

But recently our internal repo released a package with 0.7 schema.

So, we have to upgrade the version to 0.8 to follow versioning rules.
Can you please add to new schema file in res folder?

If your tasks don't have direct relation with mio-circleXX module, then you can skip adding mio-circle08. I'll handle related tasks after new schema lands.

@seanshpark seanshpark self-requested a review February 14, 2024 02:59
@BalyshevArtem
Copy link
Contributor Author

I don't think we need to add prefix Cir to GRU:

@seanshpark,
What do you think about removing Cir prefix? For me both variants are ok)

@seanshpark
Copy link
Contributor

What do you think about removing Cir prefix?

I don't have strong thoughts to keep Cir prefix :), it was a choice to reduce the changes when the time comes. As @hseok-oh wrote, we can rename for tflite Op when it's introduced as GRU_TFLITE or any variants.

@BalyshevArtem
Copy link
Contributor Author

BalyshevArtem commented Feb 14, 2024

Can you please add to new schema file in res folder?

Created 0.8 file in res folder and update GRU naming: CirGRU -> GRU

@BalyshevArtem BalyshevArtem changed the title [res] Add CirGru in CircleSchema [res] Add GRU in CircleSchema Feb 14, 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 thank you!

Copy link
Contributor

@hseok-oh hseok-oh left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@hseok-oh hseok-oh merged commit 3e47760 into Samsung:master Feb 19, 2024
6 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants