feature: add terminate for argo workflows #1309

saikonen · 2023-03-21T16:32:00Z

adds argo-workflows terminate as a command, requires a run_id for an argo workflow

relates to #1303

TODO:

add checks before issuing a terminate to a run to verify the run can be terminated.

shrinandj · 2023-03-21T18:15:00Z

metaflow/plugins/argo/argo_client.py

+        # TODO: perform check that the found workflow is able to be terminated. We do not want to patch failed / completed workflows.
+        try:
+            body = {"spec": workflow["spec"]}
+            body["spec"]["shutdown"] = "Terminate"


Can you confirm that shutdown is a required part of the spec?

shrinandj

Thanks for doing this!

metaflow/plugins/argo/argo_workflows.py

metaflow/plugins/argo/argo_client.py

metaflow/plugins/argo/argo_workflows.py

metaflow/plugins/argo/argo_workflows_cli.py

savingoyal · 2023-03-22T14:53:41Z

metaflow/plugins/argo/argo_workflows_cli.py

+
+    terminated = ArgoWorkflows.terminate(obj.workflow_name, run_id)
+    if terminated:
+        obj.echo("   Terminated.")


Nit - Run terminated. No new tasks will execute after the currently running tasks finish.

This is clearly a great feature as is. What about tasks that already started? I'd think that is the UX users actually want - though probably much more involved to implement. Imagine a wide foreach that is inflight...

Wish-list for future (--abort-running-tasks)!

note that after issuing the terminate command, even currently running tasks will not be able to finish, as they are part of the workflow to be terminated.

savingoyal

Added some minor nits.

One major question - Can you verify that terminate will only ensure that no new tasks are launched, but existing tasks still continue? We may want to introduce a kill command if it is feasible to kill all the in-flight tasks.

jackie-ob · 2023-03-22T18:53:20Z

metaflow/plugins/argo/argo_client.py

+            )
+
+        try:
+            body = {"spec": workflow["spec"]}


Minor thing: should we skip the patching if we see that shutdown is already set to Terminate as we wish? This code path probably will get exercised - when users rerun stuff (expected delays / inflight tasks etc).

jackie-ob · 2023-03-22T18:53:26Z

metaflow/plugins/argo/argo_workflows_cli.py

+
+    terminated = ArgoWorkflows.terminate(obj.workflow_name, run_id)
+    if terminated:
+        obj.echo("   Terminated.")


This is clearly a great feature as is. What about tasks that already started? I'd think that is the UX users actually want - though probably much more involved to implement. Imagine a wide foreach that is inflight...

Wish-list for future (--abort-running-tasks)!

saikonen · 2023-03-22T21:49:17Z

Added some minor nits.

One major question - Can you verify that terminate will only ensure that no new tasks are launched, but existing tasks still continue? We may want to introduce a kill command if it is feasible to kill all the in-flight tasks.

the behavior from the Task perspective is more or less the same whether we use Terminate or Stop as the shutdown plan. All tasks are stopped and not allowed to finish executing.

… finished. fix error output

saikonen · 2023-03-23T10:26:28Z

rebased off the delete feature branch so we can move forward with those two features, and leave disable for further discussion later.

savingoyal · 2023-03-28T17:08:44Z

metaflow/plugins/argo/argo_workflows_cli.py

+
+    terminated = ArgoWorkflows.terminate(obj.flow.name, run_id)
+    if terminated:
+        obj.echo("   Run terminated.")


rather than indent and print, we could print after a new line with no indent

metaflow/plugins/argo/argo_workflows_cli.py

savingoyal · 2023-03-28T17:11:40Z

metaflow/plugins/argo/argo_workflows_cli.py

+    type=str,
+    help="Authorize the termination with a production token",
+)
+@click.option(


we can take an arg run id instead of an option run id here

savingoyal · 2023-03-28T17:12:42Z

metaflow/plugins/argo/argo_workflows.py

+        # Verify that user is trying to terminate an Argo workflow
+        if not run_id.startswith("argo-"):
+            raise ArgoWorkflowsException(
+                "No active workflow for {flow_name}/{run_id} found in Argo Workflows.".format(


No execution ... to keep in sync with L:184?

savingoyal · 2023-03-28T17:16:27Z

metaflow/plugins/argo/argo_workflows_cli.py

@@ -602,3 +602,30 @@ def echo_token_instructions(obj, name, prev_user, cmd_name, cmd_description=None
        'See "Organizing Results" at docs.metaflow.org for more information '
        "about production tokens."
    )
+
+
+@argo_workflows.command(help="Terminate the Argo workflow")


minor not - Terminate flow execution on Argo Workflows. Also, in L:516 above.

Would the L:516 then read Delete the flow on Argo Workflows as its not about a single execution?

savingoyal

lgtm! minor changes after which we can merge and release

* introduce terminate command for argo-workflows * rename parameter, add check to terminate that workflow is not already finished. fix error output * changing error messages for argo workflow termination * change run_id from option to argument. refine terminate process output * Check for termination before issuing terminate on a workflow

* Add production token validation. add more output to delete process * add separate output for removing the schedule of a workflow, when applicable. * rename schedule deletion. use staticmethod instead of classmethod. error message cleanup. Document function behavior * add comments to delete behavior in argo cli. formatting fix * fix token instructions output * feature: add terminate for argo workflows (#1309) * introduce terminate command for argo-workflows * rename parameter, add check to terminate that workflow is not already finished. fix error output * changing error messages for argo workflow termination * change run_id from option to argument. refine terminate process output * Check for termination before issuing terminate on a workflow * move schedule deletion logic and comments into main delete method. * feat: refactor sensor deletion (#1343) * add sensor deletion as part of argo workflow deletion * rework token instructions output * add docstring and note about refactoring to validate_token * use validate_run_id for terminate

saikonen changed the base branch from master to feature/add-disable-for-argo-workflows March 21, 2023 16:32

saikonen requested review from savingoyal and jackie-ob March 21, 2023 16:32

shrinandj reviewed Mar 21, 2023

View reviewed changes

shrinandj approved these changes Mar 21, 2023

View reviewed changes

savingoyal reviewed Mar 22, 2023

View reviewed changes

metaflow/plugins/argo/argo_workflows.py Outdated Show resolved Hide resolved

savingoyal reviewed Mar 22, 2023

View reviewed changes

metaflow/plugins/argo/argo_client.py Outdated Show resolved Hide resolved

savingoyal reviewed Mar 22, 2023

View reviewed changes

metaflow/plugins/argo/argo_workflows.py Outdated Show resolved Hide resolved

savingoyal reviewed Mar 22, 2023

View reviewed changes

metaflow/plugins/argo/argo_workflows_cli.py Outdated Show resolved Hide resolved

savingoyal reviewed Mar 22, 2023

View reviewed changes

savingoyal requested changes Mar 22, 2023

View reviewed changes

jackie-ob reviewed Mar 22, 2023

View reviewed changes

saikonen added 3 commits March 23, 2023 12:02

introduce terminate command for argo-workflows

0c588a9

rename parameter, add check to terminate that workflow is not already…

a323325

… finished. fix error output

changing error messages for argo workflow termination

9a0a4c1

saikonen force-pushed the feature/add-terminate-for-argo-workflows branch from adedb05 to 9a0a4c1 Compare March 23, 2023 10:08

saikonen changed the base branch from feature/add-disable-for-argo-workflows to feature/add-delete-for-argo-workflows March 23, 2023 10:08

saikonen requested a review from savingoyal March 23, 2023 10:26

savingoyal reviewed Mar 28, 2023

View reviewed changes

metaflow/plugins/argo/argo_workflows_cli.py Show resolved Hide resolved

savingoyal reviewed Mar 28, 2023

View reviewed changes

savingoyal requested changes Mar 28, 2023

View reviewed changes

saikonen added 2 commits March 28, 2023 20:45

change run_id from option to argument. refine terminate process output

aea12f8

Check for termination before issuing terminate on a workflow

5b17ac6

savingoyal merged commit 32e4bff into feature/add-delete-for-argo-workflows Mar 28, 2023

savingoyal deleted the feature/add-terminate-for-argo-workflows branch March 28, 2023 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add terminate for argo workflows #1309

feature: add terminate for argo workflows #1309

saikonen commented Mar 21, 2023 •

edited

Loading

shrinandj Mar 21, 2023

shrinandj left a comment

savingoyal Mar 22, 2023

jackie-ob Mar 22, 2023

saikonen Mar 22, 2023

savingoyal left a comment

jackie-ob Mar 22, 2023

savingoyal Mar 28, 2023

jackie-ob Mar 22, 2023

saikonen commented Mar 22, 2023

saikonen commented Mar 23, 2023

savingoyal Mar 28, 2023

savingoyal Mar 28, 2023

savingoyal Mar 28, 2023

savingoyal Mar 28, 2023

saikonen Mar 28, 2023

savingoyal Mar 28, 2023

savingoyal left a comment

feature: add terminate for argo workflows #1309

feature: add terminate for argo workflows #1309

Conversation

saikonen commented Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

shrinandj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savingoyal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saikonen commented Mar 22, 2023

saikonen commented Mar 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savingoyal left a comment

Choose a reason for hiding this comment

saikonen commented Mar 21, 2023 •

edited

Loading