Feature: Workfile Template Builder

[Thierry-Submarine]

I've based it on how the template builder works in Houdini and kept mostly the same logic

Few pointers:

• Placeholder Node uses a BurnIn node, I choose this one because it has both input and output support. After the actual Load has done the links (PEG, Composite) and position in the node view are copied from the placeholder node. Also BurnIn had a good attribute to store data, as this needs to be on the node as scenedata gets lost when using Harmony Templates as an intermediate to build your workfile template.
• Template path needs to be pointed to an unzipped .tpl folder, in my testing I grouped my nodes in the nodeview, made a harmony template from the group, opened the template, ungrouped and saved. This way the hierarchy is flat and as you would expect when you work on setting up your template

update: draft removed, tested and linting fixed

[Sasbom]

Some review notes here while testing this in production that I thought would be helpful for general knowledge:

At the moment, this PR is broken testing with ayon-core 1.9.8, and the underlying issue seems to still persist in 1.9.10 (version checked just now).
It's a structural issue, the initialization of the "Create Placeholder" window fails.
This is due to the way creators are set up, inheriting from a chain of internally defined creators that are ill-formed, creating dictionary entries of <property at (adress)> : <property at (adress)> when the following function in core is ran:

https://github.com/ynput/ayon-core/blob/d056624d66cc07aea75c616b98afac31d49db75a/client/ayon_core/pipeline/workfile/workfile_template_builder.py#L375-L382

    def _collect_creators(self):
        self._creators_by_name = {
            identifier: creator
            for identifier, creator
            in self.create_context.manual_creators.items()
            # Do not list HiddenCreator even though it is a 'manual creator'
            if not isinstance(creator, HiddenCreator)
        }

It then gets sorted with a lambda, which fails to sort because it can't sort on a property type, because it expects all entries to be strings:

https://github.com/ynput/ayon-core/blob/d056624d66cc07aea75c616b98afac31d49db75a/client/ayon_core/pipeline/workfile/workfile_template_builder.py#L2191-L2199

        creators_by_name = self.builder.get_creators_by_name()

        creator_items = [
            (creator_name, creator.label or creator_name)
            for creator_name, creator in creators_by_name.items()
        ]
        # Errors on sort! Expects all keys to be strings.
        creator_items.sort(key=lambda i: i[1])
        options = options or {}

There is no validation that's ran other than checking for inheritance of "HiddenCreator" who's only characteristic is that HiddenCreator::skip_discovery is set to True by default.

https://github.com/ynput/ayon-core/blob/d056624d66cc07aea75c616b98afac31d49db75a/client/ayon_core/pipeline/create/creator_plugins.py#L1100-L1109

class HiddenCreator(BaseCreator):
     # Maybe respect this class attribute over inheritance
    skip_discovery = True

    @abstractmethod
    def create(
        self,
        instance_data: dict[str, Any],
        source_data: dict[str, Any],
    ) -> Any:
        pass

I know that automatic creators (that also have the skip_discovery = True) are supposed to get picked up. I think that relying on inheritance of some HiddenCreator when there's plenty of plugins that have similar Creator setups to this implementation is problematic. (e.g. the client/ayon_(software)/api/plugin.py having some DCC specific chain of inherited creator setups that then set up a baseclass from which all creator plugins derive from, in this instance HarmonyCreator)

Switching to inheriting from HiddenCreator would mean that everything down the line doesn't get picked up, and i don't think that hacking our way through with metaclasses and __instancecheck__ / __subclasscheck__ would make any sense.

I'd prefer it if we can simply set a flag and have it be respected, like the existing skip_discovery or maybe a skip_template_build (which could be for loaders and creators both). For our own use, I patched ayon-core locally by just filtering out the resulting creator_items dict by doing an isinstance(key, str) check on the keys. I don't think that's a neat solution so I'm not making a PR for that, but I wanted to adress this since this is a structural issue across potentially many other integrations that could benefit from having a template builder.

Kind regards,

Sas

[BigRoy]

@iLLiCiTiT can you check latest comment ☝️

[iLLiCiTiT]

This is due to the way creators are set up, inheriting from a chain of internally defined creators that are ill-formed, creating dictionary entries of <property at (adress)> : <property at (adress)> when the following function in core is ran:

Not sure I fully understand, but it sounds like a poor definition of what identifier is on create plugin. It is meant to be abstract class string attribute. It is marked as abstract property which should be changed, but I don't think python does support definition of abstract class property -> agree it is confusin, don't know how to fix it, we can validate if it is not class attribute and skip them during discovery, or move the unique validations after initialization.

There is no validation that's ran other than checking for inheritance of "HiddenCreator" who's only characteristic is that HiddenCreator::skip_discovery is set to True by default.

I don't understand what do you mean by this? What validation? If you're asking about skip_discovery then the attribute has to be set on the class that should be skipped, any inherited classes won't "inherit" that value.

BTW the if not isinstance(creator, HiddenCreator) doesn't make sense, they will never be added to the manual creators.

[Sasbom]

Not sure I fully understand, but it sounds like a poor definition of what identifier is on create plugin. It is meant to be abstract class string attribute. It is marked as abstract property which should be changed.

Yes, but oftentimes, implementations (such as this one, in harmony), set up their creators in a modular fashion, not completely implemented. They shouldn't have an identifier because they are merely there to set up some shared behavior. I'll go fetch an example, that most creator plugins for ayon_harmony inherit from.

https://github.com/ynput/ayon-harmony/blob/develop/client/ayon_harmony/api/plugin.py#L87-L202

class HarmonyCreator(Creator, HarmonyCreatorBase):
    """Creator plugin to create instances in Harmony.

    By default a Composite node is created to support any number of nodes in
    an instance, but any node type is supported.
    If the selection is used, the selected nodes will be connected to the
    created node.
    """

    settings_category = "harmony"

    def create(self, product_name, instance_data, pre_create_data):
        product_type = instance_data.get("productType")
        if not product_type:
            pt_items = self.get_product_type_items()
            if pt_items:
                product_type = pt_items[0].product_type
            else:
                product_type = self.product_base_type
            instance_data["productType"] = product_type

        # Create the node
        node = self.product_impl(product_name, instance_data, pre_create_data)

# ... omitted the rest for the sake of length

This is a creator that never should be used in isolation, it is meant to be derived from.
The create method even provides a hook to HarmonyCreator::product_impl where the actual creation logic can be set up,
but by default it raises a NotImplementedError.

def product_impl(self, name, instance_data: dict, pre_create_data: dict):
    raise NotImplementedError

Despite all this, it's still collected, even though it has no identifier defined on the class, making it ill-formed, leading to errors.

I don't understand what do you mean by this? What validation?

With validation, I mean that there's no "check" that happens to see if the identifier is there, or if the plugin is even supposed to be picked up. The way the collection for the workfile template builder seems to work, it collects plugins from every source, and it doesnt "verify" them, only if they subclass from HiddenCreator do they not get picked up (as shown in my previous comment.

I hope this clears things up a bit.

Thanks,
Sas

[iLLiCiTiT]

Despite all this, it's still collected, even though it has no identifier defined on the class, making it ill-formed, leading to errors.

Oh I see, that's because the identifier by default uses product base type. But it means that is does define also product_type, which is abstract (will be replaced by product_base_type with this PR).

With validation, I mean that there's no "check" that happens to see if the identifier is there, or if the plugin is even supposed to be picked up. The way the collection for the workfile template builder seems to work, it collects plugins from every source, and it doesnt "verify" them, only if they subclass from HiddenCreator do they not get picked up (as shown in my previous comment.

Well, the product_impl should be marked as abstract, or use skip_discovery.

If you have base class that does implement all the abstract methods, then it is not really something we should handle, validating if abstract implementation actually does implement what the abstraction defines is double checking of the same thing.

[Sasbom]

Well, the product_impl should be marked as abstract, or use skip_discovery.

If you have base class that does implement all the abstract methods, then it is not really something we should handle, validating if abstract implementation actually does implement what the abstraction defines is double checking of the same thing.

I see...
I overlooked exactly where skip_discovery was respected, but I found it.
If I have extra time I'll look into making a PR that modernizes the class definitions in ayon_harmony/api/plugin.py.

Thanks,

Sas

[iLLiCiTiT]

Made a PR #148

[kalisp]

• Template path needs to be pointed to an unzipped .tpl folder,

I dont understand this. Published harmony.template is a zip file, I cannot load that directly? Or does that mean 'workfile template'?

I opened empty workfile in Harmony added Placeholder, saved it as new version of Workfile and used that as a template path. I cannot do that? (I tried to open new context without any workfile, but it didn't create first workfile from that path automatically. But it might be that because it was .zip)

I also tried to Build Workfile Template in that opened 'workfile template' with single Placeholder, but it failed with:

>>> [  Found template at: 'c:/projects/test_project_1/sq01/sh02/work/workfile_builder/tp11_sh02_workfile_builder_v002.zip'  ]
>>> [  Building the workfile template: c:/projects/test_project_1/sq01/sh02/work/workfile_builder/tp11_sh02_workfile_builder_v002.zip  ]
DEBUG:ayon_harmony.api.server:[16:42:55.929040] Sending [3]:
    b'AH\x00\x00\x01\xc2{"function": "function ff9dc3992_f157_4316_9a44_7adb0de1e7eb_paste(args)\\n        {\\n            var template_path = args[0];\\n            var drag_object = copyPaste.pasteTemplateIntoGroup(\\n                template_path, \\"Top\\", 1\\n            );\\n        }\\n        ff9dc3992_f157_4316_9a44_7adb0de1e7eb_paste\\n        ", "args": ["c:/projects/test_project_1/sq01/sh02/work/workfile_builder/tp11_sh02_workfile_builder_v002.zip"], "message_id": 3}'
DEBUG:ayon_harmony.api.server:--- Message length: 450
DEBUG:ayon_harmony.api.server:data:: b'{"function":"function ff9dc3992_f157_4316_9a44_7adb0de1e7eb_paste(args)\\n        {\\n            var template_path = args[0];\\n            var drag_object = copyPaste.pasteTemplateIntoGroup(\\n                template_path, \\"Top\\", 1\\n            );\\n        }\\n        ff9dc3992_f157_4316_9a44_7adb0de1e7eb_paste\\n        ","args":["c:/projects/test_project_1/sq01/sh02/work/workfile_builder/tp11_sh02_workfile_builder_v002.zip"],"message_id":3,"reply":true}' <class 'bytes'>
DEBUG:ayon_harmony.api.server:[16:42:55.932546] Received:
    {"function":"function ff9dc3992_f157_4316_9a44_7adb0de1e7eb_paste(args)\n        {\n            var template_path = args[0];\n            var drag_object = copyPaste.pasteTemplateIntoGroup(\n                template_path, \"Top\", 1\n            );\n        }\n        ff9dc3992_f157_4316_9a44_7adb0de1e7eb_paste\n        ","args":["c:/projects/test_project_1/sq01/sh02/work/workfile_builder/tp11_sh02_workfile_builder_v002.zip"],"message_id":3,"reply":true}
DEBUG:ayon_harmony.api.server:--- storing request as 3
DEBUG:ayon_harmony.api.server:[16:42:55.933551] received data was just a reply.
DEBUG:ayon_harmony.api.server:[16:42:56.029987] Got request id 4, removing from queue
WARNING:ayon_core.lib.python_module_tools:Failed to load path: "C:\Users\ynput\AppData\Local\Ynput\AYON\addons\harmony_0.5.3+dev\ayon_harmony\plugins\workfile_build\create_placeholder.py"
Traceback (most recent call last):
  File "C:\Users\ynput\AppData\Local\Ynput\AYON\addons\core_1.9.8+dev\ayon_core\lib\python_module_tools.py", line 93, in modules_from_path
    module = import_filepath(full_path, mod_name)
  File "C:\Users\ynput\AppData\Local\Ynput\AYON\addons\core_1.9.8+dev\ayon_core\lib\python_module_tools.py", line 45, in import_filepath
    module_loader.exec_module(module)
  File "<frozen importlib._bootstrap_external>", line 850, in exec_module
  File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
  File "C:\Users\ynput\AppData\Local\Ynput\AYON\addons\harmony_0.5.3+dev\ayon_harmony\plugins\workfile_build\create_placeholder.py", line 12, in <module>
    class HarmonyPlaceholderCreatePlugin(
  File "C:\Users\ynput\AppData\Local\Ynput\AYON\addons\harmony_0.5.3+dev\ayon_harmony\plugins\workfile_build\create_placeholder.py", line 30, in HarmonyPlaceholderCreatePlugin
    def get_placeholder_options(self, options: dict | None = None) -> list:
TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
>>> [ *** Discovered 1 abstract plugins
- HarmonyPlaceholderPlugin
*** FAILED to load 1 files
- 

I located published harmony.template in current folder, there is only single one, I loaded it via Loader manually OK.