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

[tflchef] Use extype and custom_code type for custom operators #11785

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

Seunghui98
Copy link
Contributor

This introduces extype and custom_code for custom operators.

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


Related Issue: #11741
Draft PR: #11750

@Seunghui98 Seunghui98 self-assigned this Oct 25, 2023
@Seunghui98 Seunghui98 added the PR/ready for review It is ready to review. Please review it. label Oct 25, 2023
Comment on lines 144 to 145
if ((operation.has_extype() && operation.extype() == "Custom") ||
(operation.has_type() && operation.type() == "Custom"))
continue;

This comment was marked as outdated.

Comment on lines 48 to 57
static void CheckCustomOp(const tflchef::Operation operation, std::string op_type)
{
if ((operation.has_extype() && operation.extype() == "Custom") ||
(operation.has_type() && operation.type() == "Custom"))
{
assert(operation.has_custom_code());
assert(operation.custom_code() == op_type);
}
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a function here to avoid duplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

Q) Can you explain more about what you mean by avoid duplication ?

@Seunghui98
Copy link
Contributor Author

@seanshpark @mhs4670go

As you(@seanshpark ) advised, I rewrote the code.
Could you review the code again ??

@@ -45,4 +45,14 @@ struct OpChefFactory
virtual std::unique_ptr<OpChef> create(const tflchef::Operation *operation) const = 0;
};

static void CheckCustomOp(const tflchef::Operation operation, std::string op_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

plz do not use static in header. there will be duplicate static functions across the callers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you advise me where it would be good...?

Please think of what the purpose of this function, and the scope of the functionality.

And looking in other global methods in Convert.h, please follow existing snake_case convention.

And does CheckCustomOp express the purpose of new method?

Naming is difficult and needs some experience, which may require try and error.

@seanshpark
Copy link
Contributor

@Seunghui98 , this PR has multiple context of changes and title/description of this PR doesn't seem to explain the changes.

@Seunghui98 Seunghui98 force-pushed the add_custom_op branch 4 times, most recently from f39a238 to 7fd0282 Compare October 30, 2023 03:06
@Seunghui98 Seunghui98 changed the title [tflchef] Fix tflchef to reflect extype and custom_code [tflchef] Use extype and custom_code type for custom operators Oct 30, 2023
@Seunghui98
Copy link
Contributor Author

Seunghui98 commented Oct 31, 2023

This PR will be reflected after the previous PR.
So, the associated logic is here.
The extype will be used for a while and will be deleted later.
Could you review the code again? (@seanshpark, @mhs4670go)

This commit includes replacing op type with custom_code if op_type is Custom or extype is Custom.

ONE-DCO-1.0-Signed-off-by: SeungHui Lee <[email protected]>
customcode_set.insert(operation.type());
if ((operation.has_extype() && operation.extype() == "Custom") || operation.type() == "Custom")
{
assert(operation.custom_code() != "");
Copy link
Contributor

Choose a reason for hiding this comment

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

This one would be more readable.

Suggested change
assert(operation.custom_code() != "");
assert(not operation.custom_code().empty());

@Seunghui98 Seunghui98 force-pushed the add_custom_op branch 2 times, most recently from 3b99cd9 to 9e7ae17 Compare November 1, 2023 00:42
@Seunghui98 Seunghui98 force-pushed the add_custom_op branch 3 times, most recently from 93864d9 to 3a6ecd8 Compare November 1, 2023 02:22
@Seunghui98
Copy link
Contributor Author

@seanshpark @mhs4670go
I reflected comments in Apply comment commit.
Could you review the code again??

seanshpark
seanshpark previously approved these changes Nov 1, 2023
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

@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

@mhs4670go mhs4670go 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 ccc578c into Samsung:master Nov 1, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants