openvinotoolkit / openvino

OpenVINO™ is an open-source toolkit for optimizing and deploying AI inference
https://docs.openvino.ai
Apache License 2.0
7.36k stars 2.31k forks source link

Compile-time fallback for missing plugins.xml #13617

Closed CSBVision closed 1 year ago

CSBVision commented 2 years ago

Hello,

we recently started using OpenVINO as a backend for OpenCV's DNN module. As we only need a relatively small selection of features for this purpose (only the Intel CPU plugin as well as the Inference Engine frontend), we compiled OpenVINO ourselves. We prefer using OpenVINO as shared libraries and everything works fine. Still, the shared libraries depend on the plugins.xml configuration file at runtime. As this is redundant here - the desired configuration is already known at compile time - but also yields a runtime error if missing, we would like to propose to add a compile-time fallback for a missing plugins.xml, similar to the static library case.

By investigating how the plugin registration works in the static library case, we found the getStaticPluginsRegistry() function in ie_plugins.hpp that is generated by the plugins.cmake script. Here, we think that similarly generating the file by CMake should also be possible in the dynamic library case such that while calling Core::Core() in ie_core.cpp, similar code as in the function getStaticPluginsRegistry() could be used as a fallback if plugins.xml is missing at runtime.

Generally, we are open to modify the CMake or source code files accordingly. However, before programming anything we would like to discuss this idea first. So what do you think? Do you agree that this might be useful feature or does it even conflicts with your plans?

Thanks for your time!

ilya-lavrenov commented 2 years ago

Hi @CSBVision

Thanks for having an interest to such scenario.

As this is redundant here - the desired configuration is already known at compile time

One comment here is that such plugins.xml file can be changed after OpenVINO is compiled and you can add extra plugins here (ARM or NVIDIA plugins from OpenVINO Contrib). Also, users can add additional settings to plugins to modify the default behavior.

But I agree that for the most of use cases we work with limited set of plugins compiled together with OpenVINO itself. So, it would be good to have a combined approach like "known plugins" are located in Core itself, while extra plugins or plugins with overridden behavior can be passed as well. E.g. by default Core knows about CPU, but via plugins.xml we can override CPU with CPU + some non-default settings.

Let's try to think about how we can implement this. We will also think internally about it and hope together we can come up with final solution. Feel free to write more of your thought here.

Thanks.

ilyachur commented 2 years ago

In general we can register know plugin while are building the OpenVINO (like it happens for static build). In this case user also can change or rewrite the "default" plugins with using https://github.com/openvinotoolkit/openvino/blob/a7a14a89c8694e99c8a5aa0ac7682e336c4ca270/src/inference/include/openvino/runtime/core.hpp#L613

This method can allow to load the custom plugin in the runtime. If we have a name collision we can unload the "default" plugin: https://github.com/openvinotoolkit/openvino/blob/a7a14a89c8694e99c8a5aa0ac7682e336c4ca270/src/inference/include/openvino/runtime/core.hpp#L622

CSBVision commented 2 years ago

First of all, thanks for your positive answers. We discussed the issue and think that there are three possible alternatives (even though we do not know the code except looking briefly through the plugin registration part):

  1. Add a configuration-independent fallback case. For example, instead of plugins.xml -> not found: error try plugins.xml -> not found: try CPU plugin -> not found: error. Since this ignores the actual configuration at compile time, it is not ideal, but still an improvement if a configuration-dependent fallback turns out to be too complicated to integrate.

  2. Add a configuration-dependent fallback case, similar to the auto-generated ie_plugins.hpp in the static library case. Here, the ie_plugins.hpp file (or a different one with a similar functionality) could be generated to contain the respective code to register the plugins at compile time. Still, it is not clear to us how this ideally can be realized.

  3. Change the compilation of the plugins (e.g. by introducing a new CMake variable) to static libraries - even though OpenVINO itself is compiled as a shared library - and to ship a shared library that contains the plugins of choice that are registered at compile time as in the static library case. Optionally, further plugins might be used if plugins.xml exists. But if not, the complied-in ones are used. Ideally, the same concept should be transferred to the frontend modules (e.g. openvino_ir_frontend shared library) as well such that compiling OpenVINO this way results in a single shared library that contains all modules and does not depend on anything else (neither other shared library nor configuration files).

We neither know which option do you prefer nor how complicated they are to realize. The first two options yield the same result for our use case, still the second one is definitely cleaner. The third option is very interesting as well, still presumably is the most complicated alternative to realize. What do you think is the best option here?

As a first step, we tried to register the CPU plugin in Core::Core() instead of loading the configuration file (RegisterPlugins(ov::parseXmlConfig(xmlConfigFile));). However, the code is inside the constructor of InferenceEngine::Core and there is no ov::Core instance available to call register_plugin() on it. In any case, we think this should be solved as a first test before discussing possible next steps. We don't know whether this is covered by an API or code documentation. If this is the case, please let us know where this is available.

ilyachur commented 2 years ago

Hi,

I am not a big fan to introduce any additional logic like: something not found -> try to find something else if not found -> etc.. For me the idea with plugins.hpp looks better. For frontends, at the current moment we have a limited unconfigurable list of frontends, you cannot add something new without changes in OpenVINO, but you can deploy only necessary frontends, openvino will automatically skip anothers.

https://github.com/openvinotoolkit/openvino/blob/master/src/inference/src/ie_core.cpp#L1630 Here we automatically try to find default plugins xml and load plugins for it, need to extend this part in case if we have precompiled plugins.

CSBVision commented 2 years ago

Yes, generally we agree on that. Still, here maybe only trying the CPU shared library is still worth a try instead of throwing an error.

Nevermind, as a first step we tried to register the plugins at compile time by replacing the parse XML code by loading the CPU plugin. As you said, the method register_plugin is the way to go. However, we don't have a ov::Core instance available to call this function (cf. the last section of our previous comment). How can we register the shared library here? Once we succeed at registering the plugins at compile time, we try to find a configurable CMake-based solution to combine both approaches (XML file and compile-time fallback).

CSBVision commented 1 year ago

We investigated this issue a bit further and found a working solution that loads the CPU plugin if the plugins.xml file is missing. If this aligns with your expectations, it can form the basis for a CMake-dependent loading of the plugin libraries. Looking forward for your comments.

ilyachur commented 1 year ago

Hi @CSBVision,

Thank you for your proposal. Looks like it won't work in case if user have some list of pre-compiled plugins and also have plugins.xml (where lists of plugins don't have intersection). In this case we ignore pre-compiled plugins and use only information from plugins.xml.

CSBVision commented 1 year ago

Thanks for your comment! If the function findPluginXML successfully finds the xml configuration file (plugins.xml), it will be used as before. Otherwise, the exception in findPluginXML is not thrown but instead a runtime check in core.cpp::Core() checks whether the file was found or not. If not and xmlConfigFile.empty() holds, the compile-time configuration will be applied.

We tested both cases, i.e. plugins.xml is distributed and is not distributed with the OpenVINO DLLs, and both work as expected. If the plugins.xml is found, it always has higher priority.

Remark: Of course, instead of the compile-time configuration the CPU plugin will be used if the plugins.xml was not found. As mentioned before, the PR should only form the basis for a compile-time generation of the plugin loading code. As this depends on CMake-generated source code (which plugin libraries are compiled?), this will be addressed as a second step. Still besides configuring / refactoring the CMake stuff accordingly to generate the ie_plugins.hpp as required, everything else is done with the PR.

edit: Another possible feature are mixed configurations, i.e. some plugins are defined at runtime by plugins.xml while some others are defined at compile time. If you were referring to such a setting, this could also be implemented relatively easily once everything else works: Initialize the plugins first using the plugins.xml (i.e. check whether the file exists and initialize the DLLs accordingly) and thereafter always check for compiled plugins that were not initialized so far. It's mainly a question of preference whether these mixed configurations should be supported or not. As soon as the CMake stuff is changed accordingly, we can implement this in the preferred way. We think this is a design decision that should be made by the OpenVINO maintainers and are fine with both options 👍

ilyachur commented 1 year ago

Hmm, in general I think we can have the next pipeline:

In this case we can reuse benefits of both approaches and avoid unexpected behaviour in case if user loads custom plugins xml and at the same moment wants to use default plugins.

Moreover, for default case we can avoid generation of plugins.xml file and use pre-compiled list of plugins.

CSBVision commented 1 year ago

Alright that's perfectly fine for us. If the PR is merged, the next step is refactoring the CMake scripts. Here, our plan is extending the generation of the ie_plugins.hpp for the shared library case such that generating (or installing) of the plugins.xml file could be changed as well.

Regarding the plugin loading: We think that checking the plugins.xml first and thereafter processing all compiled-in plugins is the best solution for the use case you mentioned. The second part only has to check whether the respectively named plugin was already registered before (i.e. was defined in the xml file with extended configuration options) and add the plugin if not. Once the CMake part is extended for the shared library case, this is straightforward to implement.

ilyachur commented 1 year ago

Before the merging PR - we need to understand how it will works in all possible cases. Moreover, at the current moment PR looks like proof of concept, from my point of view PR shouldn't contain any hardcoded plugins (as it has right now). Also the PR should contain tests for the new functionality.

CSBVision commented 1 year ago

Currently the PR is a proof of concept, right, but also a first step for fixing this as a whole. Our intention is to create a relatively smooth transition. Implementing everything presumably is a bit disrupting as, on the one hand, the CMake configuration has to be extended to generate any form of configuration-dependent header file (similar to the plugins.hpp in the already existing static library case). On the other hand, the source code needs to be extended to do both, include this header file and initialize the plugin libraries accordingly. Here, our proposal is to:

  1. Implement a runtime check for a missing plugins.xml configuration file and combine this with a loading of the CPU plugin DLL as a first step. We implemented this in the current PR.
  2. Extend the CMake configuration to generate a configuration-dependent header file. We plan to address this as the next step.
  3. Refactor the fallback case in point 1. using the header file from 2. We will extend our implementation from step 1 as soon as steps 1 and 2 are done.

Each step on is own requires only relatively small modifications and that can be checked / reviewed on it's own. In particular, nothing is changed if the plugins.xml exists, as can be confirmed by an easy code review. To us, this yields a transition as smooth as possible. Currently, the library initialization simply fails if the file is missing such that at no point in this path we are loosing any form of functionality.

Optionally, further configuration options are possible. For example, remove the plugins.xml from the installation configuration. Or alternatively, both paths can be mixed: First check the plugins.xml configuration, thereafter try loading the compiled plugins, as you proposed. Of course we are open to implement these things as well. Most importantly, they can be discussed or implemented even after the three steps mentioned before are done as there could be a risk of interfering with existing functionality. Implementing these things in separate PRs allows to carefully check them all on their own.

So what are your expectations at implementing an extended logic? In theory, you can do all in one. But we think that this is much more disruptive...

CSBVision commented 1 year ago

We just updated the PR including the following changes:

Would be great if these changes align well with your expectations such that the PR could be merged.

ilyachur commented 1 year ago

@CSBVision Thank you for contribution, at the beginning of next week we will have a discussion of this proposal in order to be sure that solution will cover all existed cases. After that we will be ready to provide more details.

ilyachur commented 1 year ago

@CSBVision we had an internal discussion of this feature, results are below:

CSBVision commented 1 year ago

Thanks for your positive comments. 👍 For us, this yields to three next steps:

  1. Refactor the code to load xml-based plugins first and compiled-in, not yet loaded plugins thereafter. This is already implemented in your comment at our PR.
  2. Generate plugins.xml only if the build links again an already existing OpenVINO runtime. Our proposal is to generate the plugins.xml in either case but only copy it during the installation process (CMake's INSTALL target) accordingly. This will give the user the ability to copy it manually to the installation directory. Optionally, we could introduce a CMake flag that controls whether the plugins.xml gets installed, and initialize this flag according to the respective case. What do you think about that proposal?
  3. We are a bit unsure whether we correctly understand: Unifying dynamic and static OpenVINO in this regard means that also the static configuration checks for a plugins.xml and uses it to load additional plugins, i.e. mix statically compiled-in plugins with additional shared libraries. Is this your proposal?
ilyachur commented 1 year ago
  1. Refactor the code to load xml-based plugins first and compiled-in, not yet loaded plugins thereafter. This is already implemented in your comment at our PR.

I am not sure that it is correct for 100% because we also need to skip plugins from pre-compiled list which already were loaded from plugins.xml.

  1. Generate plugins.xml only if the build links again an already existing OpenVINO runtime. Our proposal is to generate the plugins.xml in either case but only copy it during the installation process (CMake's INSTALL target) accordingly. This will give the user the ability to copy it manually to the installation directory. Optionally, we could introduce a CMake flag that controls whether the plugins.xml gets installed, and initialize this flag according to the respective case. What do you think about that proposal?

In case if we always generate plugins.xml, how will we validate that pre-compiled list works well? The approach with pre-compiled list should become the main way and we need to validate it. Moreover developers who develop openvino and run tests on local hosts shouldn't use plugins.xml. So maybe will be better to have an option which allows to disable pre-compiled list and return back to plugins.xml but by default use only one variant. In case of usage developer config this option will be always enabled. What do you think?

  1. We are a bit unsure whether we correctly understand: Unifying dynamic and static OpenVINO in this regard means that also the static configuration checks for a plugins.xml and uses it to load additional plugins, i.e. mix statically compiled-in plugins with additional shared libraries. Is this your proposal?

Yes I meant it, that if user has plugin.xml for static build we can also load plugins from this config. Also I think that we already have a logic in cmake which generates plugins.hpp file, so the difference between static and dynamic builds is only that for static we add pointers to entry functions in the map, for dynamic we need to add names of libraries. So I suggest extend cmake code for plugins.hpp generation in order to support both cases and avoid code duplication.

CSBVision commented 1 year ago

For the moment only regarding your comment to the first point, we're coming back to the others later but maybe you could verify this in between already: In register_compile_time_plugins() we already implemented the pluginRegistry.find(deviceName) == pluginRegistry.end() check before adding it, i.e. if the plugins.xml is used to load the plugin, it will be skipped here such that the xml-based path overrules the compiled-in ones. Or did we miss anything here?

ilyachur commented 1 year ago

Yes correct, my fault. I agree that this logic should work in your PR.

CSBVision commented 1 year ago

No problem, glad it's done then. Regarding your other two comments: First, our earlier comment assumed something wrong, sorry for that. Usually, the final CMake configuration step is applying the INSTALL target to copy the actually required files. We thought that the plugins.xml would be generated before and also only gets copied during INSTALL. Still by checking the build directory, we noticed that the plugins.xml is only generated inside the install directory, not somewhere else before. Thus, we agree with you - the file should be generated if linked against an existing OpenVINO build but not otherwise by default. Still, isn't it possible that the user wants the plugin.xml for any particular reasoning? For this reasoning, our proposal is to introduce a CMake variable that controls the plugins.xml generation. It defaults to off, if OpenVINO is built completely, and to on otherwise. So the user can explicitly enable the generation. It wouldn't be that complicated either, thus we think it's a good solution - in case we didn't miss something else. What do you think?

Regarding point 3., we agree with you. The new option allows more unification between static and dynamic builds, i.e. plugins.xml allows to load additional plugins in either case. Still, it's not directly clear which refactorings are required here such that we need to analyze this in more detail.

ilyachur commented 1 year ago

For this reasoning, our proposal is to introduce a CMake variable that controls the plugins.xml generation. It defaults to off, if OpenVINO is built completely, and to on otherwise. So the user can explicitly enable the generation. It wouldn't be that complicated either, thus we think it's a good solution - in case we didn't miss something else. What do you think?

Yes, I am ok to have CMake option which enables plugins.xml generation. Moreover in the developer config we can always enable this variable with force in order to always generates plugins.xml if user is building external plugin with using the OpenVINO Developer config.

CSBVision commented 1 year ago

The unification between static and dynamic plugins should be done, actually it was simpler than expected. Both paths already registered the plugins in the same PluginDescriptor struct. We think what's left to do is to extend the CMake configuration with respect to the required generation of the plugins.xml as discussed. But please let us know in case we overlooked something else.

ilyachur commented 1 year ago

Yes, PR is good! Two moments, we need to add a CMake option to enable generation of plugins.xml and we need to disable plugins.xml generation by default

CSBVision commented 1 year ago

Yes we will take care of this, but most likely we can only add this next week - sorry for the delay.

CSBVision commented 1 year ago

Just added the respective CMake option. The only remaining part is to force this to on in case of an existing OpenVINO build, or isn't it?

ilyachur commented 1 year ago

Yes, we need to use this option for external plugins and don't generate plugins.xml for internal.

CSBVision commented 1 year ago

Alright, in which CMake scripts this has to be configured or where do the two paths (i.e. compile OpenVINO runtime or use an existing one) split? We think this is where the new option should be forced to on and that's it pretty much - or isn't it?

ilyachur commented 1 year ago

Yes it is almost done. In general I think this option can be enabled in the developer config.

CSBVision commented 1 year ago

We commited a respective update. So are we ready then or is there still something missing? Additionally, we are investigating whether the shared / static plugin loading could be more unified, however this is rather complicated and could be additionally addressed in further PRs.

ilyachur commented 1 year ago

@CSBVision At the current moment we need to get a green CI, as I see this PR has red CI right now. So we need to fix issues. As I see in some moments we have issues with plugins.xml installation, the main goal right now is to fix these issues, after that I think initial PR will be ready for the merge

ilyachur commented 1 year ago

The feature was merged to the master branch. I will close the issue. Feel free to reopen in case of any questions.