Page MenuHomePhabricator

Disable terminate button if run cannot be cancelled
ClosedPublic

Authored by alangenfeld on Mon, Nov 4, 11:43 AM.

Details

Reviewers
sashank
Group Reviewers
Restricted Project
Commits
R1:902e1b99a29d: Disable terminate button if run cannot be cancelled
Summary

The cancel_pipeline_execution mutation returns a CancelPipelineExecutionFailure if run.status is not PipelineRunStatus.STARTED.

This diff updates the UI to disable the "Terminate" button if run.status !== PipelineRunStatus.STARTED

Test Plan

Execute pipeline, wait for execution to finish/error, verify Terminate button is disabled

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sashank created this revision.Mon, Nov 4, 11:43 AM
sashank added a reviewer: Restricted Project.Mon, Nov 4, 11:44 AM
sashank edited the summary of this revision. (Show Details)
sashank edited the test plan for this revision. (Show Details)
sashank edited the test plan for this revision. (Show Details)
alangenfeld requested changes to this revision.Mon, Nov 4, 3:50 PM
alangenfeld added a subscriber: alangenfeld.

in what case is canCancel returning True but the run can not actually be canceled?

js_modules/dagit/src/runs/Run.tsx
84 ↗(On Diff #6175)

thats what this is for

259 ↗(On Diff #6175)

*

This revision now requires changes to proceed.Mon, Nov 4, 3:50 PM

If I had to guess the problem - its that we are not getting false for canCancel even though the run is finished in the last subscription payload due to latency between when the run is finished and when the subprocess execution manager has cleaned up the subprocess

alangenfeld commandeered this revision.Mon, Nov 4, 9:09 PM
alangenfeld edited reviewers, added: sashank; removed: alangenfeld.

let me right my wrongs

alangenfeld updated this revision to Diff 6189.Mon, Nov 4, 9:09 PM

update based on log stream

sashank added inline comments.Mon, Nov 4, 9:15 PM
js_modules/dagit/src/runs/LogsProvider.tsx
166–169

Should this be:

if (status === PipelineRunStatus.STARTED) { local.canCancel = true }

or we change the cancel_pipeline_execution error to be:

if run.status == PipelineRunStatus.FAILURE or run.status == PipelineRunStatus.SUCCESS:
        return graphene_info.schema.type_named('CancelPipelineExecutionFailure')()

so that the two match up?

sashank accepted this revision.Mon, Nov 4, 9:16 PM
This revision is now accepted and ready to land.Mon, Nov 4, 9:16 PM
alangenfeld added inline comments.Mon, Nov 4, 9:19 PM
js_modules/dagit/src/runs/LogsProvider.tsx
166–169

the only authority on setting this true is the execution manager since we may be looking at a run from a different machine - but we know that if a pipeline has reached a terminal state canCancel should certainly be false

ideally we would just be getting up to date status and canCancel fields from each subscription update but thats very tricky for both perf reasons and due to how the python server subscriptions work

sashank added inline comments.Mon, Nov 4, 9:31 PM
js_modules/dagit/src/runs/LogsProvider.tsx
166–169

That makes sense, thanks for clarifying!

This revision was landed with ongoing or failed builds.Mon, Nov 4, 9:52 PM
This revision was automatically updated to reflect the committed changes.