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

[Draft][tflchef/circlechef] Use a dedicated type for custom operators #11750

Closed
wants to merge 1 commit into from

Conversation

Seunghui98
Copy link
Contributor

On going draft to use a dedicated type for custom operators in the recipe.

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


Related Issue : #11741

@Seunghui98 Seunghui98 added DRAFT A draft issue or PR for sharing one's current working status and discussion. PR/NO MERGE Please don't merge. I'm still working on this :) labels Oct 21, 2023
@Seunghui98 Seunghui98 self-assigned this Oct 23, 2023
Comment on lines 180 to 182
if (operation.type() == "Custom")
{
customcode_set.insert(operation.custom_code());
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These codes can substitute below codes.

auto op_chef = op_chef_registry().lookup(operation.type()).create(&operation);
if (op_chef->code() == circle::BuiltinOperator_CUSTOM)
  customcode_set.insert(operation.type());

Which means above codes should be removed.

Copy link
Contributor Author

@Seunghui98 Seunghui98 Oct 23, 2023

Choose a reason for hiding this comment

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

@mhs4670go

Thank you for the comment ! :)

@@ -182,6 +194,12 @@ std::set<std::string> gather_customcode_set(const ::circlechef::ModelRecipe &mod
const auto &graph = model_recipe.graph(g);
for (const auto &operation : graph.operation())
{
if (operation.type() == "Custom")
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -18,13 +18,14 @@ operand {
}
}
operation {
type: "All"
type: "Custom"
Copy link
Contributor

Choose a reason for hiding this comment

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

to land separate PRs for compiler and res files,
we need to introduce some intermediate changes
for example,
1/ in res, introduce extype: "Custom" and custom_code: "All"
2/ in tflchef, if extype exist, use that, if not use type
3/ in res, change type and remove extype
4/ in tflchef remove related with extype

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment!! I will upload the code PR as you guided me. :)

@@ -29,7 +29,11 @@ AddV2Chef::custom_value(flatbuffers::FlatBufferBuilder &fbb) const
{
auto &operation = (*_operation);

assert(operation.type() == "AddV2");
if (operation.has_extype() && operation.extype() == "Custom")
Copy link
Contributor

Choose a reason for hiding this comment

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

why not check type() == Custom` too ?

{
assert(operation.has_custom_code());
assert(operation.custom_code() == "AddV2");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

as these blocks are repeated several times, would be better to add a method

@Seunghui98 Seunghui98 force-pushed the add_custom_code branch 2 times, most recently from 50d3808 to 874b1fd Compare October 27, 2023 06:18
On going draft to use a dedicated type for custom operators in the recipe.

ONE-DCO-1.0-Signed-off-by: SeungHui Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DRAFT A draft issue or PR for sharing one's current working status and discussion. PR/NO MERGE Please don't merge. I'm still working on this :)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants