-
Notifications
You must be signed in to change notification settings - Fork 21
feat: Run pre-GUI hooks in the VRED render submitter #148
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
Open
leon-li-inspire
wants to merge
4
commits into
aws-deadline:mainline
Choose a base branch
from
leon-li-inspire:feature/pre-gui-hook
base: mainline
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
e7f7513
feat: Run pre-GUI hooks in the VRED render submitter
leon-li-inspire cc5dcb3
Merge branch 'mainline' into feature/pre-gui-hook
leon-li-inspire 659c81a
test: cover _create_submitter_dialog pre-GUI wiring; fix version in d…
leon-li-inspire 65be1a5
refactor: Address PR review on the VRED pre-GUI hook integration
leon-li-inspire File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,208 @@ | ||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
|
|
||
| """Unit tests for the VRED submitter's pre-GUI hook integration. | ||
|
|
||
| ``VREDSubmitter._create_submitter_dialog`` calls deadline-cloud's ``run_pre_gui_hooks`` | ||
| (env-only, since VRED has no on-disk bundle) and then maps the merged output onto its own | ||
| ``RenderSubmitterUISettings`` + the dialog's shared parameter values via deadline-cloud's | ||
| generic ``apply_pre_gui_output``. | ||
|
|
||
| The full submitter needs a running VRED and Qt, so it is exercised in the integration suite. | ||
| This module covers the DCC-owned pieces at unit level: | ||
|
|
||
| * ``TestApplyPreGuiOutputForVred`` pins the contract that matters for VRED: its | ||
| ``RenderSubmitterUISettings`` has assignable ``name`` / ``description`` and **no** | ||
| ``.parameters`` list, so ``apply_pre_gui_output`` must write name/description onto the settings | ||
| and route every hook parameter into the shared values dict. It drives the real core function | ||
| against the real settings dataclass rather than re-testing core internals. | ||
| * ``TestPreGuiHookConfirmCallback`` covers ``_pre_gui_hook_confirm_callback`` — the | ||
| ``settings.auto_accept`` branch — including that the returned callback actually fires the Qt | ||
| confirmation dialog (via the real ``qt_hook_confirmation``) when auto_accept is disabled. | ||
| * ``TestCreateSubmitterDialogPreGuiWiring`` covers the wiring added to | ||
| ``_create_submitter_dialog`` — the ``PreGuiHookContext`` construction and that | ||
| ``apply_pre_gui_output`` is applied onto ``render_settings`` / the shared values before the | ||
| dialog is built — with deadline-cloud mocked. | ||
|
|
||
| ``apply_pre_gui_output`` first ships in deadline-cloud 0.60.1 (the floor this change sets); it is | ||
| absent from 0.60.0. | ||
| """ | ||
|
|
||
| from unittest.mock import Mock, patch | ||
|
|
||
| from deadline.client.ui.pre_gui_hooks import apply_pre_gui_output | ||
|
|
||
| from vred_submitter.data_classes import RenderSubmitterUISettings | ||
| from vred_submitter.vred_submitter import _pre_gui_hook_confirm_callback, VREDSubmitter | ||
|
|
||
|
|
||
| def _settings() -> RenderSubmitterUISettings: | ||
| s = RenderSubmitterUISettings() | ||
| s.name = "Original" | ||
| s.description = "" | ||
| return s | ||
|
|
||
|
|
||
| class TestApplyPreGuiOutputForVred: | ||
| """The generic core ``apply_pre_gui_output`` driven against VRED's real settings dataclass.""" | ||
|
|
||
| def test_settings_dataclass_has_no_parameters_list(self): | ||
| """The premise for VRED's mapping: RenderSubmitterUISettings has no .parameters list, so | ||
| apply_pre_gui_output treats every hook parameter as a shared value.""" | ||
| assert not hasattr(RenderSubmitterUISettings(), "parameters") | ||
|
|
||
| def test_name_and_description_applied_to_settings(self): | ||
| """A hook's name/description overwrite the settings fields (VRED has no .parameters list, | ||
| so these land directly on the dataclass).""" | ||
| settings = _settings() | ||
| shared = {"CondaPackages": "vredcore=2024*"} | ||
|
|
||
| apply_pre_gui_output( | ||
| {"name": "PREGUI RAN", "description": "from pipeline"}, settings, shared | ||
| ) | ||
|
|
||
| assert settings.name == "PREGUI RAN" | ||
| assert settings.description == "from pipeline" | ||
|
|
||
| def test_hook_parameters_merged_into_shared_values(self): | ||
| """With no template-parameter list, all hook parameters (queue params, deadline: | ||
| properties) flow into the shared values the dialog is seeded with, overriding defaults on | ||
| key collision.""" | ||
| settings = _settings() | ||
| shared = {"CondaPackages": "vredcore=2024*", "CondaChannels": "deadline-cloud"} | ||
|
|
||
| apply_pre_gui_output( | ||
| { | ||
| "parameters": { | ||
| "deadline:priority": 88, | ||
| "CondaPackages": "vredcore=2025* custom_pkg", # overrides the default | ||
| } | ||
| }, | ||
| settings, | ||
| shared, | ||
| ) | ||
|
|
||
| assert shared["deadline:priority"] == 88 | ||
| assert shared["CondaPackages"] == "vredcore=2025* custom_pkg" | ||
| assert shared["CondaChannels"] == "deadline-cloud" # untouched keys preserved | ||
|
|
||
| def test_empty_output_is_a_noop(self): | ||
| """No pre-GUI hook output leaves the settings and shared values unchanged.""" | ||
| settings = _settings() | ||
| shared = {"CondaPackages": "pkg"} | ||
|
|
||
| apply_pre_gui_output({}, settings, shared) | ||
|
|
||
| assert settings.name == "Original" | ||
| assert settings.description == "" | ||
| assert shared == {"CondaPackages": "pkg"} | ||
|
|
||
| def test_partial_output_only_touches_present_keys(self): | ||
| """Only the keys present in the output are applied; others keep their prior values.""" | ||
| settings = _settings() | ||
| settings.description = "keep me" | ||
| shared: dict = {} | ||
|
|
||
| apply_pre_gui_output({"name": "NewName"}, settings, shared) | ||
|
|
||
| assert settings.name == "NewName" | ||
| assert settings.description == "keep me" # not overwritten | ||
| assert shared == {} # no parameters in output | ||
|
|
||
|
|
||
| # Patch targets live on the submitter module, since it imports these names directly. | ||
| _MOD = "vred_submitter.vred_submitter" | ||
|
|
||
|
|
||
| class TestPreGuiHookConfirmCallback: | ||
| """The ``settings.auto_accept`` branch in ``_pre_gui_hook_confirm_callback``.""" | ||
|
|
||
| @patch(f"{_MOD}.get_setting", return_value="true") | ||
| def test_none_when_auto_accept_enabled(self, mock_get_setting): | ||
| """With settings.auto_accept enabled, hooks run without a confirmation prompt.""" | ||
| assert _pre_gui_hook_confirm_callback(parent=None) is None | ||
| mock_get_setting.assert_called_once_with("settings.auto_accept") | ||
|
|
||
| @patch("qtpy.QtWidgets.QMessageBox") | ||
| @patch(f"{_MOD}.get_setting", return_value="false") | ||
| def test_dialog_fires_when_auto_accept_disabled(self, mock_get_setting, mock_msgbox): | ||
| """With settings.auto_accept disabled, invoking the returned callback actually shows the | ||
| confirmation dialog (QMessageBox.question), parented to the passed-in window. | ||
|
|
||
| This exercises the real ``qt_hook_confirmation`` callback rather than mocking it out, so it | ||
| verifies the prompt fires — not merely that a non-None callback was selected. | ||
| ``run_pre_gui_hooks`` invokes ``confirm_callback(sources)`` with the hook sources; an empty | ||
| list is enough to reach the dialog. The user's answer maps from the QMessageBox reply. | ||
| """ | ||
| mock_msgbox.question.return_value = mock_msgbox.Yes | ||
|
|
||
| callback = _pre_gui_hook_confirm_callback(parent="mainwin") | ||
| assert callback is not None | ||
|
|
||
| result = callback([]) # no hook sources needed to reach the dialog | ||
|
|
||
| assert mock_msgbox.question.call_count == 1 | ||
| # The dialog is parented to the window passed into the submitter. | ||
| assert mock_msgbox.question.call_args[0][0] == "mainwin" | ||
| # "Yes" reply → proceed. | ||
| assert result is True | ||
|
|
||
|
|
||
| @patch(f"{_MOD}.SubmitJobToDeadlineDialog") | ||
| @patch(f"{_MOD}.get_dpi_scale_factor", return_value=1.0) | ||
| @patch(f"{_MOD}.get_major_version", return_value="2024") | ||
| @patch(f"{_MOD}.os.getenv", return_value=None) | ||
| @patch(f"{_MOD}.apply_pre_gui_output") | ||
| @patch(f"{_MOD}.run_pre_gui_hooks", return_value={}) | ||
| @patch(f"{_MOD}._pre_gui_hook_confirm_callback") | ||
| class TestCreateSubmitterDialogPreGuiWiring: | ||
| """Unit coverage for the pre-GUI wiring in ``_create_submitter_dialog``. | ||
|
|
||
| deadline-cloud is mocked (as in conftest), so these tests pin the DCC-owned wiring — the | ||
| ``PreGuiHookContext`` build, the confirm-callback selection delegated to | ||
| ``_pre_gui_hook_confirm_callback``, and applying the merged output — that | ||
| ``TestApplyPreGuiOutputForVred`` / ``TestPreGuiHookConfirmCallback`` and the integration suite | ||
| don't cover. | ||
| """ | ||
|
|
||
| @staticmethod | ||
| def _submitter(): | ||
| with patch(f"{_MOD}.get_yaml_contents", return_value={"steps": []}): | ||
| return VREDSubmitter(Mock()) | ||
|
|
||
| def test_hooks_run_and_output_applied_before_dialog( | ||
| self, | ||
| mock_confirm_cb, | ||
| mock_run_hooks, | ||
| mock_apply, | ||
| mock_getenv, | ||
| mock_version, | ||
| mock_dpi, | ||
| mock_dialog, | ||
| ): | ||
| """run_pre_gui_hooks is invoked with a VRED PreGuiHookContext and the confirm callback from | ||
| _pre_gui_hook_confirm_callback; its output is applied onto the settings + shared values via | ||
| apply_pre_gui_output before the dialog is built.""" | ||
| mock_run_hooks.return_value = {"name": "FromHook"} | ||
| settings = _settings() | ||
|
|
||
| submitter = self._submitter() | ||
| submitter._create_submitter_dialog(settings, (Mock(), Mock())) | ||
|
|
||
| mock_run_hooks.assert_called_once() | ||
| context = mock_run_hooks.call_args.args[0] | ||
| assert context.bundle_dir is None # VRED has no on-disk bundle at pre-GUI time | ||
| assert context.submitter_name == "vred" | ||
| assert context.job_name == settings.name | ||
| assert context.parameters["CondaPackages"] == "vredcore=2024*" | ||
|
|
||
| # The confirm callback is delegated to the helper, parented to the submitter's window. | ||
| mock_confirm_cb.assert_called_once_with(submitter.parent_window) | ||
| assert mock_run_hooks.call_args.kwargs["confirm_callback"] is mock_confirm_cb.return_value | ||
|
|
||
| # The merged output is applied onto the same settings + shared-values the dialog receives. | ||
| mock_apply.assert_called_once() | ||
| applied_output, applied_settings, applied_shared = mock_apply.call_args.args | ||
| assert applied_output == {"name": "FromHook"} | ||
| assert applied_settings is settings | ||
| seeded_shared = mock_dialog.call_args.kwargs["initial_shared_parameter_values"] | ||
| assert applied_shared is seeded_shared |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
From the 3dsmax version, it seems we need to add a try-catch?
Uh oh!
There was an error while loading. Please reload this page.
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.
From the Github scan bot on the 3dsmax PR: aws-deadline/deadline-cloud-for-3ds-max#262
When a user declines the hook confirmation prompt, run_pre_gui_hooks does not return an empty result — it raises DeadlineOperationCanceled (see deadline-cloud pre_gui_hooks.run_pre_gui_hooks: if confirm_callback is not None and not confirm_callback(sources): raise _DeadlineOperationCanceled(...)).
This call is at the top level of show_job_bundle_submitter, and neither this function nor its callers (run_ui.show_ui → show_job_bundle_submitter()) wrap it in a try/except. So a user simply clicking No on the confirmation dialog will propagate an unhandled exception up into 3ds Max, surfacing as an error/traceback rather than quietly aborting the submitter open.
Consider catching DeadlineOperationCanceled around the run_pre_gui_hooks call and returning early (no dialog), matching the "user canceled" intent. The same wrap would also contain any exception raised from within a studio hook script during execute_pre_gui_hooks.