-
Notifications
You must be signed in to change notification settings - Fork 2
Get rid of "parallel" and "oneDNN" parameters in "build_graph" #223
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
==========================================
+ Coverage 83.84% 83.97% +0.13%
==========================================
Files 44 44
Lines 2655 2665 +10
Branches 1466 1466
==========================================
+ Hits 2226 2238 +12
Misses 219 219
+ Partials 210 208 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
app/Graph/acc_check.cpp
Outdated
| int main(int argc, char* argv[]) { | ||
| std::string model_name = "alexnet_mnist"; | ||
| bool parallel = false; | ||
| // bool parallel = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, remove all debugging leftovers
app/Graph/graph_build.cpp
Outdated
| int main(int argc, char* argv[]) { | ||
| std::string model_name = "alexnet_mnist"; | ||
| bool parallel = false; | ||
| // bool parallel = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
| // bool parallel = false; | ||
| bool onednn = false; | ||
| for (int i = 1; i < argc; ++i) { | ||
| if (std::string(argv[i]) == "--parallel") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having --parallel as a runtime parameter still sounds reasonable. Why it was removed?
It we have decided to remove this then it feels like --onednn flag needs to be removed as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static analizys complained about an unused parallel variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why onednn is used then. They hold the similar purpose at this point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onednn we can only set it this way, when Andrey implements his version with parallel, he will most likely return the --parallel processing here, if I understand the question correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I got it. This explains what I was concerned about
include/graph/graph.hpp
Outdated
| } | ||
| }; | ||
| } // namespace it_lab_ai | ||
| } // namespace it_lab_ai |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
test/inference/test_inference.cpp
Outdated
| Tensor output = make_tensor(vec, sh1); | ||
| InputLayer a1(kNhwc, kNchw, 1, 2); | ||
|
|
||
| // Используем shared_ptr для всех слоев |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
#218