-
Notifications
You must be signed in to change notification settings - Fork 67
Container validation w/ status filtering #1516
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
base: develop
Are you sure you want to change the base?
Container validation w/ status filtering #1516
Conversation
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.
Pull Request Overview
This PR adds support for validating outdated containers only against approved (done state) versions. It introduces a new validate_if_approved flag to the ValidateOutdatedContainers plugin that, when enabled, filters versions by approved status.
Key changes:
- Added status-based filtering for container validation
- Implemented a new function to get last versions filtered by status
- Updated container checking logic to support optional status filtering
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| client/ayon_core/plugins/publish/validate_containers.py | Adds validate_if_approved flag and logic to extract approved statuses from project entity |
| client/ayon_core/pipeline/load/utils.py | Implements get_last_versions_with_status() function and updates container checking functions to accept optional statuses parameter |
Comments suppressed due to low confidence (2)
client/ayon_core/pipeline/load/utils.py:1134
- AttributeError will occur when
last_version_entityis None. Ifget_last_versions_with_status()returns None for a product_id (as set in line 920), accessinglast_version_entity['id']will fail. Add a None check before accessing the dictionary.
if version_id != last_version_entity["id"]:
client/ayon_core/pipeline/load/utils.py:998
- The docstring for
get_outdated_containersis missing documentation for the newly addedstatusesparameter. Add documentation describing its type (Optional[Iterable[str]]), purpose, and how it affects the function behavior.
ignore_locked_versions (bool): Locked versions are ignored.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
By the way, I saw that there is a |
|
|
||
|
|
||
| def filter_containers(containers, project_name): | ||
| def filter_containers(containers, project_name, statuses: Optional[Iterable[str]]=None): |
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 think the changes made here will cause issues. With the statuses you might have latest version, but if is not approved then it would not be marked as latest which is against the output value names and what would user see. "Last version can be outdated"
I don't have solution for that now. But I think it might cause confusion. Maybe with statuses handling the logic has to be changed to be more granular.
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.
Maybe statuses should be called approved_statuses instead, at least for that signature; in the end it is the caller who is expected to pass on the approved statuses, like in the case of ValidateOutdatedContainers, where it looks for the project statuses that are set as "done" (assuming this is the definition for approved statuses).
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's about what the naming means in readable way of what it does. If last version is 20, but last approved is 17, then I'll get information "using outdated version 20", but it is latest.
Based on how the function is called you can get different latest versions. If I call it with statuses then I will get different set of latest versions.
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.
Considering how the function is used it should work the same way across the board. If I use the same api function from anywhere I'll get exactly same output.
Considering the output, I'm not convinced that forcing latest approved version is good idea. In early stages of production you won't have any approved versions but you already might want to use them in publishing. Allowing only approved versions won't allow you to publish.
From production point of view, what is the desired and expected behavior?
I need other opinions on this topic @BigRoy @antirotor . I'm lost in approved versions.
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.
what I can suggest here is to fallback to the latest version if there is no latest version with an approved status. I added a quick commit with a proposal here; with this approach we need to do two api calls, one to get the latest version with status and the second one to get the fallback. There is room for improvement as the second call could be done only for the products with missing versions with approved status, but this would be the rough idea @iLLiCiTiT .
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.
That doesn't change the issue.
It is bending existing code to achieve some goal leading to unstable results with wrong names that are based on optional arguments.
To add status handling of container version validation it really must be done properly, fully. First we have to define what is expected behavior and how to handle all cases that are affected by it. Then we can discuss code changes to achieve it. It is not really as simple as these changes.
What we want to achieve
Consider version status in validation of loaded containers.
I guess?
What it affects
I will replace outdated with invalid and uptodate with correct from now on. Because with the goal it is also important to change the terms.
- Scene inventory tool -> Showing "invalid" containers, suggesting updates to "correct" version.
- Publishing -> validation of loaded containers.
- Popups in some host integrations checking if there are "invalid" containers.
- Messages and logs in all 3 previous cases.
- Maybe also load tool because right now it shows last version by default, is that something we should consider too?
- What if is loaded last version, but not last approved version? Should it fail too?
- What would be the logic behind that decision? Is it settings driven? Should we be able to support both, both last version and last approved versions are ok.
- Should we also validate the opossite e.g. if product status is "On hold" -> that matters too, as you might have approved version but postponed product becase of requests.
- Should we add action
Update to last approved versionnext toUpdate to last versionin scene inventory tool? - ...probably few more questions that would popup when we'll start to dig into the usage.
- Should we consider that
filter_containersis "deprecated" and instead we'll use different function that would give you full information detail about each container? So you'd get informationis_last_version: true/false, is_last_approved_version: true/false, ....
Accepted suggested change by @iLLiCiTiT Co-authored-by: Jakub Trllo <43494761+iLLiCiTiT@users.noreply.github.com>
…dated-containers-with-statuses
…m/jm22dogs/ayon-core into outdated-containers-with-statuses
Changelog Description
This PR introduces an optional statuses parameter to
ayon_core/client/ayon_core/pipeline/load/utils.py#filter_containers.When supplied, the function will compare containers against the latest approved versions that match the given statuses, instead of always using the latest.
Key Points
Additional info
The helper
ayon_core/client/ayon_core/pipeline/load/utils.py#get_last_versions_with_statusis added to fetch the appropriate versions via the new status filter.The PR also demonstrates the effect on the publish plugin ayon_core\plugins\publish\validate_containers.py#ValidateOutdatedContainers`, and it could be extended to the DCC hooks like Houdini's and Nuke's.
TODO
Expose a setting in the publish plugin to toggle the status‑based filter.
Future work (outside this PR)
Move
ayon_core/client/ayon_core/pipeline/load/utils.py#get_last_versions_with_statusinto the ayon-python-api repo as a GraphQL query for a cleaner API interface.