-
Notifications
You must be signed in to change notification settings - Fork 34
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
General: Unify profile filter criteria in code #827
Comments
'instance' passed into |
Totally agree. Best argument signature would allow us to easily test and recreate even outside Pyblish plugins, etc. with the smallest yet easiest data set possible. Whether that's anatomy data or lower level like asset entity, task name and primary family name (or families). Even anatomy data preparation appears to be quite massive in CollectContextAnatomyData and CollectInstanceAnatomyData so recreating anatomy data might also be non trivial outside those plugins. |
I would say that it must be unified how the data are stored on instance and force to do it that way first. For example each instance must know it's asset and task, and empty task on instance should be considered as it does not belong under any task and must not look in context for task. Until all collectors are changed that
This is caused by limitations of creator system when farm instance has family
True that is probably a bug
That is because PR adding the plugin was created at the same time when
I'm afraid I don't agree, at least not at this stage. Each plugin should know what he needs, right now it's not unified. We should first make sure that data on instances are ready for this before adding function that would do that and even with that plugin should have ability to decide what data are used and how. Also there are cases when these data change during publishing and it's not possible to do it only during collection because they may change, that probably should not happen but that's reason why I told many times that we have to add new integrator so we know what has to be changed and where without changing current integrator, because that just cannot be tested in one PR. |
I understand there must be a reason for splitting - I'm just pointing out that it'd be best to take a generic approach and always do it the same unless we explicitly don't want that. So if always splitting to the last part is the most sensible. Sure.
All I'm saying is we would need a trivial entry point. Like @kalisp points out we'd even rather not rely directly on the instance data since it's so non-transparent. If the same profile criteria are used in multiple locations where they should behave the same I'd prefer finding a way where we can make sure they stay consistent across.
Best to move this point to the Integrator PR if relevant to the implementation. |
It's not sensible at all :D But we don't have a better way...
Something like this?
|
Exactly like that - as long as we don't have to do any complex massaging to the input data. E.g. if I have to go through huge loops to get Do note however: I believe the filter data is also used to define the subset names so I don't think subset can be an input for these. :)
|
Not sure how much is still problematic in AYON - but will transfer it to discuss. |
My personal opinion is that DRY can hurt sometimes, especially when we did split addons. I would say it is much safer and better to create the data at the place where is used instead of trying to abstract it. |
Seems like you agreed with my previous opinion? Closing issue, please reopen if you think it still should be resolved. |
Description
Profile filtering is done in a myriad of locations in the code and in some areas I think can be unified to avoid logic differences and errors.
As an example:
subset_name_profiles
the last part of the family name is used.api.Session["AVALON_APP"]
for thehosts
filter criteria whereas the Integrator usescontext.data["host_name"]
context.data["hostName"]
which however CAN be different fromapi.Session["AVALON_APP"]
which as listed above is potentially also wrongly used in some areas of the code?task_type
entry in the filter criteria even though the Admin settings does expose it. This seems like a bug which I think can be traced back to not having a clear definition and entry point for these filter criteria.I'm aware the
filter_profiles
is also used in other ways with other filter criteria. However it seems like a bunch of areas use the same filter criteria but seemingly with little consistency? At this point in time I'm mostly targeting those filtering criteria usingEven better if we could also unify this in Admin Settings with its own Profile Filter Schema for the settings - so that if we change the schema and the function all areas support the extra filtering features, like how it appears "Task Type" was added at a later state but not correctly implemented in some areas?
Describe the solution you'd like
Preferably expose a
get_profile_filter_criteria
method that clearly describes what it expects as input values. Or find another way to ease profile filtering in the many different locations in the code base.In the Integrator Refactor I tried to use the simplest data source that is accessible that had the 'right' formatting compared to what the old integrator was doing and turn that into
get_profile_filter_criteria
and made it use only the parsedanatomyData
. Not saying that's the good way and I'd definitely love to include that in this Issue/discussion.Additionally I think we can get by by having a single
collect_profile_filter_criteria
Collector so that other plug-ins could even use the cachedinstance.data["profile_filter_criteria"]
or alike. Not sure if better but at least it'd mean consistent across the publishing workflow.Nonetheless I feel the urge to just have an easily accessible and consistent way to create the filter criteria would be beneficial.
[cuID:OP-2989]
The text was updated successfully, but these errors were encountered: