ynput / ayon-houdini

Houdini addon for AYON
Apache License 2.0
11 stars 8 forks source link

Refactor Houdini deadline submission #33

Open moonyuet opened 12 months ago

moonyuet commented 12 months ago

Hello, I'm sorry if this comes out as a bit harsh but I think the approach this PR is taking to support caching in the farm is wrong and over engineered.

First of all, caching in the farm (and rendering or any other Houdini processes) are already supported by third-party toolsets (Deadline, HQueue, Tractor...) and in WAY more powerful ways that this PR tries to accomplish and the OP plugin framework can manage. This is duplicating all of that logic in OP and adding 1,398 more lines to the already super complex code base!! Most Houdini TDs are already familiarized with those vanilla workflows and having them learn this other "black box" approach through OP is backwards and doesn't add any benefit in my opinion. You can see an example of a very normal submission to the farm here https://github.com/ynput/OpenPype/pull/5621#issuecomment-1732166830

OpenPype shouldn't try to orchestrate the extract/render dependencies of the Houdini node graph, that's already done by these schedulers/submitters, we just need means to be able to run OP publish tasks of the generated outputs, but without doing any gimmicks, just taking a path, a family, a few other data inputs and registering it to the DB so it runs the other integrate plugins of OP like publishing that to SG/ftrack as well (and ideally the API for doing that in OP should be super straightforward to call it from anywhere! the current json file input isn't the best access point to that funcionality). If we wanted to help the existing vanilla submission OP could provide a wrapper of the vanilla submitters so it sets some reasonable defaults and we could intersect the submission to run some pre-validations on the graph... set some parms that might be driven by the OP settings or create utility HDAs to facilitate the creation of the submitted graph so frame dependencies are set correctly and chunk sizes for simulations... but that's it, we don't need to reinvent the wheel by interpreting how the graph needs to be evaluated.

On the other hand, I still don't quite get why the existing submit_publish_job is limited to "render" families and why it's not abstracted in a simple reusable module that any plugins that require to submit to Deadline can reuse it. This PR showed how a lot of the code had to be duplicated again with most of the lines exactly the same, doubling in technical debt. This PR https://github.com/ynput/OpenPype/pull/5451/files goes in the right direction in abstracting some of those things but the right approach should be to remove all of the noise from submit publish job that's render specific and make use of the same util module every time we just need to run an OP publish task in the farm. However, as ï said initially, I don't think we should even take this approach for Houdini and we should just leverage the existing farm submitters code, but this is relevant for any other tasks that we choose to submit to the farm, we are going to have to write a "submit__job" set of plugins for each?

Extra notes:

Originally posted by @fabiaserra in https://github.com/ynput/OpenPype/issues/4903#issuecomment-1738343017

[cuID:OP-7455]

MustafaJafar commented 4 months ago

As far as I'm able to tell, The issue description is mentioning refactoring Houdini deadline submission. I did some search for this topic and added it on forums Houdini Future - Vanilla Deadline ROP

I'll just quote it here:

Vanilla Deadline ROP

In order to use the vanilla deadline ROP node. we need to figure a way that only performs the publishing.
@fabiaserra's solution was to implement ax_publisher_submitter that creates a json file and submits an AYON publish job (in compliance with our deadline addon) Also, he override the vanilla rop nodes to add Ayon parameters, similar to https://github.com/ynput/ayon-houdini/pull/2

So, the node network actually looks more like this

More info about it in his demo on Github.

Also, I did quick search about adopting vanilla Deadline ROP and I think such an idea requires extending the Deadline ROP itself which I have no idea how it's achieved.

It seems that deadline rop supports specific standalone plugins, and I couldn't find a way to extend its functionality to support AYON. I wish if there are some exposed callback scripts to make extend it. still need to extend my search. Here are some screenshots with placebo parameters (non-functional 'prototype').

moonyuet commented 4 months ago

Guess this issue should be after https://github.com/ynput/ayon-houdini/pull/2. As I recall from preivous discussion, we should allow setting priorities on which rop nodes to be rendered. And also allows both local and farm rendering.

fabiaserra commented 4 months ago

It seems that deadline rop supports specific standalone plugins, and I couldn't find a way to extend its functionality to support AYON. I wish if there are some exposed callback scripts to make extend it. still need to extend my search. Here are some screenshots with placebo parameters (non-functional 'prototype').

AYON is already supported by Deadline, this is literally the AYON plugin for Deadline https://github.com/ynput/ayon-deadline/tree/develop/client/ayon_deadline/repository/custom/plugins/Ayon

As for customizing Houdini's Deadline integration (i.e. to add env vars so the AYON environment gets injected) it's also pretty easy, you just need to provide your customizations under the custom/submission/Houdini/Main folder. You can also modify the Client code (if you want to do any tweaks to the HDA parms) although that's strictly not necessary for what you want as most of the logic behind the client code happens on the Main code. The main changes you need to do are on the SubmitHoudiniToDeadlineFunctions.py and add these at the SubmitRenderJob function:

                    fileHandle.write("EnvironmentKeyValue0=HIP_=%s\n" % os.getenv("HIP"))
                    # We do this so the GlobalJobPreLoad.py from Deadline injects the OP environment
                    # to the job and it picks up all the correct environment variables
                    ayon_bundle_name = os.getenv("AYON_BUNDLE_NAME")
                    fileHandle.write(
                        "EnvironmentKeyValue1=AYON_RENDER_JOB=1\n" if ayon_bundle_name
                        else "EnvironmentKeyValue1=OPENPYPE_RENDER_JOB=1\n"
                    )
                    fileHandle.write(
                        f"EnvironmentKeyValue2=AYON_PROJECT_NAME={os.getenv('AYON_PROJECT_NAME')}\n" if ayon_bundle_name
                        else f"EnvironmentKeyValue2=AVALON_PROJECT={os.getenv('AVALON_PROJECT')}\n"
                    )
                    fileHandle.write(
                        f"EnvironmentKeyValue3=AYON_FOLDER_PATH={os.getenv('AYON_FOLDER_PATH')}\n" if ayon_bundle_name
                        else f"EnvironmentKeyValue3=AVALON_ASSET={os.getenv('AVALON_ASSET')}\n"
                    )
                    fileHandle.write(
                        f"EnvironmentKeyValue4=AYON_TASK_NAME={os.getenv('AYON_TASK_NAME')}\n" if ayon_bundle_name
                        else f"EnvironmentKeyValue4=AVALON_TASK={os.getenv('AVALON_TASK')}\n"
                    )
                    fileHandle.write(
                        f"EnvironmentKeyValue5=AYON_APP_NAME={os.getenv('AYON_APP_NAME')}\n" if ayon_bundle_name
                        else f"EnvironmentKeyValue5=AVALON_APP_NAME={os.getenv('AVALON_APP_NAME')}\n"
                    )
                    fileHandle.write(
                        f"EnvironmentKeyValue6=AYON_BUNDLE_NAME={ayon_bundle_name}\n" if ayon_bundle_name
                        else f"EnvironmentKeyValue6=OPENPYPE_VERSION={os.getenv('OPENPYPE_VERSION')}\n"
                    )
MustafaJafar commented 3 months ago

Following up the discussion, I think there are two ideas mentioned:

  1. Short Term: Make cache submission more like render submission. this also includes supporting multiple render targets option in the publisher UI. I think this can be a short term goal.
  2. Long Term: Native Houdini deadline submission.

Tagging @BigRoy

dee-ynput commented 2 weeks ago

Hey @antirotor You marked this as "Needs Info". Please call out someone for the aditionnal infos you need 📢 🙂

MustafaJafar commented 2 weeks ago

Hey @antirotor You marked this as "Needs Info". Please call out someone for the aditionnal infos you need 📢 🙂

@dee-ynput @antirotor @BigRoy

As far as I can remember, this issue is related to two topics:

  1. Refactor/merge submit_houdini_render_deadline.py and submit_houdini_cache_deadline.py and avoid duplicating code. This also can be part of bigger issue/epic where we can agree on some standard workflow e.g. we may need to agree if farm instances should have farm family or have farm creator attribute, check this discussion. there's also some scattered logic in Houdini repos that should be moved to deadline repo, check this comment.
  2. Support publishing via native deadline HDA. which can be solved/partially solved by https://github.com/ynput/ayon-houdini/pull/122 .
dee-ynput commented 2 weeks ago

Thanks for the summary @MustafaJafar ✨ I'll mark this as blocked so that we can come back to it once all the discussions you've mentionned are solved 👍