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

[nnpackage,res] support RoPE operation #14013

Merged
merged 4 commits into from
Sep 24, 2024
Merged

Conversation

ys44kim
Copy link
Contributor

@ys44kim ys44kim commented Sep 13, 2024

This commit updates circle schema for RoPE op

ONE-DCO-1.0-Signed-off-by: youngsik kim [email protected]

draft : #13978
issue : #13972

ONE-DCO-1.0-Signed-off-by: youngsik kim <[email protected]>

draft : 13978
issue : 13972
@ys44kim ys44kim changed the title [circleschema] add RoPE operation [nnpackage,res] add RoPE operation Sep 13, 2024
@ys44kim ys44kim changed the title [nnpackage,res] add RoPE operation [nnpackage,res] support RoPE operation Sep 13, 2024
@ys44kim
Copy link
Contributor Author

ys44kim commented Sep 13, 2024

@glistening
please review this PR.

@glistening
Copy link
Contributor

  1. We need version up of circle schema. (0.9 -> 0.10).
  2. Usually we don't update circle schema for an addition of operator as we see 0.9 case. Because the version of circle_schema affects many other modules. Personally I prefer to update with other changes together. Let's hear others' opinions also.

@ys44kim
Copy link
Contributor Author

ys44kim commented Sep 19, 2024

  1. We need version up of circle schema. (0.9 -> 0.10).

as far as I know, when release, version up.
did I know wrong? or do you mean to change the version in advance for the next release?
and, Is there a rule to version up?

@glistening
Copy link
Contributor

glistening commented Sep 20, 2024

as far as I know, when release, version up. did I know wrong?

First, I don't know how others manages the version.
However, Personally I believe the version should be up as soon as the content of circle_schema.fbs changes. At least, "Revision History" needs to contain the change log.

Second, the public release and the version of circle_schema.fbs should be independent. We don't need to wait or release publicly to update circle_schema.

Third, assume that you're right (though I don't like this style), you should not change the content under 0.9. It is the snapshot (read-only) for specific version when the version is named.

@glistening
Copy link
Contributor

glistening commented Sep 24, 2024

I don't want to block this PR. But it seems blocked.

@seanshpark Is it okay to update res/CircleSchema/0.9/circle_schema.fbs, which is used for frontend. ? I guess not.

$+$ @hseok-oh
Is it okay to update nnpackage/schema/circle_schema.fbs?

Just for your information,
netron uses nnpackage/schema/circle_schema.fbs as schema
See https://github.com/lutzroeder/netron/blob/f4d88dcc63db415358e250e9b067e9341a32c042/tools/circle#L14.

@hseok-oh
Copy link
Contributor

OK. But please add comment in revision history of v0.9 (line 36~)

ONE-DCO-1.0-Signed-off-by: youngsik kim <[email protected]>

draft: Samsung#13978
issue: Samsung#13972
@ys44kim
Copy link
Contributor Author

ys44kim commented Sep 24, 2024

OK. But please add comment in revision history of v0.9 (line 36~)

thanks, i updated

@glistening
Copy link
Contributor

I don't think it is a good idea to add change after 0.9 is already there.
Someone may see the different content of the same version of 0.9.
However, I don't block if other users (frontend, python-tools, ...) are okay.

$+$ @ejjeong

ONE-DCO-1.0-Signed-off-by: youngsik kim <[email protected]>

draft: Samsung#13978
issue: Samsung#13972
@ejjeong
Copy link
Contributor

ejjeong commented Sep 24, 2024

@hseok-oh
Copy link
Contributor

hseok-oh commented Sep 24, 2024

I think circle schema version 0.9 is not fixed schema yet, so it's OK to update schema without version up.
Runtime has no issue updating without version up.


(ADDED) If version is changed to 0.10, luci may use new module (maybe mio-circle10?), then it also be effected runtime ODC build indirectly.

@seanshpark
Copy link
Contributor

seanshpark commented Sep 24, 2024

luci may use new module

mio-circle09 fixed name will be is being used (#13665) and maybe renamed without version number someday (#13990)

@seockho-kim
Copy link
Contributor

I don't want to block this PR. But it seems blocked.

@seanshpark Is it okay to update res/CircleSchema/0.9/circle_schema.fbs, which is used for frontend. ? I guess not.

Just for your information, netron uses nnpackage/schema/circle_schema.fbs as schema See https://github.com/lutzroeder/netron/blob/f4d88dcc63db415358e250e9b067e9341a32c042/tools/circle#L14.

FYI, frontend now uses 'nnpackage/schema/circle_schema.fbs'
#13951

Copy link
Contributor

@glistening glistening left a comment

Choose a reason for hiding this comment

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

LGTM. By offline talk, I heard that python-tools is okay, and other internal tools uses commit id, not the version number.

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

@seanshpark seanshpark merged commit e865a10 into Samsung:master Sep 24, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants