-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: Allow pre-submit and post-submit callbacks #292
base: mainline
Are you sure you want to change the base?
Conversation
5bbdb14
to
cd5eef4
Compare
Hey Alex, thanks for the PR! Your change looks good to me, but we'll need a tiny change to your commit/PR title to proceed. We're using Semantic PRs for some things like the changelogs, which means the commit messages need to follow https://www.conventionalcommits.org/. I think for this PR that means adding |
4913566
to
febec4d
Compare
Thanks for the PR - I like the idea a lot! Just a few comments.
If I'm understanding this correctly, we should be adding a test around the ability for the pre_submit callback to modify how the job gets created in on_create. As it is, the options can be inspected, but only some of the options are modifiable. Do we want all of them to be modifiable? ie. complete control? |
job_history_bundle_dir, | ||
settings, | ||
queue_parameters, | ||
asset_references, | ||
purpose=JobBundlePurpose.SUBMISSION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the intent here is to match what we pass into on_create_job_bundle_callback
to give users full control before the job bundle gets created. So we should be including requirements
here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct about that, I grabbed the on_create_job_bundle_callback signature from the else block.
I can move it below where requirements
is defined.
if self.on_pre_submit_callback: | ||
self.on_pre_submit_callback( | ||
self, | ||
job_history_bundle_dir, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't initialized until a little bit further down, so it's currently undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move the pre_submit callback lower down so that the requirements are included and have it within the "show_host_requirements_tab" branch if that is recommended to be included.
Regarding what can be modified. My specific use cases I don't think need any modified, but I could see use for changing the following:
The If you think that those other parameters will not be needed in an on submit hook, then I can modify the signature. Regarding the test to see what was modified. Are you referring to a unittest type test or a check to determine if any values were modified and then respond accordingly if they were? If it is the latter I would be interested to know which you are most concerned about. |
012ae84
to
23b55e3
Compare
I've added on to this implementation a little bit to help in my implementation of deadline-cloud-for-nuke HERE The changes are around my tests and the new |
Signed-off-by: Alex Hughes <alex@archpt.io> This will allow passing a pre/post submit callback into the deadline callback which would allow for customization and validation of scenes Signed-off-by: Alex Hughes <ahughesalex@gmail.com> Signed-off-by: Automatic Software Release Bot <autobot@archpt.io> Signed-off-by: Alex Hughes <ahughesalex@gmail.com>
Signed-off-by: Automatic Software Release Bot <autobot@archpt.io> Signed-off-by: Alex Hughes <ahughesalex@gmail.com>
…packages This implementation will be used by DCC packages such as "deadline-cloud-for-nuke" to load and pass callback functions to "deadline-cloud" to execute. Signed-off-by: Alex Hughes <ahughesalex@gmail.com>
This implements `validate_function_signature` which takes a loaded function and compares it's signature to an OrderedDict of parameters. By default this will compare it to the signature of `_reference_callback_signature`. We've specified the `settings` parameter as an object type instead of any specific settings type and then we compare the bases of the passed arguments and determine if it is contained in the class hierarchy. Signed-off-by: Alex Hughes <ahughesalex@gmail.com>
Signed-off-by: Alex Hughes <ahughesalex@gmail.com>
b3c623e
to
7b7893a
Compare
One thing I've been thinking about is the ability to run code before job creation already exists. An application already passes in an on_create_job_bundle_callback which runs immediately after the proposed pre_submit callback. So an application can just add whatever it wants to its on_create, either directly to the function or something like a decorator. The post-submit is a little more difficult since it's not about the creation of the job bundle, but the creation of the job in the service which isn't currently exposed. Though based on the identified use cases of pre_create and post_submit, it seems like pre_create is currently more desired. Based on this and the new PR in the Nuke repository, it seems like the desire is more generally centered around customization of existing applications instead of forking/creating brand new ones. A few ideas off the top of my head of things we may want to provide customization on include, but are not limited to: create/embed custom gui controls, pre-populating values/overriding certain sticky settings, translate assets into openjd compatible ones, add custom steps/modify existing ones, etc. I suggest that we create a feature issue and discuss the goals and use-cases there to ensure that we arrive at the right solution before defining a new interface to maintain. Thoughts? |
Hey @epmog
As far as
In my main specific use case, I need to "revert" the scene to the state it was in pre submission. We're stripping doing things like converting SGTK write nodes to normal write nodes, and removing unused plugins (disconnected but still in the scene will cause the Nuke process to load and license them).
Agreed, I feel those are good potential use cases. I could see some sort of "pre-UI" step that allows for additional UI widgets (potentially), setting or overriding settings, and translation of assets. The adding/modifying steps is an interesting idea, it may be difficult to define an interface that makes that easy for studios, perhaps we pass in the list of Python object representations of the job we're submitting and then receive a list of potentially modified ones back? I think the functionality that I require (modifying the scene and reverting it) would work within that framework of a "pre/post UI" step. I may create a fork of this for my clients in the meantime so that I can implement my dirty environment variable hooks to do my scene modifications but I would love to contribute to how this interface will be built. Thanks for your thoughtful response and questions |
What was the problem/requirement? (What/Why)
When working in a studio, we often want to execute local validation of scene files before we allow a submission to the render farm. In other scenarios we may need to modify or transform the scene to prepare if for the render farm.
These two hooks allow for studio specified modification hooks to be optionally run before and after a submission completes.
What was the solution? (How)
This solution implements an
on_pre_submit_callback
and aon_post_submit_callback
that take the same parameters as theon_create_job_bundle_callback
.The parameters match
on_create_job_bundle_callback
solely to provide flexibility in case the hook needs access to any of the settings of the job.What is the impact of this change?
The impact of this change as it stands would be non-breaking. I would consider adding functionality in the deadline-cloud-for-XX adaptor packages to provide a developer friendly way to provide hooks externally (Out of the scope of this PR probably, but it could be environment variables pointing to hook files, or an import of a pre-determined package name for example)
How was this change tested?
Untested
Was this change documented?
Not currently documented
Is this a breaking change?
No it is a feature addition
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.