Create & Publish: Attribute definitions per instance #860

iLLiCiTiT · 2024-08-30T15:51:14Z

Changelog Description

Create plugins and publish plugins can define attribute definitions per instance. Publish context plugins can also define attributes for instances and instance plugins can define attributes for context.

Additional info

THIS IS NOT FINAL PR

This PR is primarily created to review my current state of work, it might not be merged at the end. Following PR should add option to change instance attributes after creation using callbacks, which might mean that some of the methods will probably change to suit the usecase, which would again mean breaking backwards compatibility.

Testing notes:

Existing plugins should work exactly the same as before this PR.
With changes in this PR it is possible to define different attribute definitions per instance, for create plugin and for publish plugin.
One example could be that an instance have enumerator but with different values, e.g. based on scene location.

I've had some testing scenario, but I'm certain I don't have what might be useful.

…with-Publisher-attributes

…nstance

ynbot · 2024-08-30T15:51:19Z

Task linked: AY-5552 Publisher attr defs per instance

…er-instance

BigRoy · 2024-09-03T12:27:32Z

client/ayon_core/pipeline/create/creator_plugins.py

+    def _create_instance(
+        self, product_type: str, product_name: str, data: Dict[str, Any]
+    ) -> CreatedInstance:
+        instance = CreatedInstance(
+            product_type,
+            product_name,
+            data,
+            creator=self,
+        )
+        self._add_instance_to_context(instance)
+        return instance


Why is this needed?

Just helper method for creators. Not needed, but you don't have to import CreatedInstance and call _add_instance_to_context manually to create one.

client/ayon_core/pipeline/publish/publish_plugins.py

BigRoy · 2024-09-03T12:40:14Z

client/ayon_core/pipeline/create/creator_plugins.py

+    def get_attr_defs_for_instance(self, instance):
+        """Get attribute definitions for an instance.
+
+        Args:
+            instance (CreatedInstance): Instance for which to get
+                attribute definitions.
+
+        """
+        return self.get_instance_attr_defs()


So - how dynamic is this? Like, from the CreatedInstance to what data in it are we allowed to respond to make the returned attribute definitions unique? E.g. technically we can now differentiate attribute defs based on "other attributes" on the instance - however I wonder whether that's intended 'feature'?

Can we rely on doing something different based on e.g. folder path or task? If so, what happens if someone without resets confirms a target folder path change from within the UI? Does it refresh?

Can we rely on doing something differently based on e.g. families on the instance if it has it?

Can we rely on doing something based off of other attributes on the instance from other plug-ins, I suppose not? Since it'd only update on reset?

Can we rely on doing something differently based on e.g. families on the instance if it has it?

Yes, but if I may request, please do not use families for it if possible. We can discuss each individual use-case (not here).

Why: In case of USD, you probably should be adding families based on the definitions, instead of showing definitions based on families? Meaning, you can add "isUsdInstance": True on instance data to know if is related to USD or not (please take this as very very basic example).

Can we rely on doing something different based on e.g. folder path or task? If so, what happens if someone without resets confirms a target folder path change from within the UI? Does it refresh?

Can we rely on doing something based off of other attributes on the instance from other plug-ins, I suppose not? Since it'd only update on reset?

Both question have same answer. Right now it collects definitions only when instance is created or collected (on create and on reset). For "real time update" we need callbacks. Future PR mentioned in description, which I think might require change of the methods or create context api.

Why: In case of USD, you probably should be adding families based on the definitions, instead of showing definitions based on families? Meaning, you can add "isUsdInstance": True on instance data to know if is related to USD or not (please take this as very very basic example).

I see, but preferably we work towards something similar to families (like standardizes like that, alsmost like "tags") but for the instance's features instead instead of resulting in TONS and TONS of isUsdInstance=True and isFbxInstance=True. Stepping away from pyblish terminology like families, yes! But just doing for example targetProductTypes = {"fbx", "usd"} or whatever name we come up seems like something we can standardize way better. Or maybe better yet, have a dedicated attribute for that on the instance. E.g. being able to do something like:

if instance.supports("usd"): # bla

Likely overlaps tons with @antirotor work with OpenAssetIO but that's a can of worms I'm afraid to touch. :)

Both question have same answer. Right now it collects definitions only when instance is created or collected (on create and on reset). For "real time update" we need callbacks. Future PR mentioned in description, which I think might require change of the methods or create context api.

👍

Because I don't know the context, I can't react. Only from what you wrote I have 10 following questions that will raise another questions. Maybe with full context I would agree, but at this moment I don't.

It is possible to do anything, we should just discuss what would be the ideal approach, to make it clear from data flow.

Like I've said (wrote) we can discuss that out of this PR, as it is not directly related, and it requires more minds.

BigRoy · 2024-09-05T23:33:16Z

So I have some use cases for this that I'd like to implement soon - as a means to testing this PR, but there's one big worry that I have: performance. @iLLiCiTiT

Let me explain the use cases:

For Maya instances (and a client ticket) I'd like to set the default frame range for an instance to the target folder entity or task entity's frame range instead of the scene's frame range. As such, I'd now need to query the folder/task entity for each of the instances, in this method. Which would result in many queries, per instance. Is there some sort of shared cache I could abuse? Or for this case, should the instances provide access to the folder and task entities on the CreatedInstance itself?
For Maya Review instances I'd like to set the review resolution width/height to a default, also based off the target folder or task.
For Deadline instances I'd like to collect the pools to make them enums instead of raw string attributes and the same goes for "machine list" where it can be an enum of the available Workers, etc. - this would need to be based off of the "deadline url" set on the instance because I believe that can be defined per instance -- although I have no idea what defines that URL?

Having the defaults update related to the instance for attributes may also make #153 and #146 more sensible to implement.
What are your thoughts?

BigRoy · 2024-09-05T23:48:39Z

Calling instance.data_to_store() on the CreatedInstance in get_attr_defs_for_instance() raises:

//   File "E:\dev\ayon-core\client\ayon_core\pipeline\create\context.py", line 1037, in reset_instances
//     creator.collect_instances()
//   File "E:\dev\ayon-maya\client\ayon_maya\api\plugin.py", line 293, in collect_instances
//     return self._default_collect_instances()
//   File "E:\dev\ayon-maya\client\ayon_maya\api\plugin.py", line 234, in _default_collect_instances
//     created_instance = CreatedInstance.from_existing(node_data, self)
//   File "E:\dev\ayon-core\client\ayon_core\pipeline\create\structures.py", line 716, in from_existing
//     return cls(
//   File "E:\dev\ayon-core\client\ayon_core\pipeline\create\structures.py", line 479, in __init__
//     creator_attr_defs = creator.get_attr_defs_for_instance(self)
//   File "E:\dev\ayon-maya\client\ayon_maya\plugins\create\create_review.py", line 100, in get_attr_defs_for_instance
//     print(instance.data_to_store())
//   File "E:\dev\ayon-core\client\ayon_core\pipeline\create\structures.py", line 689, in data_to_store
//     output["creator_attributes"] = self.creator_attributes.data_to_store()
//   File "E:\dev\ayon-core\client\ayon_core\pipeline\create\structures.py", line 654, in creator_attributes
//     return self._data["creator_attributes"]

If I try to get the folder path or task name for the instance, I can't find it?

print(instance.items())
# odict_items([('id', 'pyblish.avalon.instance'), ('productType', 'review'), ('productName', 'reviewMain'), ('active', True), ('creator_identifier', 'io.openpype.creators.maya.review'), ('variant', '')])

Also, variant is an empty value?

…er-instance

…er-instance # Conflicts: # client/ayon_core/pipeline/create/structures.py

iLLiCiTiT · 2024-09-16T17:04:47Z

Calling instance.data_to_store() on the CreatedInstance in get_attr_defs_for_instance() raises:

Should be fixed, but I would recommend to not use data_to_store there, rather get only values you need...

…er-instance

MustafaJafar

Hey,
I tested this PR against ynput/ayon-houdini#2
I applied the following modification to these lines: renamed the method + add the if condition.

    def get_attr_defs_for_instance(self, instance):
        """get instance attribute definitions.

        Attributes defined in this method are exposed in
            publish tab in the publisher UI.
        """

        if instance["productType"] != "render":
            return []

and it I was able to change the attr defs per instances.
My code skips adding review and render target parameters for non render products.

non render product	render product

tbh, I find it hard to furtherly test this PR.

…er-instance

iLLiCiTiT · 2024-09-25T08:06:40Z

I find it hard to furtherly test this PR.

I don't know if there is anything else to test?

…er-instance

MustafaJafar · 2024-10-03T09:11:08Z

I find it hard to furtherly test this PR.

I don't know if there is anything else to test?

I meant it's hard for me to test it with other creator plugins. maybe because I'm not yet familiar with the enhancement in this PR.

iLLiCiTiT · 2024-10-03T12:23:07Z

Closing in favor of #935

iLLiCiTiT added 5 commits August 19, 2024 18:29

don't store 'attr_plugins'

d1bae6d

Merge branch 'develop' into enhancement/AY-2420_Callbacks-and-groups-…

a1ca6a2

…with-Publisher-attributes

added 2 new methods to be able to receive attribute definitions per i…

c337f2c

…nstance

added helper methods to create plugin

738cc82

attribute definitions are defined per instance

cb3df92

iLLiCiTiT requested a review from BigRoy August 30, 2024 15:51

ynbot added size/S type: enhancement Improvement of existing functionality or minor addition labels Aug 30, 2024

iLLiCiTiT requested review from kalisp and antirotor August 30, 2024 15:53

iLLiCiTiT added 2 commits August 30, 2024 18:31

Merge branch 'develop' into enhancement/AY-5552_Publisher-attr-defs-p…

e457580

…er-instance

removed unused imports

35d7277

BigRoy reviewed Sep 3, 2024

View reviewed changes

remove line

d970a1c

BigRoy and others added 4 commits September 6, 2024 01:48

Merge branch 'develop' into enhancement/AY-5552_Publisher-attr-defs-p…

f526245

…er-instance

use 'get_attr_defs_for_instance' after creation

f03983d

implemented 'update_create_attr_defs'

bbd3881

data to store does not have to have 'AttributeValues'

b3f39ba

iLLiCiTiT mentioned this pull request Sep 9, 2024

Add profiles to 'CollectFramesFixDef' settings #839

Closed

Merge branch 'develop' into enhancement/AY-5552_Publisher-attr-defs-p…

0770a26

…er-instance

MustafaJafar self-requested a review September 9, 2024 08:38

Merge branch 'develop' into enhancement/AY-5552_Publisher-attr-defs-p…

cf41ab6

…er-instance # Conflicts: # client/ayon_core/pipeline/create/structures.py

iLLiCiTiT added 2 commits September 16, 2024 19:05

Merge branch 'develop' into enhancement/AY-5552_Publisher-attr-defs-p…

ee4ecf1

…er-instance

modified signature of '_create_instance'

688c253

BigRoy mentioned this pull request Sep 20, 2024

Houdini: Publish any ROP node (Generic Creator) ynput/ayon-houdini#2

Open

13 tasks

MustafaJafar reviewed Sep 24, 2024

View reviewed changes

Merge branch 'develop' into enhancement/AY-5552_Publisher-attr-defs-p…

dbb427f

…er-instance

mkolar assigned iLLiCiTiT Sep 30, 2024

Merge branch 'develop' into enhancement/AY-5552_Publisher-attr-defs-p…

7427a07

…er-instance

antirotor mentioned this pull request Oct 1, 2024

Better communication between publisher and rops ynput/ayon-houdini#16

Open

2 tasks

iLLiCiTiT closed this Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create & Publish: Attribute definitions per instance #860

Create & Publish: Attribute definitions per instance #860

iLLiCiTiT commented Aug 30, 2024 •

edited

Loading

ynbot commented Aug 30, 2024

BigRoy Sep 3, 2024

iLLiCiTiT Sep 3, 2024 •

edited

Loading

BigRoy Sep 3, 2024

iLLiCiTiT Sep 3, 2024 •

edited

Loading

BigRoy Sep 3, 2024 •

edited

Loading

iLLiCiTiT Sep 3, 2024 •

edited

Loading

BigRoy commented Sep 5, 2024 •

edited

Loading

BigRoy commented Sep 5, 2024

iLLiCiTiT commented Sep 16, 2024

MustafaJafar left a comment •

edited

Loading

iLLiCiTiT commented Sep 25, 2024

MustafaJafar commented Oct 3, 2024

iLLiCiTiT commented Oct 3, 2024

Create & Publish: Attribute definitions per instance #860

Create & Publish: Attribute definitions per instance #860

Conversation

iLLiCiTiT commented Aug 30, 2024 • edited Loading

Changelog Description

Additional info

Testing notes:

ynbot commented Aug 30, 2024

BigRoy Sep 3, 2024

Choose a reason for hiding this comment

iLLiCiTiT Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

BigRoy Sep 3, 2024

Choose a reason for hiding this comment

iLLiCiTiT Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

BigRoy Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

iLLiCiTiT Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

BigRoy commented Sep 5, 2024 • edited Loading

BigRoy commented Sep 5, 2024

iLLiCiTiT commented Sep 16, 2024

MustafaJafar left a comment • edited Loading

Choose a reason for hiding this comment

iLLiCiTiT commented Sep 25, 2024

MustafaJafar commented Oct 3, 2024

iLLiCiTiT commented Oct 3, 2024

iLLiCiTiT commented Aug 30, 2024 •

edited

Loading

iLLiCiTiT Sep 3, 2024 •

edited

Loading

iLLiCiTiT Sep 3, 2024 •

edited

Loading

BigRoy Sep 3, 2024 •

edited

Loading

iLLiCiTiT Sep 3, 2024 •

edited

Loading

BigRoy commented Sep 5, 2024 •

edited

Loading

MustafaJafar left a comment •

edited

Loading