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

Global | Nuke: fixing farm publishing workflow #4623

Merged
merged 4 commits into from
Mar 21, 2023
Merged

Global | Nuke: fixing farm publishing workflow #4623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24a22e9
Nuke: fix families mixing up with old way.
jakubjezek001 Mar 13, 2023
1e458d6
global and nuke: farm publishing workflow fixes
jakubjezek001 Mar 14, 2023
8449c21
Extended Nuke testing classes with representation details
kalisp Mar 15, 2023
4506a2f
Fix broken Nuke local test
kalisp Mar 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion openpype/hosts/nuke/plugins/publish/extract_review_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def process(self, instance):
# review can be removed since `ProcessSubmittedJobOnFarm` will create
# reviewable representation if needed
if (
"render.farm" in instance.data["families"]
instance.data.get("farm")
and "review" in instance.data["families"]
):
instance.data["families"].remove("review")
Expand Down
7 changes: 6 additions & 1 deletion openpype/hosts/nuke/plugins/publish/extract_review_data_lut.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,12 @@ def process(self, instance):
exporter.stagingDir, exporter.file).replace("\\", "/")
instance.data["representations"] += data["representations"]

if "render.farm" in families:
# review can be removed since `ProcessSubmittedJobOnFarm` will create
# reviewable representation if needed
if (
instance.data.get("farm")
and "review" in instance.data["families"]
):
instance.data["families"].remove("review")

self.log.debug(
Expand Down
5 changes: 1 addition & 4 deletions openpype/hosts/nuke/plugins/publish/extract_review_data_mov.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,7 @@ def process(self, instance):
self, instance, o_name, o_data["extension"],
multiple_presets)

if (
"render.farm" in families or
"prerender.farm" in families
):
if instance.data.get("farm"):
if "review" in instance.data["families"]:
instance.data["families"].remove("review")

Expand Down
2 changes: 1 addition & 1 deletion openpype/hosts/nuke/plugins/publish/extract_thumbnail.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class ExtractThumbnail(publish.Extractor):


def process(self, instance):
if "render.farm" in instance.data["families"]:
if instance.data.get("farm"):
return

with napi.maintained_selection():
Expand Down
10 changes: 7 additions & 3 deletions openpype/modules/deadline/plugins/publish/submit_nuke_deadline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class NukeSubmitDeadline(pyblish.api.InstancePlugin,
label = "Submit Nuke to Deadline"
order = pyblish.api.IntegratorOrder + 0.1
hosts = ["nuke"]
families = ["render", "prerender.farm"]
families = ["render", "prerender"]
optional = True
targets = ["local"]

Expand Down Expand Up @@ -80,6 +80,10 @@ def get_attribute_defs(cls):
]

def process(self, instance):
if not instance.data.get("farm"):
self.log.info("Skipping local instance.")
return

instance.data["attributeValues"] = self.get_attr_values_from_data(
instance.data)

Expand Down Expand Up @@ -168,10 +172,10 @@ def process(self, instance):
resp.json()["_id"])

# redefinition of families
if "render.farm" in families:
if "render" in instance.data["family"]:
instance.data['family'] = 'write'
families.insert(0, "render2d")
elif "prerender.farm" in families:
elif "prerender" in instance.data["family"]:
instance.data['family'] = 'write'
families.insert(0, "prerender")
instance.data["families"] = families
Expand Down
4 changes: 4 additions & 0 deletions openpype/modules/deadline/plugins/publish/submit_publish_job.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,10 @@ def process(self, instance):
instance (pyblish.api.Instance): Instance data.

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

@tokejepsen tokejepsen Mar 14, 2023

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

@jakubjezek001 jakubjezek001 Mar 15, 2023

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

@jakubjezek001 jakubjezek001 Mar 15, 2023

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 •
edited
Loading

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 •
edited
Loading

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

@tokejepsen tokejepsen Mar 18, 2023

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

@iLLiCiTiT iLLiCiTiT Mar 20, 2023

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

@tokejepsen tokejepsen Mar 20, 2023

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

@BigRoy BigRoy Mar 20, 2023

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

@iLLiCiTiT iLLiCiTiT Mar 21, 2023

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).


data = instance.data.copy()
context = instance.context
self.context = context
Expand Down
4 changes: 4 additions & 0 deletions openpype/modules/deadline/plugins/publish/validate_deadline_pools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ class ValidateDeadlinePools(OptionalPyblishPluginMixin,
optional = True

def process(self, instance):
if not instance.data.get("farm"):
self.log.info("Skipping local instance.")
return

# get default deadline webservice url from deadline module
deadline_url = instance.context.data["defaultDeadline"]
self.log.info("deadline_url::{}".format(deadline_url))
Expand Down
18 changes: 18 additions & 0 deletions tests/integration/hosts/nuke/test_deadline_publish_in_nuke.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,30 @@ def test_db_asserts(self, dbcon, publish_finished):
failures.append(
DBAssert.count_of_types(dbcon, "representation", 4))

additional_args = {"context.subset": "workfileTest_task",
"context.ext": "nk"}
failures.append(
DBAssert.count_of_types(dbcon, "representation", 1,
additional_args=additional_args))

additional_args = {"context.subset": "renderTest_taskMain",
"context.ext": "exr"}
failures.append(
DBAssert.count_of_types(dbcon, "representation", 1,
additional_args=additional_args))

additional_args = {"context.subset": "renderTest_taskMain",
"name": "thumbnail"}
failures.append(
DBAssert.count_of_types(dbcon, "representation", 1,
additional_args=additional_args))

additional_args = {"context.subset": "renderTest_taskMain",
"name": "h264_mov"}
failures.append(
DBAssert.count_of_types(dbcon, "representation", 1,
additional_args=additional_args))

assert not any(failures)


Expand Down
20 changes: 19 additions & 1 deletion tests/integration/hosts/nuke/test_publish_in_nuke.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class TestPublishInNuke(NukeLocalPublishTestClass):
!!!
It expects modified path in WriteNode,
use '[python {nuke.script_directory()}]' instead of regular root
dir (eg. instead of `c:/projects/test_project/test_asset/test_task`).
dir (eg. instead of `c:/projects`).
Access file path by selecting WriteNode group, CTRL+Enter, update file
input
!!!
Expand Down Expand Up @@ -70,12 +70,30 @@ def test_db_asserts(self, dbcon, publish_finished):
failures.append(
DBAssert.count_of_types(dbcon, "representation", 4))

additional_args = {"context.subset": "workfileTest_task",
"context.ext": "nk"}
failures.append(
DBAssert.count_of_types(dbcon, "representation", 1,
additional_args=additional_args))

additional_args = {"context.subset": "renderTest_taskMain",
"context.ext": "exr"}
failures.append(
DBAssert.count_of_types(dbcon, "representation", 1,
additional_args=additional_args))

additional_args = {"context.subset": "renderTest_taskMain",
"name": "thumbnail"}
failures.append(
DBAssert.count_of_types(dbcon, "representation", 1,
additional_args=additional_args))

additional_args = {"context.subset": "renderTest_taskMain",
"name": "h264_mov"}
failures.append(
DBAssert.count_of_types(dbcon, "representation", 1,
additional_args=additional_args))

assert not any(failures)


Expand Down