Minor improvement on cbloader & Rename api.load() arg data to options

[davidlatwe]

Little changes...

Motivation

I use cbloader and the data of representation object is required but the tool did not pass it.

Implementation

Add one line to extract data from representation and pass to api.load.

Also, I noticed that instead passing the ObjectId of representation to api.load, directly pass the representation object can reduce one database query, since the function get_representation_context which been used here, is able to accept both form.

[BigRoy]

Thanks!

Isn't this actually incorrect? Isn't it so that the api.load (The api itself) officially requires the representation id, not the representation document?

Also, I wouldn't explicitly pass data from the representation if it's in the representation anyway. The data argument is intended for tool specific input, e.g. with_selection=True or animation=False. Almost like settings/options. We don't use it yet, but it has no function if it's used for the data that is housed in the database on the representation.

[davidlatwe]

Correct me if I am wrong, in my understanding, api.load is officially require representation id to acquire context, so that it can pass context to Loader.load.

And the one who responsible to acquire context from representation id is the function get_representation_context, which currently work as follow pseudo code:

def get_representation_context(representation):
    if isinstance(representation, (str, ObjectId)):
        representation = io.find_one({"_id": ObjectId(str(representation))})
    
    # acquire context
    ...
    return context

def load(Loader, representation, namespace=None, name=None, data=None):
    ...
    context = get_representation_context(representation)
    ...

The argument representation is actually able to be accepted as str or ObjectId or dict.

Thus, api.load should be able to accept representation dict directly as well, and avoid one extra unnecessary database query.

The data argument is intended for tool specific input, e.g. with_selection=True or animation=False. Almost like settings/options.

Sounds like we have different future use case on this ?
Is the tool you refer is Colorbleed's in-house tool ?

but it has no function if it's used for the data that is housed in the database on the representation.

Yes, but I did write something to the representation["data"] with my extractor plugins, and those data is required for my loaders to process representations.

Thanks for the reply @BigRoy :)
Please let me know what you think

[davidlatwe]

Hey, I am back on this :)

But I am still not quite understand on this :

I wouldn't explicitly pass data from the representation if it's in the representation anyway.

Isn't that right the data of the representation should be accepted by the Loader by default ?
Since the field data is from the Avalon integrator plugin.

Thanks!

[mottosso]

I think what Roy is saying is that the data= of a representation isn't the same as the data passed here. Maybe we should rename this data= argument to avoid confusion in the future. That argument is for tool options. Maybe options= or opts=?

The reason it isn't passed at the moment, is because there aren't any options for this particular tool, is that right @BigRoy? Does that make sense @davidlatwe?

[BigRoy]

The reason it isn't passed at the moment, is because there aren't any options for this particular tool, is that right @BigRoy?

Yes, that is what I meant. The explicit data argument on api.load is intended for customizable options for the loading, not for "data" from the representation.

Thus, api.load should be able to accept representation dict directly as well, and avoid one extra unnecessary database query.

If it technically works it's fine with me - I just recalled that the API wasn't intended to function that way but might have assumed wrong and jumped the gun on the answer.

---

The issue regarding the data is that it's indeed not clearly passed from the get go as "data" or "representation" - however the data is present even without explicitly passing it so you don't need to pass it yourself.

You can find it in the Loader.load function in the context argument, specifically:

representation = context["representation"]
data = representation["data"]

That's what I meant with the fact that you wouldn't need to pass that explicitly, that particular data is already available to you in the load method. Making this change in code redundant, correct?

Does that help?

[davidlatwe]

Ah ah! I see now !

Thanks for your help @mottosso & @BigRoy . :)
Let me try this on my plugins

[davidlatwe]

Renaming data to options seems to be logical, since avalon.api.create has the same param interface, it has both options and data with different purposes.

"""
Arguments:
        name (str): Name of subset
        asset (str): Name of asset
        family (str): Name of family
        options (dict, optional): Additional options from GUI
        data (dict, optional): Additional data from GUI
"""

I did not take too much attention on Creator Plugin, knowing that there are options param already exists on api.create, starting to make more sense now.

[davidlatwe]

I tweaked my PR, added argument renaming (data -> options on api.load).
Works on my end, should also maintained backward compatibility.

What do you think ?

[BigRoy]

backward compact should be backward compatibility or maybe backward compat if you really want it shorter? :)

[BigRoy]

Sorry, I thought I replied to this. This looks good to me if it works. :) Just commented on one minor typo.

@aardschok what do you think?

---

@davidlatwe Nice work!

[aardschok]

I have no objections to this, nice work!

[davidlatwe]

Thanks ! I got them fixed :P

[davidlatwe]

Sphinx's latest release 1.8.0 will fail the document build.
Lock version to 1.7.9 for now.

[BigRoy]

Are these latest commits supposed to be part of the same PR?

[davidlatwe]

Sorry, that should be separate.
I'll reset it and do the right thing.

[davidlatwe]

Okay it's done, I'll put that (pin sphinx==1.7.9) in next PR alone, sorry for the trouble 😢

[davidlatwe]

Please merge #338 first and I will rebase this PR so the travis will pass :)

[davidlatwe]

I think it's okay to merge now.

[mottosso]

Thanks @davidlatwe!