Page MenuHomePhabricator

emit engine events for termination w/ cli api run launcher and grpc run launcher
ClosedPublic

Authored by catherinewu on Sep 27 2020, 4:51 PM.

Details

Summary

emit engine events when user attempts to terminate pipeline run. one weirdness here is if you're using the default run launcher, that attempts to terminate via cli api run launcher and then falls back to grpc run launcher which creates confusing events

Test Plan

added some checks to bk.

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

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 27 2020, 5:09 PM
Harbormaster failed remote builds in B18791: Diff 22815!
Harbormaster returned this revision to the author for changes because remote builds failed.Sep 27 2020, 8:04 PM
Harbormaster failed remote builds in B18793: Diff 22817!
Harbormaster returned this revision to the author for changes because remote builds failed.Sep 27 2020, 8:53 PM
Harbormaster failed remote builds in B18794: Diff 22818!

up; def terminate(self, run_id):

return self._cli_api_run_launcher.terminate(run_id) or self._grpc_run_launcher.terminate(
    run_id
) in default run launcher is weird
Harbormaster returned this revision to the author for changes because remote builds failed.Sep 27 2020, 9:34 PM
Harbormaster failed remote builds in B18796: Diff 22820!
Harbormaster returned this revision to the author for changes because remote builds failed.Sep 27 2020, 9:59 PM
Harbormaster failed remote builds in B18799: Diff 22824!
catherinewu retitled this revision from pipeline termination event to emit engine events for termination w/ cli api run launcher and grpc run launcher.Sep 28 2020, 4:01 PM
catherinewu edited the summary of this revision. (Show Details)
catherinewu edited the test plan for this revision. (Show Details)
catherinewu added reviewers: alangenfeld, nate.
catherinewu edited the test plan for this revision. (Show Details)

add cls field

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 28 2020, 4:19 PM
Harbormaster failed remote builds in B18810: Diff 22841!
Harbormaster completed remote builds in B18808: Diff 22839.

Whats up with these unrelated changes? Move the AWS change out to another diff at least if its intentional

python_modules/libraries/dagster-aws/dagster_aws/ecs/launcher.py
180

?

python_modules/libraries/dagster-k8s/dagster_k8s/client.py
69–71

?

tho this one seems reasonable

trust you to do ^ before landing

This revision is now accepted and ready to land.Sep 28 2020, 8:47 PM

feel free to land as is

python_modules/libraries/dagster-aws/dagster_aws/ecs/launcher.py
182–184

oh i see cool cool

python_modules/libraries/dagster-aws/dagster_aws/ecs/launcher.py
180

can_terminate should return False since the terminate function is:

def terminate(self, run_id):
        check.str_param(run_id, "run_id")
        check.not_implemented("Termination not yet implemented")

update DefaultRunLauncher terminate() to only call either cli api run launcher's terminate() or grpc run launcher's terminate() but not both