General: Move plugins register and discover

[iLLiCiTiT]

Brief description

Move plugins register and discover logic from avalon-core repository.

Description

Discovery and register of plugins was moved to openpype/pipeline/plugin_discover.py and is used at all places where register and discover plugins is used. Added register and discover function into create plugins.

Changes

• implemented PluginDiscoverContext - it's object can be used to register and discover plugins
  • added object of PluginDiscoverContext for global access
  • discover method returns DiscoverResult which holds all discovered, crashed, ignored, duplicated and abstract plugins
• added register/deregister and discover functions into create plugins
• discover of legacy creators and loader will trigger set_plugin_attributes_from_settings
  • this was added to discover_loader_plugins and discover_legacy_creator_plugins
• function classes_from_module does not check inheritance by class names

Testing notes:

• pre/post launch hooks must work as did
  • affected by change of dynamic import functions
• all plugins (create, load, inventory, launcher action, thumbnail) must be discovered properly

Related to PR ynput/avalon-core#436

[BigRoy]

Some notes from my end - I feel we're once again increasing complexity where we don't need it. Making it much harder to maintain the code over time and have anyone go through the code and grasp what exactly is going on.

To me personally it makes just as much sense if python_module_tools and plugins_discover would be in a single file in openpype/pipeline/plugins.py. At its core that defines the complete "plug-in system".

I feel the code changes make it much less readable as opposed to how simple the code was in Avalon with no new features being implemented by the changes. Additionally the new classes (e.g. GlobalDiscover acting as a singleton) provide no docstrings or documentation and feel less python than just storing the registered paths directly. I mean - if we just compare this with what's being removed from Avalon we're doubling the size of the code. Whilst at the same time losing functionality which allows us to explicitly register a plug-in (not stored in a path but from memory) which it did previously allow - which is functionality that is very useful e.g. for running tests.

Also having discover not just return the plug-ins but a "DiscoverResult" just so we can print a report and list "duplicate" plug-ins feels like redundant complexity. I might be missing the use case as to why we need this? The same report could be generated during by the discover function itself without needing to have the intermediate class instance. Where are we using the duplicate plug-ins or those that crashed? Or where are we expecting to use those in our favor?

I'm additionally worried that I might need to alter any scripts/tools I have in place that actually run discover(). Refactoring is one thing but also expecting different output from the functions does bring the refactor to quite another level.

Having things be "simple yet effective" is much better, no? We should only make things more complex if we absolutely need to. A pipeline is complex enough by itself.

[ynbot]

Task linked: OP-2855 move plugins register and discover

[iLLiCiTiT]

I feel we're once again increasing complexity where we don't need it. Making it much harder to maintain the code over time and have anyone go through the code and grasp what exactly is going o

Current avalon's approach is based on singleton access. It is NOT possible to have registered multiple paths for different contexts which would be now possible. It's not used that way yet but until it's possible it will never be.

To me personally it makes just as much sense if python_module_tools and plugins_discover would be in a single file in openpype/pipeline/plugins.py.

Content of python_module_tools is usabled (and used) at many different places. It is more like a python module adding some functionality that can be used elsewhere. Not just in discovery of pipeline plugins.

Also I've kept the plugins in function names to keep backwards compatibility but the register and discovery is not just for plugins but also for actions and thumbnail resolvers.

Additionally the new classes (e.g. GlobalDiscover acting as a singleton) provide no docstrings or documentation and feel less python than just storing the registered paths directly.

Will add a docstring. All it does is that it gives ability to use it the same way as was in avalon.

Also having discover not just return the plug-ins but a "DiscoverResult" just so we can print a report and list "duplicate" plug-ins feels like redundant complexity.

Because it's not used yet doesn't mean it will not be used. Many many times we wanted to show in UI some report that something went wrong and give artist ability to send it to us. That is few steps away so current usage just print it out. I can modify the logic that it will not return the result object but plugins by default and will return the result only when requested.

I'm additionally worried that I might need to alter any scripts/tools I have in place that actually run discover().

We want to add ability to have a context wrapper around the actions and plugins and add them some initialization to add more abilities we're lacking right now. Again not used that way now but it must be prepared to be able do it that way so the PRs are reviewable.
One example is current applying of settings on loaders and creators. That's something what should be done during initialization and both of them should be usable to load/create multiple times using the same initialized object but it is not possible right now.

Having things be "simple yet effective" is much better, no? We should only make things more complex if we absolutely need to. A pipeline is complex enough by itself.

It's getting more and more complicated to have it "simple". We're hacking a lot because it is done that way and we will never stop hack it until we make small steps heading to make it usable without hacks. It looks simple when you see just those functions but it is much more complicated when we're bending it to make it work as we need. Also there is so much we can't do because of that...

For example easily switch Qt UI for any other UI (detached from the host process).

[BigRoy]

It's not used that way yet but until it's possible it will never be.

I do feel that might be the wrong approach. We should implement the features that we need - not expose functionality that we might never be used. It's much more likely that 'errors' will go by unnoticed.

E.g. likely it won't work with the current implementation either since it still relies on having the global singleton everywhere? In essence that part doesn't behave much different than before?

Content of python_module_tools is usabled (and used) at many different places. It is more like a python module adding some functionality that can be used elsewhere. Not just in discovery of pipeline plugins.

Agreed that it's relatively generic - but I have the same approach as the above. We might want to only break out the things that we know will be used. Having it separated or exposing it as some sort of "API" or 'use it wherever you see fit' means we should be much less inclined to ever changing the logic/results of the functions in the future. Since we don't know what users/teams/studios might have been using it for elsewhere.

Currently it isn't actually used anywhere else. I'm fine with having it separated. It just makes it appear bigger than I felt it needs to be.

Because it's not used yet doesn't mean it will not be used. Many many times we wanted to show in UI some report that something went wrong and give artist ability to send it to us. That is few steps away so current usage just print it out. I can modify the logic that it will not return the result object but plugins by default and will return the result only when requested.

I'd much prefer actually to have that feature added - so we know it actually works the way we intend it to work. Additionally we should implement tests that the feature keeps working as we ever intended it to be.

---

In short, it'd help a lot if the decision making process about making these things more complicated is much clearer. Currently I can't accurately and actively feedback the code as to decide whether it does what it should which means I could only say "this errors".

Anyway, thanks for the thorough input.

[BigRoy]

Shouldn't the printed report either be logged instead? Or put behind a verbosity flag?

[iLLiCiTiT]

I do feel that might be the wrong approach. We should implement the features that we need - not expose functionality that we might never be used. It's much more likely that 'errors' will go by unnoticed.

It is "a piece of logic" which contains some functions and attributes that do some logic. All I did is to put them in class so it is visible what are the logic boundries and what functions and attributes that logic has. Which might never be used as I described but it is at least visible what is affecting what. Also having it is class gives ability to create different parts of the logic instead of having it only as singleton (that's why AvalonMongoDB was created BTW).

I'd much prefer actually to have that feature added - so we know it actually works the way we intend it to work.

We just can't do everything in one run. We have limited time to do features. When we have one simple feature to do but the feature requires to rewrite the core function used everywhere it will never happen because it will take too much time and too much risk. Preparing it slowly is the only way I can see we could do it. At this moment we have ability to prepare it as much close to what we need as possible.

Currently it isn't actually used anywhere else. I'm fine with having it separated. It just makes it appear bigger than I felt it needs to be.

The functions were there before this PR. It is used in openpype modules, application and getting of openpype version.

One of refactor steps will be to remove all pipeline specific code from openpype/lib to openpype/pipeline. Main reason is that most of them are needed only for pipeline specific cases but not for application as it is. OpenPype is not just a pipeline anymore but an application that contain pipeline. When we need just handle application version or connection to server, we should not touch the pipeline part at all because it has high ability of application failure.

Currently I can't accurately and actively feedback the code as to decide whether it does what it should which means I could only say "this errors".

Currently the code does exactly the same like did in avalon. Only the core logic was wrapped into class and some hacks were removed from places where were not needed.

[antirotor]

It is failing in Houdini 19.0.498:

Traceback (most recent call last):
  File "E:\projects\openpype\3x\openpype\tools\loader\app.py", line 307, in _assetschanged
    self._thumbnail_widget.set_thumbnail(asset_ids)
  File "E:\projects\openpype\3x\openpype\tools\loader\widgets.py", line 871, in set_thumbnail
    thumbnail_ent, "thumbnail", self.dbcon
  File "E:\projects\openpype\3x\openpype\pipeline\thumbnail.py", line 17, in get_thumbnail_binary
    resolvers = discover_thumbnail_resolvers()
  File "E:\projects\openpype\3x\openpype\pipeline\thumbnail.py", line 134, in discover_thumbnail_resolvers
    return discover(ThumbnailResolver).plugins
AttributeError: 'list' object has no attribute 'plugins'

[antirotor]

same problem in Blender 3.0

Traceback (most recent call last):
  File "E:\projects\openpype\3x\openpype\tools\loader\app.py", line 307, in _assetschanged
    self._thumbnail_widget.set_thumbnail(asset_ids)
  File "E:\projects\openpype\3x\openpype\tools\loader\widgets.py", line 870, in set_thumbnail
    thumbnail_bin = get_thumbnail_binary(
  File "E:\projects\openpype\3x\openpype\pipeline\thumbnail.py", line 17, in get_thumbnail_binary
    resolvers = discover_thumbnail_resolvers()
  File "E:\projects\openpype\3x\openpype\pipeline\thumbnail.py", line 134, in discover_thumbnail_resolvers
    return discover(ThumbnailResolver).plugins
AttributeError: 'list' object has no attribute 'plugins'

[iLLiCiTiT]

It is failing in Houdini 19.0.498:

Should be fixed now

[antirotor]

works now, thanks!