[IE][VPU]: Configuration options in VPU plugins refactoring #3211
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ggladilov
commented
Nov 18, 2020
ilyachur
approved these changes
Nov 19, 2020
ilya-lavrenov
approved these changes
Nov 19, 2020
|
@vinograd47 @ArtemySkrebkov-intel Please take a look. Right now there are only changes for options containers, I'll fill it with options step by step |
|
I've update the PR. There are new changes for |
|
@vinograd47 @ArtemySkrebkov-intel ping |
|
@ntfshard |
ntfshard
reviewed
Mar 17, 2021
|
|
||
| template<class Key, class Value, template<class...> class Map> | ||
| inline std::vector<Key> getKeys(const Map<Key, Value>& map) { | ||
| auto keys = std::vector<Key>{}; |
There was a problem hiding this comment.
Just curious, what is the purpose of such variable creation syntax.
std::vector<Key> keys; is shorter and more common way
There was a problem hiding this comment.
There is no much functional details about that, just code style. You can see some motivation at Herb Sutter's talk on cppcon about that.
ntfshard
reviewed
Mar 17, 2021
inference-engine/tests_deprecated/functional/vpu/common/layers/myriad_layers_blob_test.cpp
Show resolved
Hide resolved
ntfshard
reviewed
Mar 17, 2021
ggladilov
commented
Mar 24, 2021
Comment on lines
+28
to
+30
| namespace InferenceEngine { | ||
| namespace details { | ||
|
|
There was a problem hiding this comment.
@ilya-lavrenov @apankratovantonp
Please take a look at changes at this file. For explanation see commit message. Thanks
rnugmanx
pushed a commit
to rnugmanx/openvino
that referenced
this pull request
Aug 26, 2021
…toolkit#3211) * [IE]: Enables Abstract class -> Parameter conversion support Parameter has templated constructor allowing to write code ``` Parameter p = i; // i of type int for example ``` This constructor uses SFINAE to resolve ambiguity with move-constructor, so checks that argument is not of the same type. In case it's not the same type it calls std::tuple constructors that constructs an instance of argument type. In the following case: ``` Parameter p = static_cast<Parameter>(abstractRef); // abstractRef is a reference to abstract class ``` We have a reference to some abstract class that defines explicit cast operator to Parameter. In contrast with expectations, instead of cast operator, Parameter constructor is instantiated, since template type deduction for Parameter constructor didn't fail (abstract class has not the same type as Parameter). Instantiation of tuple constructor used inside failed: it's impossible to create an instance of abstract class what lead to compile-time error. To resolve the issue additional condition introduced to check if argument type is abstract. Signed-off-by: Gladilov, Gleb <[email protected]> * [IE]: Enables PrintTo method for Parameter and tests on it Inference Engine API for configuration options uses Parameter type as a return type of GetConfig method. Parameter is intended to store object associated with configuration option. To support objects of different types its constructor is templated. Parameter overloads cast operators which are templated as well. Both constructor and cast operators are implicit, which makes it possible to implicitly convert any type to Parameter and vice versa. Since Parameter is a part of Inference Engine configuration API it's essential google tests on API contain Parameter as tests parameter. For each test parameter Google Test framework tries to print it to an output stream. For that purpose, Google Test checks if test parameter has output stream operator or PrintTo method. If not, it checks if it could be implicitly converted to integral type and, in this case, prints it as a long integer. InferenceEngine::Parameter does not define output stream operator, but could be implicitly converted to an integer, according cast operators mentioned above, so Google Test tries to convert to integer. Since Parameter not necessarily contains integer, this conversion throws an exception of type mismatch, which makes it impossible to use Parameter in Google Test framework as is. In order to resolve that issue Parameter should define either output stream operator or PrintTo method. If Parameter will define output stream operator it will make it possible to compile streaming almost any object to an output stream. The reason for it is C++ checks if object could be implicitly converted to other type which defines output stream operator, if objects itself doesn't do it (e.g. `stream << "text";` calls std::string::operator<<, since char const* is implicitly convertible to std::string). Taking this into consideration the only way to support Parameter in Google Test without breaking backward compatibility is define PrintTo method. Signed-off-by: Gladilov, Gleb <[email protected]> * [IE]: Fixes ill-formed extending std names According to the standard: The behavior of a C++ program is undefined if it adds declarations or definitions to namespace std or to a namespace within namespace std unless otherwise specified. A program may add a template specialization for any standard library template to namespace std only if the declaration depends on a user-defined type and the specialization meets the standard library requirements for the original template and is not explicitly prohibited. As as an unexpected result, InferenceEngine::Parameter that contains std::vector<std::string> can be printed via PrintTo. In that case operator<< version from Inference Engine is picked up. Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Moves CompilationConfig out of GT header Keeping config in a separate header simplifies migration to new interface. Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Removes Platform enum Since there is enum from MVNC for the same purpose there is no need in Platform anyway Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Introduces containers utility header Contains some helpers to work with C++ maps Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Introduces new configuration API The main ideas are separate option-specific logic from common container, automate logic processing public vs private, deprecated, compile-time vs runtime-time options and remove code duplication. Since IE defines configuration API using std::string and Parameter, options have to provide ways to be represented as Parameter (ex.: GetConfig is called) and be defined using std::string (ex.: SetConfig is called). Keeping information about actual key value is useful for error reporting. New API fallbacks to previous version in case of unsupported options are requested. This way migration becomes iterative and looks simpler. Options containers are related to corresponding components: CompilationConfig (name to be changed) - GraphTransformer, PluginConfiguration - base class for plugins configurations, MyriadConfiguration - Myriad plugin configuration, HDDLConfiguration - HDDL plugin configuration (to be introduced in a separate request) Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Replaces CompilationConfig with PluginConfiguration Some of options to be refactored are stored inside CompilationConfig. CompilationConfig is passed to graph transformer as a compiler to be processed. Since it's separate data structure and migration process is iterative we need a mechanism to provide some of compilation options from new interface and some from old. It cannot be done via plugin specific class (MyriadConfiguration), since there are others plugins as graph transformer users. Plugin specific class (MyriadConfiguration) already inherits from old version (MyriadConfig), which in turn inherits from ParsedConfig containing CompilationConfig. To resolve the issue MyriadConfig inheritance from ParsedConfig is made virtual to make it possible for PluginConfiguration to virtually inherit from ParsedConfig as well an so make PluginConfiguration data structure for configuration options for graph transformer. Since PluginConfiguration is base class of MyriadConfiguration as well as MyriadConfig and inheritance is virtual plugin just casts its specific configuration to base one passing to graph transformer. Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Enables new tests on configuration API * Enables following new shared tests on configuration API * Can load network with empty configuration * Check default value for configuration option * Can load network with correct configuration * Check custom value for configuration option (set and compare) * Check public configuration options are visible through API * Check private configuration options are invisible through API * Check GetConfig throws an exception on incorrect key * Refactors myriad plugin instantiations for shared tests Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Extracts LogLevel enum to a separate header Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Refactors LOG_LEVEL configuration option Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Refactors COPY_OPTIMIZATION configuration option Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Fixes behavior tests build Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Updates tests on new exception class Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Removes unused variable from mvnc test Signed-off-by: Gladilov, Gleb <[email protected]> * [IE][VPU]: Removes SizeVector streaming call New assertion macro IE_ASSERT implementation uses output streaming operator with r-value reference argument as a stream. This prevents the compiler from picking up overload from InferenceEngine::details, since our version takes stream by non-const l-value reference. Since there is no simple solution to provide output streaming operator overload for r-value references as well and this call is just a message for assert in test utilities, it was decided just to remove call for now. Signed-off-by: Gladilov, Gleb <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Task
#-41782
Description
Removes
PlatformenumSince there is enum from
MVNCfor the same purpose there is no need inPlatformanywayEnables abstract class ->
Parameterconversion supportIntroduces containers utility header
Contains some helpers to work with C++ maps
Introduces new configuration API
The main ideas are separate option-specific logic from common container, automate logic processing public vs private, deprecated, compile-time vs runtime-time options and remove code duplication.
Enables
PrintTomethod forInferenceEngine::Parameterto enable its usage in Google Test frameworkRefactors shared configuration tests in Inference Engine
Since IE defines configuration API using
std::stringandParameter, options have to provide ways to be represented asParameter(ex.:GetConfigis called) and be defined usingstd::string(ex.:SetConfigis called). Keeping information about actual key value is useful for error reporting.New API fallbacks to previous version in case of unsupported options are requested. This way migration becomes iterative and looks simpler.
Options containers are related to corresponding components:
CompilationConfig(name to be changed) -GraphTransformer,PluginConfiguration- base class for plugins configurations,MyriadConfiguration- Myriad plugin configuration,HDDLConfiguration- HDDL plugin configuration (to be introduced in a separate request)Options will be migrate in further commits