fix: connect gui-submit --parameter validation to live queue signal - #1288
fix: connect gui-submit --parameter validation to live queue signal#1288crowecawcaw wants to merge 7 commits into
Conversation
bundle gui-submit --parameter hard-crashed with AttributeError before the dialog could show. show_job_bundle_submitter connected the CLI-parameter validation callback to shared_job_settings._queue_parameters_update, a signal that no longer exists: queue-parameter loading moved to DeadlineUIController, which emits queue_parameters_updated(list). The SharedJobSettingsWidget only exposes parameter_changed / valid_parameters, and its child settings widget exposes selection_changed -- none carry the loaded queue parameters. Connect the callback to shared_job_settings._controller.queue_parameters_updated (the signal the widget itself consumes) and update the callback signature to match its list payload (dropping the stale refresh_id arg from the removed Signal(int, list)). Add a GUI regression test that wires the --parameter path with a real SharedJobSettingsWidget (a MagicMock dialog masked the crash) and asserts no AttributeError plus that emitting queue_parameters_updated runs the validator with the emitted parameter list. Signed-off-by: Stephen Crowe <[email protected]>
github-actions
Bot
added
the
waiting-on-maintainers
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # Connect to the queue parameters update signal | ||
| submitter_dialog.shared_job_settings._queue_parameters_update.connect( | ||
| # Validate CLI params once the controller finishes loading queue params. | ||
| submitter_dialog.shared_job_settings._controller.queue_parameters_updated.connect( |
There was a problem hiding this comment.
This now connects to the global singleton controller (DeadlineUIController.getInstance()) instead of a per-widget signal, which changes the lifecycle and firing semantics in two ways worth checking:
-
Fires more than once, including with an empty list.
queue_parameters_updatedis emitted whenever queue params reload — not just after the initial load. In particular the controller emitsqueue_parameters_updated.emit([])on farm/queue selection changes (select_farm,_deadline_controller.py:547), when no farm/queue is selected (:475), and on fetch error (:509). Each such emission re-runsvalidate_parameters_after_queue_loadwithqueue_parameters=[], so any CLI--parameterthat is actually a queue parameter (not in the job template) gets flagged as "unrecognized," popping the warning dialog spuriously — and closing the whole submitter dialog if the user answers No. A user simply switching farm/queue in the open dialog can trigger this. -
Never disconnected. Because the singleton outlives the dialog, this connection is never torn down.
validate_parameters_after_queue_loadcapturessubmitter_dialogin its closure, so after the dialog is closed the slot keeps firing against a stale/closed dialog (and connections accumulate if multiple submitter dialogs are opened over the singleton's lifetime).
Consider connecting with a single-shot semantics (disconnect after the first non-empty load) or gating on if queue_parameters: / dialog liveness, and/or scoping the connection to the dialog's lifetime.
There was a problem hiding this comment.
The validation callback skips clearing emissions (empty queue_parameters), so farm/queue switches, no-selection, and fetch-error paths do not trigger a spurious "unrecognized parameters" warning. It runs single-shot (disconnects after validating) and is torn down on submitter_dialog.destroyed, so it cannot fire against a stale dialog or accumulate connections on the singleton controller.
…oped The controller is a global singleton, so the previous connection had two lifecycle problems flagged in review: - queue_parameters_updated also fires with [] to clear stale state (farm/ queue switch, fetch error, nothing selected), which would spuriously flag queue parameters as unrecognized and could close the dialog. - The connection was never torn down, so the closure over the dialog kept firing after the dialog closed (and accumulated across dialogs). Now the callback ignores clearing ([]) emissions, disconnects itself after the first real load, and is also disconnected on dialog destruction. Adds tests for empty-emission skip, single-shot behavior, and teardown. Signed-off-by: Stephen Crowe <[email protected]>
| """Validate CLI parameters against loaded queue parameters and set parameter values""" | ||
| if not queue_parameters: | ||
| # A clearing emission, not a completed load. Keep waiting. | ||
| return |
There was a problem hiding this comment.
Treating every empty queue_parameters list as a "clearing emission" means CLI --parameter validation is silently skipped for any queue that genuinely has zero queue parameters. _on_queue_parameters_success emits queue_parameters_updated.emit(parameters) with parameters == [] in that case (see _deadline_controller.py:494-497), which is indistinguishable here from the farm/queue-switch/error clearing emissions. For such a queue the callback keeps waiting forever and never runs, so an unrecognized CLI parameter that is in neither the template nor the (empty) queue params will not be flagged — the exact case this validation is meant to catch.
The controller does distinguish load-vs-clear via the queue_parameters_loading signal (emit(True) before a real fetch, emit(False) on success/error). Consider gating on a "a real load completed" signal rather than on non-emptiness of the payload, so an empty-but-real load still validates.
There was a problem hiding this comment.
Validation is driven by a success-only signal rather than by payload non-emptiness, so a queue with zero queue parameters still validates and flags unrecognized CLI parameters. Clearing emissions (farm/queue switch, nothing selected) carry no success signal and are skipped.
…ayload Review follow-up: gating on a non-empty payload meant a queue that genuinely has zero queue parameters would never trigger validation, so an unrecognized CLI --parameter could slip through unflagged. The controller distinguishes a real load from a clearing emission via queue_parameters_loading: it emits loading(False) immediately before queue_parameters_updated on fetch completion (success or error), while clearing emissions (farm/queue switch, nothing selected) have no such prefix. Track that and run the single-shot validation on the first completed load regardless of payload emptiness. Dialog-destroyed teardown now disconnects both signals. Adds a test that an empty-but-real load still runs the validator. Signed-off-by: Stephen Crowe <[email protected]>
|
|
||
| def validate_parameters_after_queue_load(queue_parameters: list): | ||
| """Validate CLI parameters against loaded queue parameters and set parameter values""" | ||
| if not load_finished[0]: |
There was a problem hiding this comment.
On the controller error path, _on_queue_parameters_error also emits queue_parameters_loading(False) then queue_parameters_updated([]) (see _deadline_controller.py:508-509), so a failed fetch is indistinguishable here from a successful empty load: load_finished[0] becomes True and the validator runs against queue_parameters == [].
Because validation is single-shot (it disconnects on the first completed load), a transient fetch failure on the first load — e.g. the ResourceNotFoundException/AccessDeniedException cases the error handler specifically anticipates during a profile switch — will validate the CLI --parameters against an empty queue-parameter set and then never re-validate, even once a subsequent load succeeds. Any CLI parameter that is in fact a valid queue parameter would be spuriously flagged as unrecognized, and a genuinely-unrecognized one that a later successful load could have confirmed is decided prematurely.
Consider only treating a success completion as validation-eligible (e.g. a distinct signal/flag set in _on_queue_parameters_success but not _on_queue_parameters_error), so an errored load keeps waiting rather than validating against [].
There was a problem hiding this comment.
Validation connects to a success-only queue_parameters_load_succeeded signal on DeadlineUIController, emitted from _on_queue_parameters_success only. A failed fetch (e.g. a transient ResourceNotFoundException/AccessDeniedException during a profile switch) defers validation to the next successful load instead of validating against an empty list; an empty-but-real successful load still validates. Validation stays single-shot and is torn down on dialog destruction.
…oads Review follow-up: gating on load completion via queue_parameters_loading still couldn't distinguish a failed fetch from a successful empty load — _on_queue_parameters_error also emits loading(False) then updated([]). A transient first-load failure (e.g. ResourceNotFoundException during a profile switch) would validate CLI parameters against an empty set, spuriously flag valid queue parameters, and never re-validate. Add a success-only queue_parameters_load_succeeded signal to DeadlineUIController, emitted from _on_queue_parameters_success but not the error or clearing paths, and connect the single-shot CLI --parameter validation to it. An errored load keeps waiting; an empty-but-real load still validates. Dialog-destroyed teardown unchanged. Adds controller tests for the new signal's success/error/clear semantics and a submitter test that a failed load defers validation to the next successful one. Signed-off-by: Stephen Crowe <[email protected]>
Found during headless end-to-end verification: when validation runs (and self-disconnects) and the dialog is later destroyed, the destroyed-hook tried to disconnect again. PySide6 reports that with a RuntimeWarning rather than raising, so the try/except never caught it. Track connection state in a shared cell and skip the second disconnect. Adds a recwarn test that destroy-after-validation emits no disconnect warning. Signed-off-by: Stephen Crowe <[email protected]>
| controller.queue_parameters_load_succeeded.connect(validate_parameters_after_queue_load) | ||
| connected[0] = True | ||
| # If the dialog goes away before any load succeeds, tear the connection down. | ||
| submitter_dialog.destroyed.connect(disconnect_validation_callback) |
There was a problem hiding this comment.
The cleanup relies on submitter_dialog.destroyed to tear down the connection to the singleton controller, but destroyed only fires when the QObject is actually deleted, not when the dialog is merely closed/hidden. SubmitJobToDeadlineDialog does not set WA_DeleteOnClose (nor call deleteLater), so a user who closes the dialog before the first successful queue-parameter load leaves this connection live on the long-lived singleton.
If any other consumer then triggers a successful queue-parameters fetch, validate_parameters_after_queue_load fires against the closed-but-alive dialog: it pops a QMessageBox parented to a dead window and may call submitter_dialog.close() on it. The comment claims the disconnect protects against firing against a closed dialog, but that only holds for the single-shot self-disconnect after a load, not for a plain close before any load.
Note that test_dialog_destroyed_disconnects_validator emits destroyed explicitly, so it does not exercise the ordinary close path where destroyed never fires. Consider tying the teardown to the dialog close event (or setting WA_DeleteOnClose) rather than destroyed.
There was a problem hiding this comment.
Confirmed: SubmitJobToDeadlineDialog does not set WA_DeleteOnClose, and the gui-submit CLI path holds a reference through app.exec(), so an ordinary close never emits destroyed and the connection would have stayed live on the singleton.
The teardown is now also connected to QDialog.finished, which fires on accept/reject/close of a shown dialog, so close-before-any-load disconnects the callback immediately. destroyed remains connected to cover deletion without done(), and a shared connected-state guard makes all three teardown paths (single-shot self-disconnect, finished, destroyed) idempotent.
The test stand-in dialog is now a real QDialog (no WA_DeleteOnClose, matching production), and test_dialog_closed_before_load_disconnects_validator shows and closes it through the normal close path — no destroyed emission — then asserts a subsequent queue_parameters_load_succeeded emission does not invoke the validator or touch the closed dialog. This test fails without the finished hook.
SubmitJobToDeadlineDialog does not set WA_DeleteOnClose (and gui-submit holds a reference through app.exec()), so an ordinary close never emits destroyed. Closing the dialog before the first successful queue-parameter load left the validation callback connected to the long-lived singleton controller; a later successful fetch by another consumer would pop the warning against the closed dialog. Also connect the teardown to QDialog.finished, which fires on accept/reject/close of a shown dialog. The destroyed hook stays for deletion without done(); the connected-state guard makes the three teardown paths (self-disconnect, finished, destroyed) idempotent. The FakeDialog test stand-in is now a real QDialog so finished/destroyed behave as in production, and a new test closes the dialog the ordinary way (no destroyed emission) and asserts a later successful load does not invoke the validator. Signed-off-by: Stephen Crowe <[email protected]>
2 participants
Fixes:
What was the problem/requirement? (What/Why)
deadline bundle gui-submit --parametercrashed withAttributeErrorbefore the dialog could show. The code connected a validation callback toshared_job_settings._queue_parameters_update— a signal that no longer exists. Queue-parameter loading moved toDeadlineUIController, whosequeue_parameters_updated(list)signal is the live one; the--parametercall site was never updated.What was the solution? (How)
Connect the validation callback to
submitter_dialog.shared_job_settings._controller.queue_parameters_updated(the signal that carries the loaded queue parameters, which the widget itself already consumes) and drop the now-stalerefresh_idargument from the callback signature.What is the impact of this change?
gui-submit --parameterno longer crashes; CLI-supplied parameters are validated once queue parameters finish loading.How was this change tested?
Added a GUI test using a real
SharedJobSettingsWidget(not a MagicMock, which hid the bug because.connectsilently succeeds): asserts the--parameterpath doesn't raise, that emittingqueue_parameters_updatedruns the validator with the emitted list (happy path), and that a validator cancel closes the dialog.test_gui_submitter_cli_parameters.py(3 passed) plustest_pregui_hooks.pyregression (22 passed).Was this change documented?
Reviewer note
The fix reaches the widget's private
_controllerattribute and changes an internal callback signature (droppingrefresh_id). None of the widget's public signals (parameter_changed/valid_parameters/selection_changed) carry the loaded queue-parameter list, so the controller signal is the correct source.Does this PR introduce new dependencies?
Is this a breaking change?
No.
Does this change impact security?
No.
Testing
Automated (unit,
hatch run test):test/unit/deadline_client/ui/gui/test_gui_submitter_cli_parameters.py(9 tests) exercises the wiring through a realSharedJobSettingsWidgetand the real controller signals: happy-path validation, cancel-closes-dialog, clearing emission ([], no loading prefix) skipped, empty-but-real successful load still validates, failed load defers validation to the next successful load, single-shot self-disconnect, dialog-destroyed teardown, and no double-disconnect warning when destroy follows validation.test/unit/deadline_client/ui/controllers/test_deadline_controller.pycovers the newqueue_parameters_load_succeededsignal semantics: emitted on success (including with[]), not emitted on fetch error, not emitted for clearing emissions.hatch run fmt/hatch run lint(ruff + mypy) clean.Manual headless verification (
QT_QPA_PLATFORM=offscreen):Drove the real
DeadlineUIController(AWS api layer mocked, real_validate_and_warn_about_parameters,QMessageBox.questionrecorded) through the realshow_job_bundle_submitterwiring:[]) — no warning popup, dialog stays open.--parameter.ResourceNotFoundException), later success where the CLI param is a valid queue param — no popup after the error, no popup on the success (param recognized), dialog stays open.Suggested human GUI spot-check:
deadline bundle gui-submit <bundle> --parameter BogusParam=xagainst a live farm/queue: the "Unrecognized Parameters" warning should appear once after queue params load (including for a queue with no queue environments); answering No closes the submitter.--parameter <ValidQueueParam>=<value>: no warning.Manually verified (2026-07-23): ran
deadline bundle gui-submit <bundle> --parameter BogusParam=xagainst a live gamma queue from an editable install of this branch — the "Unrecognized Parameters" warning fired once after the queue params loaded, switching farms/queues produced no spurious popups, a valid parameter produced no warning, and closing the dialog before the load completed tore the connection down cleanly.