Implement ValidateFolderCreationResolution.

[rdelillo]

Changelog Description

resolve: #1289

Implement a new validator that check existing shot resolution instead of blindly update.

If a mismatch is detected, 2 repair actions are suggested:

• Force update to new resolution
• Ignore new resolution and leave as-is

Testing notes:

1. Try to re-create an existing shot with this option enabled (Resolve, Hiero, Flame or traypublisher)
   2a. Ensure disabling the validator let the shot product to go through with no resolution changes
   2b. Ensure enabling the validator does nothing if the resolution match
   2c. Ensure the validator raises if the resolution mismatches with 2 suggested repairs:
   • force resolution update
   • ignore, which goes back to 2a

[ynbot]

Task linked: AY-7197 Override resolution on editorial publish

[BigRoy]

A validator should usually not change the data - it's really about validating, not by adjusting how the rest of publishing behaves. @iLLiCiTiT any pointers?

[rdelillo]

To explain a bit more here. extract_hierarchy_to_ayon is blindly updating any folder attributes from the data provided by the instance. See https://github.com/ynput/ayon-core/blob/develop/client/ayon_core/plugins/publish/extract_hierarchy_to_ayon.py#L128

Most of our editorial publishers creating shots are forcing resolution attributes in different ways (from DCC timeline depending or the source, from the collect_explicit_resolution.py, or from CSV/EDL entry...).

So far, the most elegant way I found to prevent a resolution update was to simply get rid of the values here and keep the logic self-scoped. Do you guys think of anything better ?

[BigRoy]

The data change should technically need to occur during collecting stage as far as I know, but it can definitely appear to complicate code for these use cases. Will leave it to @antirotor @iLLiCiTiT @philippe-ynput to state how we should approach this case.

[rdelillo]

Maybe some concret examples to help understand how various settings/attributes/input might interfer the shot resolution present in hierarchyContext data as I'd need some guidance if we want to streamline that.

• Hiero publisher:

  • shot resolution computed in ayon-hiero/collect_shots plugin
    • either from timeline if sourceResolution creator attribute is disabled
    • or read from plate source if sourceResolution creator attribute is enabled
  • shot resolution then potentially overriden by ayon_core/collect_explicit_resolution if configured via settings
  • shot resolution is set in hierarchyContext via ayon-core/collect_hierarchy plugin

• CSV ingest (Traypublisher)

  • shot resolution is read from input CSV file if exists
  • shot resolution then potentially overriden by ayon_core/collect_explicit_resolution if configured via settings
  • shot resolution is reported to hierarchyContext via ayon-traypublisher/collect_shot_instances

[iLLiCiTiT]

I'm sorry but I'm lost in this PR. Is the goal to not update attributes on folders based on checkbox, and have option to validate the attribute values (based on the same checkbox)?

If that's the case then the checkbox value should change behavior of the extractor to not override the attributes, and the same value should be used in validator. The validator should not change anything, should only compare and raise error if it's wrong (repair actions can change).

Or there can be additional collector that would "set" the attributes to values set on existing entities, so rest of publishing is using values that are really. I can't tell which is better because I don't know at which places it is used.

To explain a bit more here. extract_hierarchy_to_ayon is blindly updating any folder attributes from the data provided by the instance.

I don't think it's blindly updating it, it is updating them based on what was collected from scene, or? The host said, that the resolution is A, so I should set it to A.

[rdelillo]

No you're right, with @moonyuet and your feedback, I think I overworked a bit the feature here.
I've change this to make it a simpler validator:

• Validate resolution from hierarchyContext (disabled by default): if a folder already exists under a different resolution this validation fails.
• This can be skipped via UI (and a repair action).
• If the validation fails, this becomes the user responsability to accomodate the instance (via different ways) to keep a consistent resolution.

Sounds safer to me, what do you guys think ?

[moonyuet]

I tested with hiero and get confused on the testing notes. Is the validator supposed to be optional?

I can't spot the toggle to turn/turn off the validator, but it is actually doing the check.

Otherwise, it seems okay and works.

[rdelillo]

I tested with hiero and get confused on the testing notes. Is the validator supposed to be optional? I can't spot the toggle to turn/turn off the validator, but it is actually doing the check.

Thanks @moonyuet for the feedback !
The toogle to skip/perform the validation is here. Should I rephrase the label a bit or detail more in the testing steps ?

[moonyuet]

I tested with hiero and get confused on the testing notes. Is the validator supposed to be optional? I can't spot the toggle to turn/turn off the validator, but it is actually doing the check.

Thanks @moonyuet for the feedback !

The toogle to skip/perform the validation is here. Should I rephrase the label a bit or detail more in the testing steps ?

I see. I think the label should be more descriptive and otherwise it looks good.

[iLLiCiTiT]

I'm confused about what this actually does? It returns first folder with "Shot" folder type that can be found in project?

What is that good for? Why we're using EntityHub for it? How do we know the folder type is actually "Shot"? Shouldn't we find existing entity by the path we're looking for, or by something that has merit? How does fixing one entity help with the others? Or we're just finding first shot to set the same values for all other shots?

[rdelillo]

I've reworked this function. It is now _validate_hierarchy_resolution which validate the resolution attributes of hierarchyContext against existing folders (hence the EntityHub usage).

Does this work OK for you ?

[antirotor]

with the code changes I think this looks good. Can't test it right now, though.

[moonyuet]

Tested successfully in Hiero

[iLLiCiTiT]

Suggestion to avoid using EntityHub here. It would be faster to prepare all folder paths you need, fetch all folder entities at once using those paths and then compare them.

entity_items_by_path = {}
folder_paths = []

current_item = next(iter(hierarchy_context.values()))
queue = collections.deque()
queue.append((current_item, ""))
while queue:
    current_item, current_path = queue.popleft()
    children = current_item.get("children") or {}
    for name, child in children.items():
        path = f"{current_path}/{name}"
        folder_paths.append(path)
        queue.append((child, path))
        # Skip validation of items without attributes
        if child.get("attributes"):
            entity_items_by_path[path] = child

# Fetch folder entities that should be validated
folder_entities_by_path = {
    folder_entity["path"]: folder_entity
    for folder_entity in ayon_api.get_folders(
        project_name,
        folder_paths=folder_paths,
        fields={"path", "attrib"}
    )
}

# Run the validation
for path, item in entity_items_by_path.items():
    folder_entity = folder_entities_by_path.get(path)
    if folder_entity:
        self._validate_folder_resolution(
            item["attributes"],
            folder_entity,
        )

(Would require change in _validate_folder_resolution to work with flder entity as dictionary.)

[iLLiCiTiT]

I still think this should be addressed. Using EntityHub can slow down the publishing a lot. EntityHub is fetching the entities one by one if are not pre-fetched... You know ahead which entities you're interested in, so using get_folders with prepared paths will make it signifincatly faster for bigger hierarchies.

If you'd compare e.g. 1 seqeunce vs. 10 sequences it can be 10x slower.

[jakubjezek001]

@rdelillo are you planning to address the changes and remove the EntityHub?

[iLLiCiTiT]

I think this plugin should be Context plugin to run only once, or?

Currently it is instance plugin, with attribute on each instance, but validates all instances (and is running on all instances).

[jakubjezek001]

Perhaps the main reason this is on instance is that user can control this per instance @rdelillo ?