[compiler/circle-input-names] Introduce a new tool displaying input names #14472
Conversation
…ames The new tool 'circle_input_names' displays the all input names of each operator in JSON format. ONE-DCO-1.0-Signed-off-by: Jonghwa Lee <[email protected]>
| template <> struct is_VariadicOut<CircleIf> : std::true_type {}; | ||
| template <> struct is_VariadicOut<CircleWhile> : std::true_type {}; | ||
| // clang-format on | ||
|
|
There was a problem hiding this comment.
Note
Here, SFINAE is used to create CircleNode depending on default contructor for each.
| file(GLOB SOURCES "src/*.cpp") | ||
|
|
||
| add_executable(circle_input_names ${SOURCES}) | ||
| target_include_directories(circle_input_names PUBLIC include) |
There was a problem hiding this comment.
I do not see include folder (in also draft). can we remove this line?
There was a problem hiding this comment.
Oh, I'll remove it.
Thanks ;)
|
I can see |
Okay, I'll revise it too. |
It removes a unnecessary line in CMakeLists by the comment. ONE-DCO-1.0-Signed-off-by: Jonghwa Lee <[email protected]>
| { | ||
| std::string lookup(const loco::Node *) const override { return ""; } | ||
| }; | ||
|
|
There was a problem hiding this comment.
Note
In fact, this tool is nothing but prints all input names defined in CircleNodeSummaryBuilder.
However, it requires some hacks to instantiate the builder outside of the module.
All above codes are the Hacks.
It introduces a test to check the result of circle-input-names. ONE-DCO-1.0-Signed-off-by: Jonghwa Lee <[email protected]>
| // "{" | ||
| json_export.open_brace(); | ||
| #define CIRCLE_NODE(OP, CIRCLE_OP) \ | ||
| { \ |
There was a problem hiding this comment.
As I said in the offline, it's a bit hard to know why these are needed. Therefore, adding a comment about why dummy values are set would be helpful.
Moreover, functions that fill with dummy data can be made into a single function, too. The function name would show what it does.
void populate_dummy_data(/*..*/):
{
add_fused_actfn_option(&node);
add_padding_option(&node);
add_mode_option(&node);
}The name candidates are follows.
- fill_with_dummy_data
- initialize_with_defaults
- mock_data_setup
- prepare_mock_values
- set_dummy_state
There was a problem hiding this comment.
Good point, I'll add a wrapper function as you suggested.
Thanks !
There was a problem hiding this comment.
Applied!
@mhs4670go
PTAL if you have a spare time :)
| @@ -0,0 +1,3 @@ | |||
| # circle-input-names | |||
|
|
|||
| `circle-input-names` is a tool to generate input names of the Circle model's operators in JSON format. | |||
There was a problem hiding this comment.
How about show a part of expected outputs? This would be helpful to know what it does.
There was a problem hiding this comment.
Good suggestion 2.
I'll update it too.
There was a problem hiding this comment.
Applied!
@mhs4670go
ditto.
It adds a wrapper function for mocking functions which helps to to create circle node summary. And it also modifies other function names to make them easier to understand. ONE-DCO-1.0-Signed-off-by: Jonghwa Lee <[email protected]>
It adds the sample of result to README file. ONE-DCO-1.0-Signed-off-by: Jonghwa Lee <[email protected]>
ONE-DCO-1.0-Signed-off-by: Jonghwa Lee <[email protected]>
ONE-DCO-1.0-Signed-off-by: Jonghwa Lee <[email protected]>
batcheu
force-pushed
the
circle-input-names
branch
from
11d2c76 to
dc64c85
Compare
ONE-DCO-1.0-Signed-off-by: Jonghwa Lee <[email protected]>
| auto summary = create_circle_node_summary(&node); \ | ||
| get_input_names_from_summary(&node, summary, json_export); \ | ||
| } | ||
| #define CIRCLE_VNODE(_1, _2) |
There was a problem hiding this comment.
Just curiosity, what does this code mean? What does it do?
There was a problem hiding this comment.
Defined to ignore CIRCLE_VNODEs in circle_nodes.lst.
There was a problem hiding this comment.
Ah, I got it. This would be better to be described as a comment. Maybe in the next PR.
| throw std::runtime_error("CIRCLE_INPUT_NAMES_PATH is not found"); | ||
| } | ||
|
|
||
| FILE *fp = popen(cmd.c_str(), "r"); |
There was a problem hiding this comment.
(Optional) Just as a minor comment, I think we can revisit the program not to use popen function in the test by making a function that returns the stringstream. Which makes a test flow simpler and makes a program have higher extensibility.
int main()
{
# ..
auto ss_input_names = # ..
std::cout << ss_input_names.str();
return 0;
}But, this program seems not to have high maintainability. So, it's not that important.
There was a problem hiding this comment.
If you don't mind, could you elaborate more about it?
Is there a way to redirect forked process standard output to stream buffer?
There was a problem hiding this comment.
@seanshpark explained about it in offline ;)
Now, I understand what you meant, Thanks 👍
I'll land with this message. :) |
The new tool 'circle_input_names' displays the all input names of each operator in JSON format.
This will be used to generate input metadata in Model Explorer.