Skip to content
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

Using representation config instead of parents when formatting template in loader #351

Open
mkolar opened this issue Nov 8, 2018 · 20 comments
Labels
api document-this Needs to be documented. enhancement

Comments

@mkolar
Copy link
Member

mkolar commented Nov 8, 2018

This is directly related to #121

We have more complex templates than what is expected by default. That means our paths are not formatted correctly when trying to load a representation.

The issue seems to be here

core/avalon/pipeline.py

Lines 161 to 167 in 79ea85a

data = {
key: value["name"]
for key, value in context.items()
}
data["root"] = registered_root()
data["silo"] = context["asset"]["silo"]

We are storing context in the db, but the data used to format the publish template is explicitly set to be only made up of parents. That results in technically hardcoding how we can work with templates and is not very flexible.

I suggest using data = context['representation']['context'] instead to make this more arbitrary and open to customisation in different studios.

I've done a quick test and it seems to solve all our issues and shouldn't break any backwards compatibility (we might have to do it via condition though in case old db data doesn't have the context field in the representation)

@tokejepsen
Copy link
Collaborator

What are your thoughts on the issue of updating old assets when a parent is modified?

@mkolar
Copy link
Member Author

mkolar commented Nov 9, 2018

Not sure what exactly you mean. Can you give example?

@tokejepsen
Copy link
Collaborator

Sorry ignore that comment. I misunderstood the issue.

@mkolar
Copy link
Member Author

mkolar commented Nov 15, 2018

On top of this we would actually also save the template that was originally used to format the path. This is to prevent mismatch during the import in case the publish template had to be tweaked during the production. (No it shouldn't happen, Yes it might happen on longer productions)

So the representation itself would hold all the data needed to format the right path, and still allow for overriding some keys if the project is for example moved to a different root.

@BigRoy
Copy link
Collaborator

BigRoy commented Nov 16, 2018

I suggest using data = context['representation']['context'] instead to make this more arbitrary and open to customisation in different studios.

I think allowing to make it more arbitrary is perfect! It should allow formatting with custom data or other required keys a studio might introduce in their configuration so that they take more control over their template.

However, we should choose a simple interface for doing so that will remain consistent. That could be context or maybe just the representation's data. It should be something that makes sense to customize. Both context and data sound like good options to me, if at least the functionality is documented clearly for new developers.

@mottosso might have some good input on keeping this lean and mean and where to put it. Also especially since he originally created #121.

@mottosso
Copy link
Contributor

mottosso commented Nov 17, 2018

Thanks for pinging me on this issue, I think @mkolar is right about storing context inside of the representation. My initial concern was about whether a parent should be allowed to change; for example, if the project is called hulk and the name hulk is stored in a representation, than if hulk were to change and become e.g. hulkRising then suddenly every representation would be out of date.

By traversing the hierarchy each time, things would always remain up-to-date, even as things change.

However, I think it's fairly safe to say at this point that most things don't need to change, and in fact is encouraged not to, as there are lots more benefits of having a fixed hierarchy, such as keeping tabs on history and dependencies between asset-to-asset and shot-to-asset (some of which we aren't doing yet, but I expect we will).

For the things that do need to change, such as the nice names of things or icons or artist assignments and what not, that simply won't be stored in multiple places like this.

With this in mind, there are two kinds of data

  • Static
  • Dynamic

Static being things like the identifier of assets and projects, such as hulk and Dynamic being data that can change. Static data may be duplicated in things like a Representation, to simplify code and avoid needless calls to the DB, whereas Dynamic data is expected to change and need re-querying to ensure up-to-datedness.

Does that sound ok to everyone?

@mottosso mottosso added enhancement api document-this Needs to be documented. labels Nov 17, 2018
@mkolar
Copy link
Member Author

mkolar commented Nov 19, 2018

That could be context or maybe just the representation's data.

context sounds preferable to me. Even though there is a caveat if we only store it on the representation. There are situations where you're filling a path but not working with representation. Workfiles app is a good example. Currently, the work template is not filled by representation data, but asset data. So if we want to keep it consistent we might need to store context on all the objects.

@BigRoy
Copy link
Collaborator

BigRoy commented Nov 20, 2018

If we can write out what the difference is between context and data it makes sense to me. But context being static compared to data didn't make it clear to me. Since a published instance's data shouldn't change either. I can see how Marcus' his more detailed explanation describes what the intent is of the keys/values in either of the two - yet I still feel it's a bit ambiguous for newcomers.

@mottosso do you think there's a better way to describe the differences?

@BigRoy
Copy link
Collaborator

BigRoy commented Jan 14, 2019

@mkolar I believe you guys are using this, right? Would you happen to be able to set up a PR?

@mkolar
Copy link
Member Author

mkolar commented Jan 17, 2019

Hey. Yes we are using this, but we've actually added another piece of data to data on representation and that is path, which is the full formatted path to the file it represents and we're currently using that explicitly to load the assets.

There we multiple reasons for that, most importantly that we quickly needed the system to support out hierarchical asset structures and this was a good way to do it that doesn't really break anything.

Our fort of core is currently contains a few connected features that we need to clean to be able to send PRs for discussion. But I'll try doing it this week.

@BigRoy
Copy link
Collaborator

BigRoy commented Jan 17, 2019

Hey. Yes we are using this, but we've actually added another piece of data to data on representation and that is path, which is the full formatted path to the file it represents and we're currently using that explicitly to load the assets.

If this is the unformatted path with just the keys and the keys to format them are actually included (and some based on the live environment), e.g. the root could be different depending on the environment (like different OS) you are working on. Then that could be helpful.

But I'll try doing it this week.

Great!

@mkolar
Copy link
Member Author

mkolar commented Jan 18, 2019

This is what we are saving currently to DB. So we can use the full path, or reconstruct it from the template and context keys (which can be replaced dynamically if needed of course)

id: 5c41dcd19e4ee0671c7e594a
name: "exr"
parent : 5c41dcd19e4ee0671c7e5949
type : "representation"
dependencies : Array
context : Object
    subset: "render_writeDefault"
    family: "write"
    hierarchy: "sequences\sq01"
    version: 3
    projectcode: "mln"
    task: "compositing"
    project: "milan_testing"
    asset: "sh010"
    representation: "exr"
    root: "C:\projects\_AVALON"
    padding: #####
data: Object
    path: "C:\projects\_AVALON\milan_testing\sequences\sq01\sh010\publish\render\compositing\render_writeDefault\v003\mln_sh010_compositing_v003_render_writeDefault.#####.exr"
    template : "{root}/{project}/{hierarchy}/{asset}/publish/{family}/{task}/{subset}/{version}/{projectcode}_{asset}_{task}_{version}_{family}_{subset}.{padding}.{representation}

@mottosso
Copy link
Contributor

Sorry for a delayed response to this one. I like the ideas presented here, but still can't wrap my head around when it is OK to include the root anywhere in the DB? In this case C:\projects\_AVALON. The root will always be relative a given OS and mounts in that OS, and should always be registered by the time Avalon is launched.

Could we remove both the root key and reference to root in the path key?

@mkolar
Copy link
Member Author

mkolar commented Feb 25, 2019

Could we remove both the root key and reference to root in the path key?

we're actually ignoring the root in the context when filling the path to do exactly what you said, but having it there keeps the record of the exact way it was published for later if needed.

dropping root from the template is not the best idea I think. considering you still have to fill it when trying to get the final path. Having it there also paves the way for multi-root setup where render could have {root_renders} and alembic {root_cache} this way it would be trivial to fill it correctly dynamically.

Dropping it from path certainly would work, as that data is not useful from a different platform that where it was published, but again we saw this key more as a fallback and way to check how exactly it was published to be able to solve possible import issues in the future. Say someone screws up the DB templates, or something similar. Having explicit path allow to do post validation on where the newly resolved path matches originaly published and so on.

Id hope that 99.9% of representations would get loaded with

data = representation['context']
data['root'] = registered_root()
representation['data']['template'].format(data)

@mottosso
Copy link
Contributor

but having it there keeps the record of the exact way it was published for later if needed.

If needed for what, though? It doesn't tell you anything if the one reading it is on a different OS or mount setup. Seems to just open up room for confusion - e.g. "should I use it?". We could call it originalRoot or the like, to make clear that it is for visual inspection only, and not actually for direct use.

dropping root from the template is not the best idea I think.

Yes, we still need it in the template, what I meant was removing it from your path key, that currently says:

path: "C:\projects\_AVALON\milan_testing\sequences\sq01\sh010\publish\render\compositing\render_writeDefault\v003\mln_sh010_compositing_v003_render_writeDefault.#####.exr"

Though if you aren't using the context["root"] key, then do you also not use this path key? Could we remove this key then altogether as well? Is it also just for visual inspection?

@mkolar
Copy link
Member Author

mkolar commented Feb 25, 2019

Yep it is mostly for visual inspection and knowing exactly what the path was originaly when published. We find this kind of data very useful after project is archived for example and we need to go back to it. It's comparable to maya referenced files for example and paths can be easily fixed.

The point is that I'm not keen on throwing away information about exact published path at the time od publishing. If I never need it than perfect but we have occasions in the past where a simple absolute path saved our neck. (not in avalon :) ), however dropping the root section is fine I think.

With the root in context I agree. That could be dropped

@mottosso
Copy link
Contributor

Mm, storing all relevant state relative a publish is a good idea in any case I think. But I wonder whether that type of "historical" or "statistics" data is better suited as contents of the actual publish, rather than in the schema of the database. Could it be stored in the asset["data"] dictionary instead? That's meant as a more generic kind of "user-data", things not relevant to the pipeline nor tools, but to the people (and as optional data for tools).

@mkolar
Copy link
Member Author

mkolar commented Feb 25, 2019

You mean stored on the parent asset? Or in a data member asset['data'].

the second one makes sense I think. For us its more about having the data, than where it's stored.

@mottosso
Copy link
Contributor

I mean in asset['data] yes.

Anything part of an objects schema (i.e. at the root level) is meant to be asserted against, guaranteed to exist and follow a strict convention, such that tools and things can rely on it. Anything in ['data'] = {} on the other hand is optional/additional. Seems a good place for this kind of statistics IMO.

@mkolar
Copy link
Member Author

mkolar commented Feb 26, 2019

Right. If you look at the example that's exactly where we store it. In which case the question might be the opposite. Shouldn't template be required then and not in asset['data']? I feel that should be the case. Each representation would then be required to have context and template which would be all that is need to fill the path for loading it.

tokejepsen pushed a commit to tokejepsen/core that referenced this issue Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api document-this Needs to be documented. enhancement
Projects
None yet
Development

No branches or pull requests

4 participants