Skip to content

Representation traits: hero version integrator#1766

Open
antirotor wants to merge 26 commits into
developfrom
1685-yn-0470-integrating-hero-version-not-using-all-representations
Open

Representation traits: hero version integrator#1766
antirotor wants to merge 26 commits into
developfrom
1685-yn-0470-integrating-hero-version-not-using-all-representations

Conversation

@antirotor
Copy link
Copy Markdown
Member

@antirotor antirotor commented Mar 27, 2026

Changelog Description

Adding hero version integrator and re-organizing code for publishing traits. Added possibility to integrate file paths sharing common root but living under different leaf folders where the anatomy template is used just for the common part. Added more unit tests for publishing traits.

Additional info

This adds hero version integrator for trait-based representations. It is a little bit simpler, basically replacing the whole hero version with the new one, It tries to follow the logic of the "standard" integrators but during writing this I realized, we should really simplify the logic, get rid of all the exceptions and make it more api (and therefor testing) friendly.

One additional change adds possibility to integrate representation made up by paths pointing to files in different directories but sharing the same root. For example:

Representation "foo":
    - file path 1: /foo/bar/baz/file.1
    - file path 2: /foo/bar/goo/file.2

in this case the template would be applied to /foo/bar and the baz/file.1 and goo/file.2 will be copied there as they are. Note that this won't change the frame padding, or do re-numbering of the files when needed - so in that sense it is not functionally on par with how the representations are integrated right now, that functionality has to be added later when needed.

Testing notes:

  1. Publish something using trait-based representation with enabled hero version.
  2. Check the hero version exists and can be loaded.

Note: I'll link simple testing branch for Maya that I used to publish pointcaches as trait-based representations. Currently only Marvelous Designer is using them.

@antirotor antirotor self-assigned this Mar 27, 2026
@antirotor antirotor added the type: feature Adding something new and exciting to the product label Mar 27, 2026
@antirotor antirotor linked an issue Mar 27, 2026 that may be closed by this pull request
@ynbot ynbot added the size/XXL label Mar 27, 2026
@antirotor antirotor marked this pull request as ready for review March 30, 2026 12:13
@antirotor antirotor requested a review from Copilot March 30, 2026 12:13
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds trait-focused publishing helpers and introduces a new hero-version integrator for trait-based representations, alongside refactors to reuse the new helpers and broaden supported file-layout cases (shared common root with preserved leaf hierarchy).

Changes:

  • Added ayon_core.pipeline.traits.publishing module to centralize transfer-building, template data, and legacy file info logic.
  • Added IntegrateHeroVersionTraits plugin to create/replace hero versions for trait-based representations.
  • Expanded FileLocations validation/integration to allow multi-file representations without Sequence/UDIM when files share a meaningful common root; added unit tests.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
tests/client/ayon_core/plugins/publish/test_integrate_traits.py Updated tests to use the new publishing helper functions and added a hierarchy-preservation test.
tests/client/ayon_core/pipeline/traits/test_publishing.py New unit tests for transfer generation across single-file/sequence/UDIM/common-root/bundle cases.
tests/client/ayon_core/pipeline/traits/test_content_traits.py Updated validation test to reflect new “common root” allowance for FileLocations.
client/ayon_core/plugins/publish/integrate_traits.py Refactored integrator to use centralized helper functions from traits publishing module.
client/ayon_core/plugins/publish/integrate_hero_version_with_traits.py New hero-version integrator for trait-based representations.
client/ayon_core/plugins/publish/integrate_hero_version.py Minor typing adjustments.
client/ayon_core/pipeline/traits/utils.py Removed/moved sequence detection helper from utils.
client/ayon_core/pipeline/traits/publishing.py New shared logic for building transfers, template data, and legacy file dictionaries.
client/ayon_core/pipeline/traits/content.py Added FileLocations.get_common_root() and updated validation to permit common-root hierarchies; added OriginalFilename trait.
client/ayon_core/pipeline/traits/init.py Re-exported new publishing helpers and TransferItem.
client/ayon_core/pipeline/publish/lib.py Exposed template/version/rootless helper utilities and TemplateItem for broader reuse.
client/ayon_core/pipeline/publish/init.py Re-exported new helpers from publish library.
client/ayon_core/pipeline/anatomy/templates.py Added typing to get_template_item.
client/ayon_core/pipeline/anatomy/anatomy.py Added typed get_template_item wrapper (but changes default-missing behavior).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/client/ayon_core/pipeline/traits/test_publishing.py Outdated
Comment thread tests/client/ayon_core/pipeline/traits/test_publishing.py Outdated
Comment thread client/ayon_core/plugins/publish/integrate_hero_version_with_traits.py Outdated
Comment thread client/ayon_core/pipeline/anatomy/anatomy.py
Comment thread client/ayon_core/pipeline/traits/content.py
Comment thread client/ayon_core/plugins/publish/integrate_hero_version_with_traits.py Outdated
Comment thread client/ayon_core/pipeline/traits/publishing.py Outdated
@ynbot ynbot moved this to Review In Progress in PR reviewing Mar 30, 2026
@antirotor antirotor requested a review from moonyuet March 30, 2026 16:24
@moonyuet
Copy link
Copy Markdown
Member

Tested in my side, successfully created the hero version
image
image

publish-report-260331-13-17.json

@github-project-automation github-project-automation Bot moved this from Review In Progress to Merge Requested in PR reviewing Mar 31, 2026
Copy link
Copy Markdown
Member

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The amount of code here is massive :')
But overall seems ok, commented on some code bits mostly.

from .exceptions import RootCombinationError, ProjectNotSet
from .roots import AnatomyRoots
from .templates import AnatomyTemplates
from .templates import AnatomyTemplates, PLACEHOLDER
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have my doubts about importing this placeholder object from templates. Seems a bit odd. @iLLiCiTiT thoughts?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right there, but I couldn't quickly think of another solution. PLACEHOLDER is used there as a guard and while my brain was broken about all the arguments of get_template_item and stuff I wanted to expose the real function arguments downstream without rewriting the whole thing. Maybe there is a better way.

Copy link
Copy Markdown
Member

@iLLiCiTiT iLLiCiTiT May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then rename it, PLACEHOLDER is a "placeholder", if it's being made public it has to make sense, something like NOT_SET.

return anatomy.get_template_item("publish", template_name)


def get_instance_families(instance: pyblish.api.Instance) -> list[str]:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May want to make regular Integrator start using this as well then?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely, but I think this should be done in another PR

return families


def get_version_data_from_instance(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably make regular Integrator use this as well, to ensure both approaches are the same, no? Especially because this doesn't seem representation traits specific?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, definitely, but I think this should be done in another PR (I'll copy paste it to all similar :) - I get your point but even for testing it would be better to have it in separate PR)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me being technicall, the get_ probably should be prepare_ as it also changes data on the instance.

return version_data


def get_rootless_path(anatomy: "Anatomy", path: str) -> str:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably also use in regular integrator?

Copy link
Copy Markdown
Member

@iLLiCiTiT iLLiCiTiT May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I don't see any advantage of having this function. I see it being used to get source, I would rather have get_instance_source instead of this.

EDITED:
I see it is also being used for representation entity files, but that one probably should not gracefully use path but force to use rootless path.

Comment thread client/ayon_core/pipeline/publish/lib.py
sorted_frames = sorted(col.indexes)
# First frame used for end value
first_frame = sorted_frames[0]
# Get last frame for padding
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is invalid - it's not use for padding?

Attributes:
name (str): Trait name.
description (str): Trait description.
id (str): id should be namespaced trait name with version
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should persistent be in docstring too?

Comment on lines +679 to +680
This expects the file to exist - it must run after the transfer
is done.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be good to describe here why we need the legacy files still?

Comment on lines +551 to +561
if representation.contains_trait(FileLocations):
# If representation has FileLocations trait (list of files)
# it can be a Sequence, UDIM tile set, or a group of related
# files that share a common root and preserve their hierarchy.
# Note: we do not support yet frame sequence of multiple UDIM
# tiles in the same representation.
get_transfers_from_file_locations(
representation, template_item, transfers
)
elif representation.contains_trait(FileLocation):
# This is just a single file representation
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean that if FileLocation trait is set but also FileLocations that FileLocation is ignored? Should this be validated/warned upon somewhere? or are we already doing that?

Same for Bundle btw.

Comment on lines +538 to +542
# treat Variant as `output` in template data
with contextlib.suppress(MissingTraitError):
template_data["output"] = (
representation.get_trait(Variant).variant
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the assumption that variant is the output is correct, right? I assume output here is what outputName was supposed to be? But a single instance (hence one variant) can have multiple output names.

Or is the fact that Representations have "Variant" traits just misleading/confusing and do have mixed terminology here between Variants for products and Variants for representations now?

@ynbot ynbot moved this from Merge Requested to Review In Progress in PR reviewing Mar 31, 2026
Copy link
Copy Markdown
Member

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't test but aside of the still pending comments, code changes seem fine.

@github-project-automation github-project-automation Bot moved this from Review In Progress to Merge Requested in PR reviewing Mar 31, 2026
…rating-hero-version-not-using-all-representations
@BigRoy
Copy link
Copy Markdown
Member

BigRoy commented Apr 30, 2026

Can someone resolve the conflicts.
@iLLiCiTiT can you give it your review please?

return path


class TemplateItem:
Copy link
Copy Markdown
Member

@iLLiCiTiT iLLiCiTiT May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very confusing a TemplateItem holding AnatomyTemplateItem, which in fact is TemplateItem... I wonder if you ever tried to find out where classes are being used... This is maintanance hell.

I would move it to traits as it has no other use, and rename it to something like IntegrateTemplateItem.

Is there a reason to have both template and template_object? You could just have the object and get the template from it...

template_name: str,
subkey: Optional[str] = None,
default: Any = PLACEHOLDER,
) -> Union[TemplateItem, str, None]:
Copy link
Copy Markdown
Member

@iLLiCiTiT iLLiCiTiT May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How it can return str? It either returns TemplateItem or Any if default is passed in.

Suggested change
) -> Union[TemplateItem, str, None]:
) -> TemplateItem | Any:

get_publish_template_object,
get_instance_families,
get_version_data_from_instance,
get_rootless_path,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put instance to each of the functions (get_instance_template_name, get_instance_publish_template). We should easily distinct these helper from "the" implementations.

Comment thread client/ayon_core/pipeline/publish/lib.py
).hexdigest()


def get_publish_template_object(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have the same function twice now?

self.related_trait = related_trait

@staticmethod
def get_size(file_path: Path) -> int:
Copy link
Copy Markdown
Member

@iLLiCiTiT iLLiCiTiT May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these helper methods on the class? If it's because it is directly related to them then rename it to get_file_size and get_file_checksum as I would expect that get_size would return what self.size value is. We should also have a way how to get infromation about which checksum type was used (sha256).



@dataclass
class OriginalFilename(TraitBase):
Copy link
Copy Markdown
Member

@iLLiCiTiT iLLiCiTiT May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what this trait is about? When do we set this? What is usage of it? Because we do define this based on template, and it might be partial, we technically do allow to have template like:

  • directory: {originalDirname}/publish
  • file: {originalFilename}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that this trait is set by the integrator if the file names needs to remain in case where there are multiple files in forking paths. Until we support it directly in templates, this is at least flag to know that it happened.

for file_location in self.file_paths:
yield file_location.file_path

def get_common_root(self) -> Path:
Copy link
Copy Markdown
Member

@iLLiCiTiT iLLiCiTiT May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the use-case of this again? How can one representation have multiple files in different directories?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With traits, it can. Under the common root. If that's the case, integrator will apply template formatting up to the common root. If we ever need to go further, we would need to add some "globing" functionality to templates..,

@iLLiCiTiT
Copy link
Copy Markdown
Member

Why we don't integrate traits within integrate hero plugin?

@BigRoy
Copy link
Copy Markdown
Member

BigRoy commented May 14, 2026

Why we don't integrate traits within integrate hero plugin?

@antirotor can you loop back here?

antirotor added 2 commits May 21, 2026 17:42
…rating-hero-version-not-using-all-representations
…rating-hero-version-not-using-all-representations
@ynbot ynbot moved this from Merge Requested to Review In Progress in PR reviewing May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XXL type: feature Adding something new and exciting to the product

Projects

Status: Review In Progress

Development

Successfully merging this pull request may close these issues.

YN-0470: Integrating hero version not using all representations

6 participants