-
Notifications
You must be signed in to change notification settings - Fork 38
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
[WIP] Customizing DCC Submitters (UI, settings, hooks, custom steps) #327
base: mainline
Are you sure you want to change the base?
[WIP] Customizing DCC Submitters (UI, settings, hooks, custom steps) #327
Conversation
We also had to update callback_loader to allow generic abstraction around it Signed-off-by: Alex Hughes <[email protected]>
This implements the UI Callback by passing the dialog, initial_job_settings, queue_parameters, asset_references, and host_requirements to it and then updating all settings and attachign the new UI widget. This includes work to implement setters for the settings returned from the UI Callback. This UI Callback actually runs after the UI has been built so I have to modify what is already there. Signed-off-by: Alex Hughes <[email protected]>
… signature Signed-off-by: Alex Hughes <[email protected]>
Signed-off-by: Alex Hughes <[email protected]>
e42db3d
to
cb814c5
Compare
Signed-off-by: Alex Hughes <[email protected]>
Signed-off-by: Alex Hughes <[email protected]>
Signed-off-by: Alex Hughes <[email protected]>
@@ -82,6 +83,8 @@ def __init__( | |||
auto_detected_attachments: AssetReferences, | |||
attachments: AssetReferences, | |||
on_create_job_bundle_callback, | |||
on_ui_callback=None, | |||
on_post_submit_callback=None, |
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 know we started down this road of callbacks being passed in here, but I wonder if these two cases are better served via a plugin structure like what @epmog alluded to in #308 (comment) ?
The repo + blogpost https://github.com/wax911/plugin-architecture looks like a decent high level way to think about plugins - with discovery and registration being the key concepts to think about first. With callbacks via the parameter here, it is the code that is using the deadline-cloud library that has to perform any discovery of plugins available.
For on_post_submit_callback
, does this meet the goals of the callback? If we want to let someone write code that runs on every single job submission, this won't work, because every downstream dependency would need to implement specific code to support that, and other ways of submitting jobs like the CLI deadline bundle submit
don't use these GUI code flows at all.
If we have a "post submit" action that a plugin can register, and within the deadline-cloud library every path to submitting a job runs the plugins (probably with an explicit opt-out --no-plugins
option or similar to help with testing whether a plugin is responsible for an issue or not), maybe that's closer to the extensibility we want?
Signed-off-by: Alex Hughes <[email protected]>
This is a replacement for the existing on_create_job_bundle_callback. This is not how I want it to stay. Ideally this allows adding/modifying steps and parameters on top of the default DCC code. It feels unwieldy to make the callback user reimplement everything just to modify or add a step. Signed-off-by: Alex Hughes <[email protected]>
Signed-off-by: Alex Hughes <[email protected]>
Signed-off-by: Alex Hughes <[email protected]>
|
What was the problem/requirement? (What/Why)
This PR is my WIP implementation to be discussed in #308
What was the solution? (How)
This currently starts to implement the UI Hook, Post Submission Hook, and the remaining JOB_CALLBACK has not been implemented yet.
What is the impact of this change?
This is a major change, however it is intended to be an "opt-in" change where you need to provide callbacks to run, and if none are provided, no change will be noticed.
How was this change tested?
Not at the testing point yet. Will need to write some tests
Was this change documented?
Is this a breaking change?
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.