Skip to content
This repository was archived by the owner on Sep 20, 2024. It is now read-only.

Conversation

@jakubjezek001
Copy link
Member

Changelog Description

After Nuke had adopted new publisher with new creators new issues were introduced. Those issues were addressed with this PR. Those are for example broken reviewable video files publishing if published via farm. Also fixed local publishing.

Additional info

Global plugins had to be addressed too so please test also other DCCs if those are still working - talking of AfterEffects, Maya mainly.

Testing notes:

  1. Open Nuke workfile and publish render family node to farm.
  2. Ideally test also included slate workflow (you might need to add slate tag to your ExctractReview output preset)
  3. After the publish is done check Ftrack if reviewable video (with slate) was published.

Comment on lines +759 to +761
if not instance.data.get("farm"):
self.log.info("Skipping local instance.")
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this is needed?
Ideally we should be filtering with families, for example render.farm, or at least have a comment about why this is needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is right, new publisher workflow had introduced this requirement. Is there any particular host you know about which is not adding farm key into instance.data?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had just agreed with @antirotor @iLLiCiTiT @kalisp that it should be required for all DCC to collect farm key as True in case it needs to be published on a farm.

Copy link
Collaborator

@BigRoy BigRoy Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I believe @tokejepsen tried to ask is why use an instance.data attribute instead of adding e.g. farm family so that we can explicitly filter the relevant plug-ins to whether it's a farm submission or not instead of always showing the plug-in but then skipping half the instances by comparing the value inside the Plug-ins' process method.

So that'd be:

instance.data["families"].append("farm")

Instead of:

instance.data["farm"] = True

If a particular plug-in still needs to check whether it's marked for farm it could do "farm" in instance.data["families"]


Ah wait, I think I know why. The new publisher can't show instance attribute definitions (like e.g. Deadline Priority, Pool, Chunk Size attributes from e.g. a SubmitJobToDeadline plug-in) if the instance itself before the collectors would not already have the "farm" family. So it would disallow showing settings in the publisher UI based on whether it's marked as farm or not.

Quite a downside of its design I suppose. Seems like an implementation like pyblish/pyblish-qml#356 with Pre-Collect vs Post-Collect would've solved that. The farm setting could then have been detected during pre-collect through plug-ins that are NOT the Creator. Making it much more extendable based on what values are set to on the instance? (The UI would need a refresh after settings influencing pre-collects however).

Anyway, likely this change is to workaround that UX design challenge.

Copy link
Member Author

@jakubjezek001 jakubjezek001 Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding here parallel conversation we had yesterday at discord. This explains a lot and perhaps even addressing @tokejepsen and @BigRoy concerns.

Quoting @iLLiCiTiT from here :

  • I don't think pyblish families would save us here. Family filtering requires only one of defined families on instance which can cause more issues and confusion then explicit check of a value -> plugin filter families = ["render", "farm"] would catch all render instances and all farm instances.
    So a pyblish plugin which would care only about render going to farm would still have to do a check of families in process method, which is at the end more complex than simple check for a single key.
  • Also the families filter is affecting submit publish job which probably should not care about families at all. It's current family filtering can break other hosts if we would add support for same families but without farm support.
  • I understand that the plugins would not be filtered by pyblish logic, so they would show in details and their process would be triggered, but it doesn't mean that family filtering is a good idea at this point.
  • At the same time I'm not saying that "farm" key is the best solution, but I really don't see how families would solve all the side effects

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the issue is with the matching algorithm on Pyblish. Think we can reduce the issue to this test;

from pyblish import api, util


class RenderValidator(api.InstancePlugin):
    order = api.ValidatorOrder
    families = ["render"]
    match = api.Exact

    def process(self, instance):
        instance.data["validationCount"] += 1


class FarmValidator(api.InstancePlugin):
    order = api.ValidatorOrder
    families = ["farm"]
    match = api.Exact

    def process(self, instance):
        instance.data["validationCount"] += 1


class RenderFarmValidator(api.InstancePlugin):
    order = api.ValidatorOrder
    families = ["render", "farm"]
    match = api.Subset

    def process(self, instance):
        instance.data["validationCount"] += 1


plugins = [RenderValidator, FarmValidator, RenderFarmValidator]

context = api.Context()
instance = context.create_instance(
    name="Render", family="render", families=["render"], validationCount=0
)
util.publish(context, plugins)
assert instance.data["validationCount"] == 1

context = api.Context()
instance = context.create_instance(
    name="Farm", family="farm", families=["farm"], validationCount=0
)
util.publish(context, plugins)
assert instance.data["validationCount"] == 1

context = api.Context()
instance = context.create_instance(
    name="RenderFarm", families=["render", "farm"], validationCount=0
)
util.publish(context, plugins)
assert instance.data["validationCount"] == 1

context = api.Context()
instance = context.create_instance(
    name="RenderLayer", families=["layer", "render"], validationCount=0
)
util.publish(context, plugins)
assert instance.data["validationCount"] == 1

Not matter what matching algorithm combination we try here, we will always end up with an assertion error due to missing an exclusion matching algorithm, which might be a good idea to implement in Pyblish?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how would that solve that the plugin should be executed by combination of ["render", "farm"]? Also there already were ideas how to add that to pyblish and I don' think any were successful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how would that solve that the plugin should be executed by combination of ["render", "farm"]? Also there already were ideas how to add that to pyblish and I don' think any were successful.

Not sure. Does the above testing code describe the issue we have?
If so we could use it as basis to come up with ideas to solve the filtering problem.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how would that solve that the plugin should be executed by combination of ["render", "farm"]? Also there already were ideas how to add that to pyblish and I don' think any were successful.

I think this is where things get interesting - because do we have a use case for this where it's the case?

I'd say the logic should be this:

Collecting does this:

  1. The instance family is render.
  2. The instance is set to render locally: render.local is added to families
  3. The instance is set to render om farm: render.farm is added to families.

From that moment onwards it becomes easier:

  • Now I want to run a plug-in for any render, either local or on farm I target it to render.
  • I want to run a plug-in for any local render: render.local
  • I want to run a plug-in for any farm render: render.farm

We can now explicitly target any of the combinations, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's reasonable for filtering of families but still some plugins must be able to find out if instance will go to farm or not.

Now practicalities. We need to merge this because it is just broken now and we definetely won't do suggested changes it in this PR. Moving the conversation here https://github.com/ynput/OpenPype/discussions/4663 please. We have to also care about cases when there is more than one farm addon + some changes related to Publisher attributes + families cleanup (exactly the conversation here).

Copy link
Member

@kalisp kalisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested seems to work fine (even in Maya, AE)

@kalisp
Copy link
Member

kalisp commented Mar 15, 2023

After discussion with Jezza I enhanced details in asserts tests for Nuke. Name of representation looks kinda weird though.
2023-03-15 18_29_30-Testing Platform PC - AnyDesk

@kalisp kalisp assigned jakubjezek001 and unassigned kalisp Mar 15, 2023
@jakubjezek001
Copy link
Member Author

After discussion with Jezza I enhanced details in asserts tests for Nuke. Name of representation looks kinda weird though.

Awesome, thanks a lot @kalisp!

Accidentally used DL version
@jakubjezek001 jakubjezek001 merged commit a2f56b7 into develop Mar 21, 2023
@ynbot ynbot added this to the next-patch milestone Mar 21, 2023
@kalisp kalisp deleted the bugfix/nuke-deadline-submitter branch March 22, 2023 08:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

host: Nuke type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants