Open BigRoy opened 9 months ago
Thank you @BigRoy for starting this. I like where this is going but could we rethink this proposal so it's not only for workfiles and we can use the same API to write "comment"/"description" to any product entity? The only difference here would be that the workfile UI shows that "comment" for the workfile entities
I like where this is going but could we rethink this proposal so it's not only for workfiles and we can use the same API to write "comment"/"description" to any product entity? The only difference here would be that the workfile UI shows that "comment" for the workfile entities
Could be nice - I'm not sure how that models to the different entities. I know e.g. assets have data.label
(and data.description
maybe?) but no data.comment
and version
entities have data.comment
. Workfiles apparently have data.note
- haha.
I'm not sure how the data model in AYON would be streamlined to maybe ease these efforts. With those differing so much (in OpenPype at least) I'm not sure what type of consistently exposed functions make sense here. Maybe @mkolar can offer more insight how this might change with AYON.
By the way, looking at the basics of the logic above, updating any data of an entity seems to follow this API:
Please consider these pseudocode examples - since these operations are intended to 'update database' entries proceed with care when running them as tests. ;)
doc = {...} # project, asset, subset, version or workfile doc?
new_doc = copy.deepcopy(doc)
# Make changes to document here
update_data = prepare_workfile_info_update_data(
doc, new_doc
)
if update_data:
session = OperationsSession()
session.update_entity(
project_name, doc["type"], doc["_id"], update_data
)
session.commit()
Which means that updating/setting comment, note or label would be as simple as doing that with either data.note
or data.label
, etc. Taking that a bit further, some pseudocode like this could be used:
import copy
from openpype.client.operations import (
OperationsSession,
# Note that operations actually has
# prepare update data functions for the
# different types - to ensure some keys
# are never updated
_prepare_update_data
)
def update_entity(doc, changes):
new_doc = copy.deepcopy(doc)
for key, value in changes.items():
if "." in key:
# Allow specifying e.g. key data.note
# to only adjust data.note
parent_keys = key.split(".")
key = parents.pop()
target = new_doc
for parent_key in parents:
# TODO: What to do if an existing parent key
# isn't a dict itself. Do we want to replace it?
target = target.setdefault(parent_keys, {})
target[key] = value
else:
new_doc[key] = value
update_data = _prepare_update_data(doc, new_doc)
if not update_data:
return
session = OperationsSession()
session.update_entity(
project_name, doc["type"], doc["_id"], update_data
)
session.commit()
# Some examples
update_entity(asset_doc, {"data.label": "new label"})
update_entity(version_doc, {"data.comment": "new group"})
update_entity(version_doc, {"data.subsetGroup": "my new subset group"})
update_entity(workfile_doc, {"data.note": "new note"})
Note that I have not tested these pseudocode examples. Since they "update database entries" please proceed with caution when testing. But as the comment there shows the openpype.client.server.operations
has dedicated update functions for the different entity types which should be used instead to have "some safeguards"
Coincidentally, today I had one of these issues were I had to update some published mongo data so thank you @BigRoy for that code snippet! Here's the updated working version I used for updating a few versions:
from openpype.client.operations import (
OperationsSession,
prepare_subset_update_data
)
def update_entity(project_name, doc, changes):
new_doc = copy.deepcopy(doc)
for key, value in changes.items():
if "." in key:
# Allow specifying e.g. key data.note
# to only adjust data.note
parent_keys = key.split(".")
key = parent_keys.pop()
target = new_doc
for parent_key in parent_keys:
# TODO: What to do if an existing parent key
# isn't a dict itself. Do we want to replace it?
target = target.setdefault(parent_key, {})
target[key] = value
else:
new_doc[key] = value
update_data = prepare_subset_update_data(doc, new_doc, replace=False)
if not update_data:
return
session = OperationsSession()
session.update_entity(
project_name, doc["type"], doc["_id"], update_data
)
session.commit()
And we should make sure to validate that the doc
given to update_entity
has all the "data" queried... otherwise you end up removing the data fields that weren't queried (I might or might have not done that mistake and ended up having to write extra logic to rewrite it)!
is there any difference really on prepare_subset_update_data
or prepare_version_update_data
?
And we should make sure to validate that the doc given to update_entity has all the "data" queried... otherwise you end up removing the data fields that weren't queried (I might or might have not done that mistake and ended up having to write extra logic to rewrite it)!
By the way, I believe if you only want to update values and never remove values then I think you can maybe do without prepare_update_data
and directly pass only the to change data to:
session.update_entity(
project_name, doc["type"], doc["_id"], update_data
)
Since the preparing function seems to only record the 'changes' between the documents and has a special value for removals. _N
Again, test this assumption ;) do not run it live, etc. <- this assumption was wrong.
For example:
update_data = {"data": {'hello": "world"}}
This would replace the full data
key with the new dict - it doesn't just update "data.hello".
EDIT: Looking at this code I'm pretty sure nested dicts will be completely replaced, not updated partially.
You might "beat that" by turning nested dicts into flattened keys like: {"data.note": "value"}
in MongoDB operations
But I'm not sure if those will eventually also work with the AYON API. Maybe @iLLiCiTiT knows. (referencing discord question here)
is there any difference really on prepare_subset_update_data or prepare_version_update_data?
As I see it, currently not but of course it might be in the future.
Yeah that was my issue that prepare_update_data
value
is directly the nested dictionary of data
... so it was replacing it completely with my small subset of data
...
Is there any easy way to retrieve the mongo _id in any of the OP uis? I was thinking that it would have been useful if I could just see it in the Loader
"Version info" tab
Is there any easy way to retrieve the mongo _id in any of the OP uis? I was thinking that it would have been useful if I could just see it in the Loader "Version info" tab
Right click in the version tab, copy raw data:
But I believe that only provides the representation document.
Otherwise you might want something like this Debug Loader (note that I believe this one only enables when running from a build or OPENPYPE_DEBUG
env var is 1 - it doesn't come natively with OpenPype however)
nice, that's useful, I will add it to my fork, thank you!!
One problem I see here is that artist notes as they are now only seem to affect local workfiles, which in OpenPype aren't tied to the db. So applying artist notes to products instead (it's equivalent to subsets right?), would be a completely different thing. In the context of workfiles, it would mean that the notes would be written to published workfiles (which are subsets, therefore they should be able to use the same data as other subsets), but that would not cover cases in which we would want to add notes to local workfiles. Shouldn't local workfiles and products have their own separate implementation?
One problem I see here
Where is that exactly? Are you responding to something specific by Fabia? I don't see him requesting to set artist notes on published workfiles. The way I interpreted it is: "Can we make the API more generalized so that it becomes trivial to update the comment
or note
or anything else on published data.
I agree that the request differs from the current PRs direct needs - and regarding comment, notes, descriptions, etc. as I remarked here there's also not one general name for it.
I'd say having a simple "update any data" API would be nice but might be best left for a separate issue referencing this PR as a prototype for one aspect.
I'm just saying local workfiles and products are two different things. I'm not sure how this API change could work out for both, unless it's separated, with one feature for workfile note, and another for products comment. What do you think about it?
Agreed. I think this PR focused on just workfiles is fine. @fabiaserra how would you conclude after above discussion?
yeah definitely seems like it would have to be two different APIs for local workfiles and products, how are the artist notes for local workfiles stored?
@iLLiCiTiT if I'm not mistaken, this is not really relevant in AYON, right? or rather fixed by the new API
@iLLiCiTiT if I'm not mistaken, this is not really relevant in AYON, right? or rather fixed by the new API
I believe it's been said that the workfile API is different in AYON, therefore I don't think this can be used as is in AYON. Also the initial goal with this feature was to be used in #5660 which has been closed. Its implementation in #5808 has been closed as well. I think it would be fine to close this issue.
Is there an existing issue for this?
Please describe the feature you have in mind and explain what the current shortcomings are?
Through the workfiles tool the user can get and/or save an "Artist note" per workfile and potentially store other data into the workfile documents. This functionality seems deeply embedded into the UIs but there seems to be no simple API to set these through python code.
Why?
Exposing this through the API could allow it to be used for:
Published {datetime} to subsets: {",".join(subsets)}
so that it could e.g. show:How would you imagine the implementation of the feature?
I'd expect a simple
set_workfile_data
and/orset_workfile_note
function that'd work for both OpenPype and AYON.Here's working code that could be the basics for the OpenPype API but it'll likely not match for AYON:
Are there any labels you wish to add?
Describe alternatives you've considered:
No response
Additional context:
@iLLiCiTiT mentioned that for AYON workfiles implementation a separate controller implementation of saving workfile notes, etc. has been implemented. As such it's likely that both the old tool and new tool should be refactored to use a singular API (which in the API might need an OpenPype way and an AYON way - with the OpenPype code being described in the issue ☝️ )
If the API is written, I'm personally not sure where this would be stored within OpenPype or Ayon? Is it
openpype.pipeline.workfile.{???}
?[cuID:OP-7102]