-
Notifications
You must be signed in to change notification settings - Fork 17
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
Fix: product ending with _1 confuses expected files validator #125
Conversation
renderMain_1.mov caused issues as was recalculated by frames provided into multiple files.
… bugfix/AY-7453_Deadline-product-ending-with-_1-confuses-expected-files-validator
|
||
# Update the expected files | ||
expected_files = job_expected_files | ||
is_video = f'.{repre["ext"]}' in VIDEO_EXTENSIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather check if is in IMAGE_EXTENSIONS
...
client/ayon_deadline/plugins/publish/global/validate_expected_and_rendered_files.py
Outdated
Show resolved
Hide resolved
client/ayon_deadline/plugins/publish/global/validate_expected_and_rendered_files.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Jakub Trllo <[email protected]>
Co-authored-by: Jakub Trllo <[email protected]>
client/ayon_deadline/plugins/publish/global/validate_expected_and_rendered_files.py
Outdated
Show resolved
Hide resolved
client/ayon_deadline/plugins/publish/global/validate_expected_and_rendered_files.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
# Update the expected files | ||
expected_files = job_expected_files | ||
is_image = f'.{repre["ext"]}' in IMAGE_EXTENSIONS |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
- It's listed as required here: https://github.com/ynput/ayon-core/blob/9ea9e344a2e147e1681a291795f3352ea71cb5af/client/ayon_core/plugins/publish/integrate.py#L100
- Plus we're directly accessing the
ext
key here: https://github.com/ynput/ayon-core/blob/9ea9e344a2e147e1681a291795f3352ea71cb5af/client/ayon_core/plugins/publish/integrate.py#L542
So ignore me, definitely doesn't seem optional anymore. 👍
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
… bugfix/AY-7453_Deadline-product-ending-with-_1-confuses-expected-files-validator
…ending-with-_1-confuses-expected-files-validator' into bugfix/AY-7453_Deadline-product-ending-with-_1-confuses-expected-files-validator
It was highlighed that `repre["ext"]` might not be required. This should be safer.
This reverts commit 091d74d.
Reverted logic for optional |
Changelog Description
ValidateExpectedFiles
resolved burnin.mov
file containing_1.mov
as sequnce and exploded it into multple expected files.Now logic does this only for image sequences, it doesn't make sense to recalculate expected files for movie like representations.
Testing notes:
renderSmthMain_1
in Nuke