Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: product ending with _1 confuses expected files validator #125

Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import clique

from ayon_core.pipeline import PublishValidationError
from ayon_core.lib.transcoding import IMAGE_EXTENSIONS


class ValidateExpectedFiles(pyblish.api.InstancePlugin):
Expand Down Expand Up @@ -41,34 +42,10 @@ def process(self, instance):
staging_dir = repre["stagingDir"]
existing_files = self._get_existing_files(staging_dir)

if self.allow_user_override:
# We always check for user override because the user might have
# also overridden the Job frame list to be longer than the
# originally submitted frame range
# todo: We should first check if Job frame range was overridden
# at all so we don't unnecessarily override anything
collection_or_filename = self._get_collection(expected_files)
job_expected_files = self._get_job_expected_files(
collection_or_filename, frame_list)

job_files_diff = job_expected_files.difference(expected_files)
if job_files_diff:
self.log.debug(
"Detected difference in expected output files from "
"Deadline job. Assuming an updated frame list by the "
"user. Difference: {}".format(sorted(job_files_diff))
)

# Update the representation expected files
self.log.info("Update range from actual job range "
"to frame list: {}".format(frame_list))
# single item files must be string not list
repre["files"] = (sorted(job_expected_files)
if len(job_expected_files) > 1 else
list(job_expected_files)[0])

# Update the expected files
expected_files = job_expected_files
is_image = f'.{repre["ext"]}' in IMAGE_EXTENSIONS
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know representation["ext"] is not 100% required to be set, it used to be totally valid to have representation data as e.g. {"name": "exr"} without specifying ext.

Copy link
Member

Choose a reason for hiding this comment

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

Is it? I though it is requirements...

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, made logic bit safer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually - I did a search over the codebase and didn't quickly find any place where we're still generating it without "ext". But I'm quite sure it used to be optional - but it doesnt seem to be now anymore...

So ignore me, definitely doesn't seem optional anymore. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about that @kalisp - I'd say, feel free to revert... and then if we face errors somehow we should be fixing the ext requirement elsewhere most likely.

Copy link
Member

Choose a reason for hiding this comment

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

It was optional, when we started to use it, but that's a long time ago.

Copy link
Member Author

Choose a reason for hiding this comment

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

But there is some issue for representation as a folder, so what will be in those?

Copy link
Member

@iLLiCiTiT iLLiCiTiT Feb 11, 2025

Choose a reason for hiding this comment

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

I don't have answers to everything, my guess is that the "ext" would be filled to None, which should not break logic here.

Copy link
Contributor

Choose a reason for hiding this comment

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

But there is some issue for representation as a folder, so what will be in those?

Those are still required to have ext plus that is currently NOT implemented so can be disregarded for this PR I'd say.

if self.allow_user_override and is_image:
expected_files = self._recalculate_expected_files(
expected_files, frame_list, repre)

# We don't use set.difference because we do allow other existing
# files to be in the folder that we might not want to use.
Expand All @@ -84,6 +61,36 @@ def process(self, instance):
)
)

def _recalculate_expected_files(self, expected_files, frame_list, repre):
# We always check for user override because the user might have
# also overridden the Job frame list to be longer than the
# originally submitted frame range
# todo: We should first check if Job frame range was overridden
# at all so we don't unnecessarily override anything
collection_or_filename = self._get_collection(expected_files)
job_expected_files = self._get_job_expected_files(
collection_or_filename, frame_list)

job_files_diff = job_expected_files.difference(expected_files)
if job_files_diff:
self.log.debug(
"Detected difference in expected output files from "
"Deadline job. Assuming an updated frame list by the "
"user. Difference: {}".format(sorted(job_files_diff))
)

# Update the representation expected files
self.log.info("Update range from actual job range "
"to frame list: {}".format(frame_list))
# single item files must be string not list
repre["files"] = (sorted(job_expected_files)
if len(job_expected_files) > 1 else
list(job_expected_files)[0])

# Update the expected files
expected_files = job_expected_files
return expected_files

def _get_dependent_job_ids(self, instance):
"""Returns list of dependent job ids from instance metadata.json

Expand Down