search

ynput / ayon-deadline

Deadline addon for AYON
Apache License 2.0
2 stars 4 forks source link

adding support for {representation} anatomy key #24

Closed timsergeeff closed 2 weeks ago

timsergeeff commented 2 weeks ago

Changelog Description

adding empty string to dictionary TEMLATE DATA [representation] because this key solves later in integration step so for submossion time its not here and need to presist as var but be empty

Testing notes:

to recreate a bug:

  1. add {representation} key to anatomy tempate
  2. try to publish and see it fails
  3. try this PR and ses that it passes and all representations collected in separate folders
BigRoy commented 2 weeks ago

So basically this PR is to support adding {representation} key into the publish file template to have an extra directory in-between per representation?

  • add {representation} key to anatomy tempate
  • try to publish and see it fails

I'm quite confident that if this fixes it that it's not the fix we want. If for whatever reason no valid value was put into the anatomy data down the line - we would basically be formatting it with incorrect data.

Also, there are some plug-ins, like collecting of instance.data["publishDir"] which as far as I know still assume that products of a publish end up as representations next to each other - and hence, there may be way more logic relying on that. E.g. published textures with Maya looks - we may need to 'test' whether it actually applies the resource transfer correctly and correctly remaps them.

Also, for resources that are sometimes collected during publishing like that, those are put into {publishDir}/resources. But what if there are multiple representations, in which of those representations' "publish directories" should those files end up? The logic is very likely undefined.

iLLiCiTiT commented 2 weeks ago

I have to agree with @BigRoy that this does not look as bugfix at correct place, but more like a source of future bug. Maybe rather create issue with more information about the issue you're facing?

timsergeeff commented 2 weeks ago

The problem with that that we remove original for example "redshift_rop" instance and creane new instances "render" per aov and generate representations after so redshift_rop will never get representations and json whitch we use for ayon publisher task dont have anything regardles publsigh folder. All steps of collecting and sorting thins performs in the integrator stage of ayon publish tack itself so at submission time there will not be any representation that we can put here

timsergeeff commented 2 weeks ago

but i see what you mean, this fix will only work if {representation} key is the last in a chain in anatomy template, so agree maybe we need to do something better, i just dont get why are we solving this PATH and never use it in a json?

timsergeeff commented 2 weeks ago

now i've found where we use it... image hmmm so sometime rop node will produse two or more reoresentations and as i see we have only one deadline task for dealing with all of this representations....

timsergeeff commented 2 weeks ago

Guys I think maybe... is it an old piece of code? I'm asking because looks like my "fix" actually breaks an anatomy folder path but it still publishes In right directories because all path stuff hanled after in separate task at integration stage when we launch ayon task in deadline, so do we realy need that ENV at publish time? What the function of it if it still ignored? Maybe can we just get rid of this part of cod where we set outpudirectory0?

BigRoy commented 2 weeks ago

I commented here: https://community.ynput.io/t/deadline-support-for-representation-anatomy-key/1717/2?u=bigroy

The solution required for {representation} key inside the publish template path may require some massive changes across the codebase because there are multiple places where it relies on that not being unique per representation.

Those are only the ones that I could quickly find within a few minutes - there is likely more, but more obscurely hidden away.

iLLiCiTiT commented 2 weeks ago

by marking the representation key as optional. i.e. by writing it in this form </{representation}>.

Well, using </{representation}> will solve @timsergeeff issue, but it is not something I would recommend. This PR is dangerous to be merged, it only hides wrong setup, which is even worse.

As roy mentioned, proper support of representation in publish template folder path would require a lot of changes across the board.

timsergeeff commented 2 weeks ago

Totally agree for dont merging!!! I just don't know that keys could be optional! Thanks and please close this pr!)