Cavusmustafa/modular backend review v6 #762
Conversation
| NGraphClusterManager::NumberOfClusters() == 1) { | ||
| multi_req_execution = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
Minor: Add a log to let know if batching is enabled
ngraph_bridge/ie_vadm_engine.h
Outdated
| #include <ie_core.hpp> | ||
| #include <memory> | ||
| #include <string> | ||
| #include <vector> | ||
| #include "ngraph_bridge/ie_backend_engine.h" |
There was a problem hiding this comment.
General guideline followed in the bridge
standard includes
TF
OV
nGraph
bridge
ngraph_bridge/ie_backend_engine.cc
Outdated
| #include "ngraph_bridge/ie_backend_engine.h" | ||
| #include <iostream> | ||
| #include "ngraph_bridge/ie_utils.h" |
There was a problem hiding this comment.
General guideline followed in the bridge
standard includes
TF
OV
nGraph
bridge
ngraph_bridge/ie_backend_engine.h
Outdated
| #include <ie_core.hpp> | ||
| #include <memory> | ||
| #include <string> | ||
| #include <vector> | ||
| #include "ngraph_bridge/ie_tensor.h" | ||
|
|
ngraph_bridge/ie_basic_engine.h
Outdated
| #include <ie_core.hpp> | ||
| #include <memory> | ||
| #include <string> | ||
| #include <vector> | ||
| #include "ngraph_bridge/ie_backend_engine.h" |
ngraph_bridge/ie_utils.h
Outdated
| #include <atomic> | ||
| #include <ie_core.hpp> | ||
| #include <mutex> | ||
| #include <ostream> | ||
| #include <vector> |
ngraph_bridge/ie_vadm_engine.cc
Outdated
| #include "ngraph_bridge/ie_vadm_engine.h" | ||
| #include <iostream> | ||
| #include "ngraph_bridge/ie_utils.h" |
ngraph_bridge/ie_basic_engine.cc
Outdated
| #include "ngraph_bridge/ie_basic_engine.h" | ||
| #include <iostream> | ||
| #include "logging/ngraph_log.h" | ||
| #include "ngraph_bridge/ie_utils.h" |
There was a problem hiding this comment.
General guideline followed in the bridge
standard includes
TF
OV
nGraph
bridge
ngraph_bridge/executable.cc
Outdated
| if (multi_req_execution) { | ||
| m_ie_engine->enable_multi_req_execution(); | ||
| } else { | ||
| m_ie_engine->disable_multi_req_execution(); |
There was a problem hiding this comment.
Is this needed, isn't it disabled by default?
There was a problem hiding this comment.
It's added in case batching might be disabled during consecutive executions. But we don't have advanced checks for now and there is no scenario to disable batching if it's already enabled. I updated it accordingly.
I'm confused about using CamelCase. Some function names does not use it in bridge now. Is there a plan to update them all? |
Yes, we will update the function names that don't follow it |
There was a problem hiding this comment.
Thanks for the revised PR. I have a few high-level comments:
-
Please break this PR into individual self-contained PRs. The changes here not only introduce a new backend infrastructure (with possibly redundant interfaces to those that currently exist) but also add a new HDDL/VADM backend. I'd propose that we add a modular backend first, followed by a separate PR to add a new HDDL/VADM backend with the tests enabled for it.
-
Secondly, please take a look at the TensorFlow Contribution guidelines and standards for the general contribution guidelines that we use. Please use a short explanatory title for the PR and add a description to your PR if the title is not self-explanatory. The guide also points to the Google C++ style guide that's being used for the code in here.
No description provided.