Implementation of representation dataclass - #1973
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Representation dataclass (plus repre_get/repre_set access helpers) and updates many publish/farm plugins to work with either legacy representation dicts or the new dataclass, as part of addressing representation “files” typing/backward-compatibility concerns.
Changes:
- Added
client/ayon_core/pipeline/publish/representation.pywithRepresentation+repre_get/repre_set. - Updated publish plugins to use
repre_get/repre_setinstead of direct dict access in many places. - Updated farm publish logic and some pipeline utilities to read representation fields via
repre_get.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| client/ayon_core/plugins/publish/integrate.py | Switches representation key access to repre_get/repre_set during integration prep. |
| client/ayon_core/plugins/publish/integrate_review.py | Uses repre_get for tags/flags during reviewable uploads. |
| client/ayon_core/plugins/publish/integrate_attach_reviewable.py | Updates tag mutation to go through repre_get/repre_set. |
| client/ayon_core/plugins/publish/extract_thumbnail.py | Updates thumbnail extraction logic to use repre_get/repre_set for representation fields. |
| client/ayon_core/plugins/publish/extract_thumbnail_from_source.py | Uses repre_get for thumbnail presence checks. |
| client/ayon_core/plugins/publish/extract_slate_data.py | Attempts to use repre_get/repre_set for slate metadata injection. |
| client/ayon_core/plugins/publish/extract_scanline_exr.py | Updates scanline conversion logic to use repre_get/repre_set. |
| client/ayon_core/plugins/publish/extract_review.py | Updates review extraction logic to use repre_get/repre_set broadly. |
| client/ayon_core/plugins/publish/extract_review_slate.py | Updates slate review extraction logic to use repre_get. |
| client/ayon_core/plugins/publish/extract_otio_trimming_video.py | Updates OTIO trim logic to use repre_get/repre_set. |
| client/ayon_core/plugins/publish/extract_oiio_postprocess.py | Updates OIIO postprocess logic to use repre_get/repre_set. |
| client/ayon_core/plugins/publish/extract_colorspace_data.py | Uses repre_get for colorspaceData checks. |
| client/ayon_core/plugins/publish/extract_color_transcode.py | Updates color transcode logic to use repre_get/repre_set. |
| client/ayon_core/plugins/publish/extract_burnin.py | Updates burnin extraction to use repre_get/repre_set in multiple paths. |
| client/ayon_core/plugins/publish/collect_rendered_files.py | Starts using repre_get/repre_set when filling staging directories from anatomy. |
| client/ayon_core/plugins/publish/cleanup_farm.py | Uses repre_get for stagingDir cleanup collection. |
| client/ayon_core/pipeline/publish/representation.py | New Representation dataclass and helpers enabling dual dict/dataclass access. |
| client/ayon_core/pipeline/publish/publish_plugins.py | Adds add_representation that creates Representation objects on instances. |
| client/ayon_core/pipeline/publish/lib.py | Uses repre_get for representation path computation and cleanup registration. |
| client/ayon_core/pipeline/publish/init.py | Exposes Representation, repre_get, repre_set from publish package. |
| client/ayon_core/pipeline/farm/tools.py | Uses repre_get for workfile representation template fields. |
| client/ayon_core/pipeline/farm/pyblish_functions.py | Uses repre_get/repre_set; restricts clique patterns to frames in one assemble call. |
| client/ayon_core/pipeline/colorspace.py | Starts using repre_get/repre_set (but currently has import/path and mixed-access issues). |
Comments suppressed due to low confidence (3)
client/ayon_core/plugins/publish/extract_burnin.py:709
repre_get(repre, "tags")may returnNone, causing"no-handles" in ...to raiseTypeError. Useor []for safe membership checks.
# no handles switch from profile tags
if "no-handles" in repre_get(repre, "tags"):
burnin_frame_start = temp_data["frame_start"]
burnin_frame_end = temp_data["frame_end"]
client/ayon_core/plugins/publish/extract_burnin.py:732
- Same issue as above:
repre_get(repre, "tags")can beNone, so membership checks should useor [].
if (
"slate" in instance.data["families"]
and "slate-frame" in repre_get(repre, "tags")
):
burnin_slate_frame_start -= 1
client/ayon_core/plugins/publish/extract_scanline_exr.py:94
- Removing the
toScanlinetag should also handle missing/None tags safely. The currentrepre_get(...).remove(...)will fail if tags isNone.
repre_set(repre, "name", "exr")
try:
repre_get(repre, "tags").remove("toScanline")
except ValueError:
# no `toScanline` tag present
pass
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for representation in instance.data.get("representations", []): | ||
| if "slate-frame" not in representation.get("tags", []): | ||
| if "slate-frame" not in repre_get(representation, "tags", []): | ||
| continue |
| def _fill_staging_dir(self, data_object, anatomy): | ||
| staging_dir = data_object.get("stagingDir") | ||
| staging_dir = repre_get(data_object, "stagingDir") | ||
| if staging_dir: | ||
| repre_set(data_object, "stagingDir", anatomy.fill_root(staging_dir)) | ||
| data_object["stagingDir"] = anatomy.fill_root(staging_dir) | ||
| self.log.debug("Filling stagingDir with root to: %s", | ||
| data_object["stagingDir"]) |
| for burnin_def in repre_burnin_defs: | ||
| filename_suffix = burnin_def["name"] | ||
| new_repre = copy.deepcopy(repre) | ||
| new_repre["stagingDir"] = src_repre_staging_dir | ||
| repre_set(new_repre, "name", src_repre_staging_dir) | ||
|
|
| for _repre in representations: | ||
| if "trim" not in _repre.get("tags", []): | ||
| _repre_tags = repre_get(_repre, "tags") | ||
| if "trim" not in _repre_tags: | ||
| continue |
| } | ||
|
|
||
| # update data key | ||
| repre_set(representation, "colorspaceData", colorspace_data) | ||
| representation["colorspaceData"] = colorspace_data |
| for repre in tuple(instance.data["representations"]): | ||
| if all(x in repre.get("tags", []) for x in ['delete', 'burnin']): | ||
| if all( | ||
| x in repre_get(repre, "tags") for x in ['delete', 'burnin'] | ||
| ): |
| # Remove "delete" tag from new representation | ||
| if "delete" in new_repre["tags"]: | ||
| new_repre["tags"].remove("delete") | ||
| repre_get(new_repre, "tags").remove("delete") |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
BigRoy
left a comment
There was a problem hiding this comment.
Oh boy - we may need to just discuss this @iLLiCiTiT before continuing more efforts here.
| if isinstance(repre, dict): | ||
| return repre.get(key, default) | ||
|
|
||
| return getattr(repre, key, default) |
There was a problem hiding this comment.
Shouldn't we be more explicit?
if isinstance(repre, dict):
return repre.get(key, default)
elif isinstance(repre, Representation):
return getattr(repre, key, default)
raise TypeError(...)
Same for repre_set.
| if len(files) == 1: | ||
| self.set_single_file(files[0]) |
There was a problem hiding this comment.
This is wrong. The whole point is to support single frame sequences... and not enforce them to be "a single file" as str so that we can differentiate between what is a single frame sequence and what is a single file.
Also, I don't see the Sequence[str] handled separately, as such I think the typing you're trying to do is files: list[str] | Sequence[str] and isn't list[str] actually a Sequence[str] to begin with?
I actually think we can fix the whole list[str] for single frame already, without having to refactor to the whole dataclass stuff to begin with. 🤔 Looking at this PR, that may make more sense to start with.
| self.files = list(files) | ||
| self.sequence_files = list(files) | ||
|
|
||
| def set_colorspace_data(self, colorspaceData: dict) -> None: |
There was a problem hiding this comment.
This should really be more explicit about what data is supposed to be in colorspaceData - a dict is way too generic.
There was a problem hiding this comment.
I am okay with that but does it mean we also need to convert the colorspaceData as dataclass?
I actually agree with this, It is just way too much changes I am not sure if it is worth for that. |
|
If this PR is to solve the sequence issues, then representation class won't solve anything. We still have exactly same issue, there are files and we don't know anything about it. If we don't know anything about it, it is just guess game that might or might not work. I honestly do believe that only correct fix is to store more information about files. Considering representation as a class, that is different discussion, I think that needs much more discussion to think about some of the data we do store there. Clean it up a little, some of that is also related to the files themselved, because if we do store udim/frame etc. to the files then we don't need that information on representation. It is also something that will take months to fully process, it is as with product base types. It should be as backwards compatible as possible, but with the enhancements in mind, which this PR is not, this just changed keys to attributes -> that's not helpfull or solving any issue. NOTE: There already is |
I am thinking if it is the best-case to use the traits which is initiated by @antirotor. If there are some cravats, we can redesign or refactor it instead. @BigRoy @iLLiCiTiT @antirotor thoughts? If I dont get it wrong, the representation trait is only used in marvelous designer. |
|
I'd like to say yes, but I don't know how do traits solve any of the issues we have with files. We can store additional information to the representation, but not to the files themselves. It is nice to know that I have representation with sequence |
Changelog Description
This PR is to implement representation dataclass to deal with the files data type issue.
Additional info
This is implemented from the ongoing discussion. Very experimental. We would change back to the initial method by #1950.
I dont know the changes would be good idea as it vastly lowers down the readability for the backward compatibility.
Links #1855 and Related to #1950
Testing notes: