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

Nuke: simplified deadline submission on write node - AY-974 #314

Conversation

tokejepsen
Copy link
Member

@tokejepsen tokejepsen commented Mar 28, 2024

Changelog Description

This adds a Render Farm button to the instance node, next to the Render Local.

To use as much existing code as possible, we publish headless to submit to the farm with the instance from the node and the workfile. There are some slight hacking to point the job submission to a timestamped copy of the workfile.

Testing notes:

  1. In the settings ayon+settings://nuke/create/CreateWriteRender/instance_attributes add Render On Farm attribute.
  2. Create write instance in Nuke.
  3. Hit the Render Farm buttton.
  4. Validate job is submitted to Deadline and renders correctly.

@tokejepsen tokejepsen added the sponsored This is directly sponsored by a client or community member label Mar 28, 2024
@ynbot ynbot added type: enhancement Improvement of existing functionality or minor addition host: Nuke size/S labels Mar 28, 2024
Copy link
Member

@moonyuet moonyuet left a comment

Choose a reason for hiding this comment

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

The button shows up in the write instance:)
image

and submit to render farm successfully
image
If my webservice for deadline has gone, it raise the error as expected too.
image

@jakubjezek001
Copy link
Member

jakubjezek001 commented Apr 3, 2024

The button shows up in the write instance:) image

and submit to render farm successfully image If my webservice for deadline has gone, it raise the error as expected too. image

@tokejepsen could we add some better UX communication in case the sanity check for the connection is failing?

Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

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

The code looks solid and feature was working for me. I would add some webservice sanity check and button on demand and then it is good to be merged. Thanks @tokejepsen for the speed!

client/ayon_core/hosts/nuke/api/lib.py Outdated Show resolved Hide resolved
@tokejepsen
Copy link
Member Author

could we add some better UX communication in case the sanity check for the connection is failing?

@jakubjezek001 could you elaborate on the what you want to happen? The error for webservice should not happen if setup correctly, so should not really be artists facing.

@kjpoli
Copy link

kjpoli commented Apr 3, 2024

This matches pretty closely the functionality we have at Hornet, but retains the paradigm of having a publish for every submission if i'm understanding the call to util.integrate in context here, I should have time to test tomorrow, but is this process blocking for Nuke ?. The UX is more or less exactly what we have and was hugely popular with the compers.

client/ayon_core/hosts/nuke/api/utils.py Outdated Show resolved Hide resolved
client/ayon_core/hosts/nuke/api/utils.py Outdated Show resolved Hide resolved
client/ayon_core/hosts/nuke/api/utils.py Outdated Show resolved Hide resolved
client/ayon_core/hosts/nuke/api/utils.py Outdated Show resolved Hide resolved
os.path.basename(base), formatted_timestamp, ext
)
path = os.path.join(directory, filename).replace("\\", "/")
context.data["currentFile"] = path
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to hijack the publishing logic this much? This looks like an anti-pattern?

Copy link
Member Author

Choose a reason for hiding this comment

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

Got any suggestions to improve it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like:

  • Collect instances
  • Find the relevant instance
  • Set the relevant instance attribute value (toggle the active state, etc.)
  • Continue publish with only that instance active

Which would be along the lines of:

# pseudocode
create_context = CreateContext(host)
for instance in create_context.instances:
    instance.data["active"] = is_the_relevant_intsance(instance)
    instance.data["render_mode"] = "farm"  # or whatever the attribute definition and value is
try:
    create_context.publish()
except PublishFailed:
    report = create_context.get_report()
    show_message_dialog("Publish failed", report, level="critical")

This should essentially all be handled by 'default publishing logic' - we just want to trigger it simply for this one instance, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

The client has requested to have a timestamped workfile instead of rendering from the current workfile, hence the hijacked publishing process.

Copy link
Collaborator

@BigRoy BigRoy Apr 12, 2024

Choose a reason for hiding this comment

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

The client has requested to have a timestamped workfile instead of rendering from the current workfile, hence the hijacked publishing process.

I mean - that sounds just as useful for any farm submission. Why not implement it in the regular publishing logic?

Even more so - wasn't there like tons of logic already that allowed farm rendering to basically render from a published workfile which in essence is exactly that - a 'stamped' static version for the farm? (Timestamp is then the publish datetime, etc.) - I know Maya has that and uses it e.g. for the submissiosn to the farm.

This client request and how it's implemented here is then even worse than I thought. This feels like something to take back to the drawing board and simplify across the board. This doesn't sound like a problem unique to this particular Nuke submission - but to any farm submission.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even more so - wasn't there like tons of logic already that allowed farm rendering to basically render from a published workfile which in essence is exactly that - a 'stamped' static version for the farm? (Timestamp is then the publish datetime, etc.) - I know Maya has that and uses it e.g. for the submissiosn to the farm.

Client does not want a published workfile to clutter the database.

I guess we could have the whole thing using targets in Pyblish, which then would gets a custom extraction/integration, but that wont use a lot of the existing code.

@jakubjezek001 could you weigh in here?

Copy link
Collaborator

@BigRoy BigRoy Apr 12, 2024

Choose a reason for hiding this comment

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

Client does not want a published workfile to clutter the database.

This feels quite related to this Ynput Community forum discussion on not publishing directly, but first making a 'reviewable' first.

Anyway, since your logic is still not removing the workfile instance, nor removing it - isn't it still being published?

Also, this very much feels like a very specific client request - which may, as it stands, not be mergable into the main codebase if their workflow needs to deviate this much?

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway, since your logic is still not removing the workfile instance, nor removing it - isn't it still being published?

True, did not notice that. The publishing does not actually publish the workfile when testing, and I dont know why, but I've deactivated the instance just in case.

@tokejepsen
Copy link
Member Author

but is this process blocking for Nuke ?

@kjpoli yes, the processing of the submission is blocking Nuke, but should not take long to process since you are just copying the workfile and submitting that.

@tokejepsen tokejepsen assigned BigRoy and iLLiCiTiT and unassigned jakubjezek001 Apr 12, 2024
@@ -142,3 +149,150 @@ def is_headless():
bool: headless
"""
return QtWidgets.QApplication.instance() is None


def create_error_report(context):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's too bad this is in nuke.api.utils - this is really something that should be "easy" to retrieve from the publishing API and should not need a 'nuke specific implementation' I would say.

Anyway, docstring would be welcome.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Could this be at least renamed to create_publish_error_report?

Copy link
Member

Choose a reason for hiding this comment

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

👆

@fabiaserra
Copy link
Contributor

I really don't get what this is doing. Is the Render farm also doing a headless publish or just rendering out?

Our integration of OP in Nuke separates very clearly the rendering vs the publishing, on the write node we only expose the options to streamline the render and write either locally or in the farm (completely split from any publish process), like this screenshot shows: https://community.ynput.io/t/nuke-creators-as-nodes/124/4?u=fabiaserra for the Deadline submission to use a copy of the script and not the workfile you simply need to avoid passing the SceneFile arg in the plugin info and it will do the copy automatically.

And then to publish a write node directly we have a util command they can run by simply selecting the write node and typing Publish, running the following code:

def publish():
    """Publish utility to publish selected Write node"""
    
    write_selected_nodes = [
        selected_nodes for selected_nodes in nuke.selectedNodes()
        if selected_nodes.Class() == "Write"
    ]
    if not write_selected_nodes:
        nuke.message("You must select a write node")
        return
    # For now it's been decided to limit the publish to only a single Write node
    # at a time but if in the future we decide we want to be able to publish
    # all selected write nodes it would simply be matter of removing this
    elif not len(write_selected_nodes) == 1:
        nuke.message("You must select only one write node")
        return

    user_input = PublishPanel()
    submission_notes = None

    # Until user sets submission notes it keeps prompting the dialog unless they
    # click cancel
    while not submission_notes:
        if not user_input.showModalDialog():
            return

        submission_notes = user_input.submission_notes_knob.value()
        if not submission_notes:
            nuke.message("You must enter submission notes before publishing")

    host = registered_host()
    create_context = create.CreateContext(host, reset=True)
    instance_exists = False
    for create_instance in create_context.instances:
        node = create_instance.transient_data["node"]
        if create_instance.family == "workfile":
            create_instance["active"] = True
            data_to_update = {
                "publish_attributes": {
                    "ValidateVersion": {"active": False}
                }
            }
            api.update_node_data(node, "publish_instance", data_to_update)
            log.info(api.get_node_data(node, "publish_instance"))
            continue
        
        if node in write_selected_nodes:
            instance_exists = True
            create_instance["active"] = True
            data_to_update = {
                # Remove comment from any instance. Ensure comment is from context
                "comment": "",
                "version": int(node["ver"].value()),
                "creator_attributes": {
                    "render_target": "frames_farm",
                    "review": True
                }
            }
            api.update_node_data(node, "publish_instance", data_to_update)
        else:
            create_instance["active"] = False

    if not instance_exists:
        nuke.message("Couldn't find publish instance on selected write node")
        return

    create_context.save_changes()

    main_window = lib.get_main_window()
    publisher_window = host_tools.get_tool_by_name(
        tool_name="publisher", parent=main_window
    )
    publisher_window.show_and_publish(submission_notes)

It would be very straightforward to add this command as a button directly on the Write node. What am I missing? It really is not beneficial to tie rendering and publishing together as I have said multiple times here and in Houdini discussions because you really want to reduce the issues that arise when rendering and adding Ayon to the mix just creates more issues so I hope this is not doing that?

@tokejepsen
Copy link
Member Author

I really don't get what this is doing. Is the Render farm also doing a headless publish or just rendering out?

@fabiaserra the Render farm button does a headless publish of a timestamped workfile, to re-use as much as possible of the publishing code for farm submission. The job on the farm is only the Nuke render job. Nothing gets published.

And then to publish a write node directly we have a util command they can run by simply selecting the write node and typing Publish, running the following code:

@fabiaserra thanks for the code! This is doing almost the same thing, but without showing the publishing window. Just error messages if there is any during "publishing" (in quotes here, cause we are not actually publishing anything) or a message if the submission was successful.

@fabiaserra
Copy link
Contributor

@fabiaserra the Render farm button does a headless publish of a timestamped workfile, to re-use as much as possible of the publishing code for farm submission. The job on the farm is only the Nuke render job. Nothing gets published.

How is that not contradictory? hahah you say the button does a headless publish but it doesn't do any publish? Please explain

@fabiaserra thanks for the code! This is doing almost the same thing, but without showing the publishing window. Just error messages if there is any during "publishing" (in quotes here, cause we are not actually publishing anything) or a message if the submission was successful.

You are just reusing the publish code to do the render submission but skip the publish dependency task?

@BigRoy
Copy link
Collaborator

BigRoy commented Apr 12, 2024

button does a headless publish of a timestamped workfile, to re-use as much as possible of the publishing code for farm submission. The job on the farm is only the Nuke render job. Nothing gets published.

This really sounds like they just want a button that does:

  • Save my file as a timestamped filename
  • Submit to deadline for rendering
    (And that is all)?

@jakubjezek001 jakubjezek001 requested a review from BigRoy May 17, 2024 11:18
@jakubjezek001
Copy link
Member

@tokejepsen can we finalize this PR. It seems that it would be good to merge after some cosmetic suggestions are applied.

@iLLiCiTiT
Copy link
Member

I have nasty question, why it is named "headless_farm". It is combination of 2 words that might be misleading as I would expect it will create remote publish job (run publishing of the workfile on farm), but it just renders the output. Could we rename it to e.g. "render_on_farm"?

@fabiaserra
Copy link
Contributor

I have nasty question, why it is named "headless_farm". It is combination of 2 words that might be misleading as I would expect it will create remote publish job (run publishing of the workfile on farm), but it just renders the output. Could we rename it to e.g. "render_on_farm"?

This is not a nasty question at all, this is something that I raised already in the very beginning of this PR #314 (comment) on how confusing and convoluted the implementation of this is. Sorry to play devil's advocate (again), you can ignore me if you want but please hear me out, the fact that this PR is still going and the numerous comments, validate my initial point. You are trying to fit a solution into a framework that wasn't designed for this and we will keep suffering from it and wasting more and more time unless you pivot and re-design the framework to be not only for publishing but having separate APIs. Deadline submissions are something that's already been solved a long time ago and it works in most of the DCCs that it's supported for, we don't need to reinvent the wheel and add more technical debt to the codebase. It's the exact same issue you are having in Houdini and that I have raised quite a few times, you are trying to hijack the publish system to do rendering and do just a subset of the things you can do already with vanilla Deadline implementation.

You could argue that you'd want to run the same validation plugins that you run during publish during the render submission but then just abstract the validation parts of the framework so you can run them before submitting the render to Deadline using an abstracted API that's just for that and don't make the system even more confusing.

The argument you said for the reason to do it this way was to do it for "simplicity" and to have something ready before you have resources to do it properly... but the fact that this PR has been opened for 2 months prove that's not simple and quick!! Are we going to keep creating PRs such as this to support deadline submission on every publish type? Studios can already benefit from all the things supported by Deadline without us having to provide it. You did the same for point caches in Houdini (and I already raised this issue then ynput/OpenPype#4903), I did it myself when I had to spend quite a bit of time to be able to support splitting extraction and rendering for render submissions in Houdini (ynput/OpenPype#5420) - which is when I realized it was pointless to do all of that work if Deadline already had it working for that and a lot of other things it would take me quite a long time to do the same for in OP - and we are doing it here... All of these things work in Deadline, please take advantage of it!!!

tokejepsen and others added 7 commits May 23, 2024 16:16
…arm.py

Co-authored-by: Jakub Trllo <43494761+iLLiCiTiT@users.noreply.github.com>
Co-authored-by: Jakub Trllo <43494761+iLLiCiTiT@users.noreply.github.com>
…arm.py

Co-authored-by: Jakub Trllo <43494761+iLLiCiTiT@users.noreply.github.com>
@tokejepsen tokejepsen requested a review from iLLiCiTiT May 23, 2024 21:41
@tokejepsen
Copy link
Member Author

Could we rename it to e.g. "render_on_farm"?

@iLLiCiTiT done

@dee-ynput
Copy link

Hello Fabia :)
As we said previously, we're quite on the same page.
Your concerns are being addressed with our work on the Houdini side, and will probably trigger implementation of APIs as you, and I, recommend.
Gradual adoption of those APIs will come later on, per host most probably.

We're doing it at the pace and in the order dictated by client needs. Not only because it is how it's funded but also because it's a better strategy than changing everything at once.

Technical debt isn't solved by disrupting the system but by making it evolve...

Thank you for the passion you put in this. It helps validate our intentions. But I do hope you will come to understand that we have very different constraints as a multi studio technology than what each studio has in isolation :)

Refactored render submission process to handle plugins differently, bypassing version validation and incrementing by removing specific plugins from the list.
@jakubjezek001 jakubjezek001 merged commit 39717a1 into develop May 24, 2024
3 checks passed
@jakubjezek001 jakubjezek001 deleted the enhancement/AY-974_Nuke-simplified-deadline-submission-on-write-node branch May 24, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host: Nuke size/S sponsored This is directly sponsored by a client or community member type: enhancement Improvement of existing functionality or minor addition
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants