Warn when a post job action cannot run on a step with no job - #23334
Draft
jmchilton wants to merge 5 commits into
Draft
Warn when a post job action cannot run on a step with no job#23334jmchilton wants to merge 5 commits into
jmchilton wants to merge 5 commits into
Conversation
pick_value steps produce outputs with no job to hang an action off of, so PickValueModule dispatches every post job action through ActionBox.execute_on_mapped_over. Actions that never implemented execute_on_mapped_over inherit the base no-op, so EmailAction and DeleteIntermediatesAction can be configured in the editor, are saved to the database, and are then silently dropped at runtime. Make the capability explicit as DefaultJobAction.supports_mapped_over and have ActionBox log a warning naming the step and output rather than dispatching into a no-op. The other caller, ToolModule, filters through mapped_over_output_actions first, so it never reaches the warning. This is the backend half only. The editor still offers the two toggles on a pick_value step - FormSection renders them for every step - so the workflow author still gets no feedback at configuration time. Suppressing them there is a separate change. Adds test/unit/job_execution/test_action_box.py - the first unit coverage of ActionBox - pinning supports_mapped_over to what each class actually overrides so the two cannot drift, and pinning that a supported action still reaches its output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_pick_value_hides_actions_needing_a_job asserts Email notification and Output cleanup are absent on a pick_value step and still present on a tool step. It also waits on the output card, so the absence assertions cannot pass against a form that simply had not rendered. The pick_value step is selected first - the panel a selected node opens covers the nodes to its right, so selecting the tool step first makes the pick node unclickable. Red without the v-if in FormSection: "Expected DOM elements [...] to be empty for selector target XPATH selector [//div[@data-label='Email notification']". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PostJobAction and HistoryDatasetAssociation both construct without a session, so the two Mocks were standing in for objects that are cheaper to just build. trans and sa_session become None rather than Mocks. Nothing on this path reads them - a rename only touches the output it is handed, and unsupported or unknown actions never dispatch - so None turns that into something the test enforces instead of something a Mock would quietly absorb. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mypy sees a classmethod read off the class as a plain Callable, which has no __func__, so `make mypy` and the packages job both failed on it. inspect .getattr_static hands back the classmethod object instead, so __func__ reaches the underlying function. Same identity comparison as before - RemoveTagDatasetAction still reads as implementing it via TagDatasetAction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
A
pick_valuestep has no job to hang a post job action off, soPickValueModule._apply_post_job_actionsdispatches throughActionBox.execute_on_mapped_overinstead. That dispatches every action on the step with no allowlist, and any action that does not implementexecute_on_mapped_overinherits theDefaultJobActionbase, which ispass. Today that is exactlyEmailActionandDeleteIntermediatesAction: both are configurable on apick_valuestep, both silently do nothing, and nothing anywhere says so.Add a
supports_mapped_overclass attribute recording whether an action actually implements the method, and haveActionBox.execute_on_mapped_overwarn — naming the step and output — rather than dispatching into a no-op. A unit test asserts the flag agrees with what each class overrides, so it cannot go stale, and a second asserts every action inmapped_over_output_actionssupports it. This is the first unit coverageActionBoxhas had.The second commit is the editor half, so the two toggles cannot be configured in the first place.
FormSectionrenders "Email notification" and "Output cleanup" for every step; it now takes asupportsJobBasedActionsprop defaulting totrue, andFormPickValuepassesfalse.FormTool, the only other consumer, is untouched.One consequence worth knowing: a workflow that already has one of these actions on a
pick_valuestep keeps it —FormSectionstill round-trips the hidden keys, so nothing is dropped on save — but it is no longer visible or clearable in the editor, while the backend now logs a warning naming that step. Clearing it means editing the workflow outside the editor.Part of #22200.
How to test the changes?
test/unit/job_execution/test_action_box.pycovers the dispatch: an unsupported action warns instead of running, a supported one still reaches the output, and an unknown action type is ignored silently. It builds realPostJobActionandHistoryDatasetAssociationobjects rather than mocks — both construct without a session — and passesNonefortransandsa_session, since nothing on this path reads them.On the client,
FormSection.test.tscovers the two render paths andFormPickValue.test.tscovers the prop being passed.test_workflow_editor.py::test_pick_value_hides_actions_needing_a_jobcovers it end to end: both toggles absent on apick_valuestep, both present on a tool step. It waits on the output card first, so the absence assertions cannot pass against a form that simply had not rendered.License