Skip to content

[WIP][Remote API] delete execution phase #3162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WIP][Remote API] delete execution phase #3162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions flytekit/clients/friendly.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,23 @@ def terminate_execution(self, id, cause):
_execution_pb2.ExecutionTerminateRequest(id=id.to_flyte_idl(), cause=cause)
)

def delete_execution_phase(self, project, domain, phase):
"""
Deletes a phase from an execution.
:param Text project: Project to delete phase from
:param Text domain: Domain to delete phase from
:param Text phase: Phase to delete
"""
from flytekit.models.core.identifier import WorkflowExecutionIdentifier

workflow_execution_id = WorkflowExecutionIdentifier(project=project, domain=domain, name="")

request_dict = {"id": workflow_execution_id.to_flyte_idl(), "phase": phase}
Comment on lines +670 to +672
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider validating execution phase parameter

Consider validating the phase parameter against allowed execution phases before making the delete request. This could help catch invalid phase values early.

Code suggestion
Check the AI-generated fix before applying
Suggested change
workflow_execution_id = WorkflowExecutionIdentifier(project=project, domain=domain, name="")
request_dict = {"id": workflow_execution_id.to_flyte_idl(), "phase": phase}
VALID_PHASES = ["QUEUED", "RUNNING", "SUCCEEDED", "FAILED", "ABORTED", "TIMED_OUT"]
if phase not in VALID_PHASES:
raise ValueError(f"Invalid phase '{phase}'. Must be one of {VALID_PHASES}")
workflow_execution_id = WorkflowExecutionIdentifier(project=project, domain=domain, name="")
request_dict = {"id": workflow_execution_id.to_flyte_idl(), "phase": phase}

Code Review Run #3b0d37

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them


super(SynchronousFlyteClient, self).delete_execution_phase(
_execution_pb2.ExecutionPhaseDeleteRequest(**request_dict)
)

def relaunch_execution(self, id, name=None):
"""
:param flytekit.models.core.identifier.WorkflowExecutionIdentifier id:
Expand Down
7 changes: 7 additions & 0 deletions flytekit/clients/raw.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,13 @@ def terminate_execution(self, terminate_execution_request):
"""
return self._stub.TerminateExecution(terminate_execution_request, metadata=self._metadata)

def delete_execution_phase(self, delete_execution_phase_request):
"""
:param flyteidl.admin.execution_pb2.DeleteExecutionPhaseRequest delete_execution_phase_request:
:rtype: flyteidl.admin.execution_pb2.DeleteExecutionPhaseResponse
"""
return self._stub.DeleteExecutionPhase(delete_execution_phase_request, metadata=self._metadata)
Comment on lines +419 to +424
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding gRPC error handling

Consider adding error handling for potential gRPC exceptions in the delete_execution_phase method. The current implementation might fail silently if the gRPC call encounters network issues or server errors.

Code suggestion
Check the AI-generated fix before applying
Suggested change
def delete_execution_phase(self, delete_execution_phase_request):
"""
:param flyteidl.admin.execution_pb2.DeleteExecutionPhaseRequest delete_execution_phase_request:
:rtype: flyteidl.admin.execution_pb2.DeleteExecutionPhaseResponse
"""
return self._stub.DeleteExecutionPhase(delete_execution_phase_request, metadata=self._metadata)
def delete_execution_phase(self, delete_execution_phase_request):
"""
:param flyteidl.admin.execution_pb2.DeleteExecutionPhaseRequest delete_execution_phase_request:
:rtype: flyteidl.admin.execution_pb2.DeleteExecutionPhaseResponse
"""
try:
return self._stub.DeleteExecutionPhase(delete_execution_phase_request, metadata=self._metadata)
except grpc.RpcError as e:
logger.error(f"Failed to delete execution phase: {e}")
raise

Code Review Run #3b0d37

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them


def relaunch_execution(self, relaunch_execution_request):
"""
:param flyteidl.admin.execution_pb2.ExecutionRelaunchRequest relaunch_execution_request:
Expand Down
30 changes: 30 additions & 0 deletions flytekit/clis/flyte_cli/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1170,6 +1170,36 @@ def terminate_execution(host, insecure, cause, urn=None):
_terminate_one_execution(client, urn, cause)


@_flyte_cli.command("delete-execution-phase", cls=_FlyteSubCommand)
@_host_option
@_insecure_option
@click.option("-p", "--project", required=True, help="Project to delete execution phase from")
@click.option("-d", "--domain", required=True, help="Domain to delete execution phase from")
@click.option("-ph", "--phase", required=True, help="Phase to delete")
def delete_execution_phase(host, insecure, project, domain, phase):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding phase parameter validation

Consider adding validation for the phase parameter to ensure it matches valid execution phase values. The phase parameter appears to be used directly without validation which could lead to invalid phase deletion attempts.

Code suggestion
Check the AI-generated fix before applying
Suggested change
def delete_execution_phase(host, insecure, project, domain, phase):
VALID_PHASES = ["QUEUED", "RUNNING", "SUCCEEDED", "FAILED", "ABORTED", "TIMED_OUT"]
def delete_execution_phase(host, insecure, project, domain, phase):
if phase not in VALID_PHASES:
click.echo(f"\nInvalid phase value: {phase}. Valid phases are: {', '.join(VALID_PHASES)}")
sys.exit(1)

Code Review Run #3b0d37

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

"""
Delete a phase from executions. This command deletes a phase from executions
in the specified project and domain.

e.g.,
$ flyte-cli -h localhost:30081 delete-execution-phase \
-p flyteexamples -d development -ph RUNNING
"""
_welcome_message()
client = _get_client(host, insecure)

click.echo("Deleting phase from executions:\n")
click.echo("{:40} {:40} {:40}".format("Project", "Domain", "Phase"))
click.echo("{:40} {:40} {:40}".format(project, domain, phase))

try:
client.delete_execution_phase(project, domain, phase)
click.echo("\nSuccessfully deleted phase from executions.")
except Exception as e:
click.echo(f"\nFailed to delete phase from executions: {str(e)}")
sys.exit(1)


@_flyte_cli.command("list-executions", cls=_FlyteSubCommand)
@_project_option
@_domain_option
Expand Down
12 changes: 12 additions & 0 deletions flytekit/remote/remote.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2677,6 +2677,18 @@ def terminate(self, execution: FlyteWorkflowExecution, cause: str):
"""
self.client.terminate_execution(execution.id, cause)

#############################
# Delete Execution Phase #
#############################

def delete_execution_phase(self, execution: FlyteWorkflowExecution, phase: str):
"""Delete a phase from executions.

:param execution: workflow execution to delete phase from
:param phase: phase to delete
"""
self.client.delete_execution_phase(execution.id.project, execution.id.domain, phase)
Comment on lines +2684 to +2690
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding phase parameter validation

Consider validating the phase parameter against allowed execution phases before making the delete call. This could help catch invalid phase values early.

Code suggestion
Check the AI-generated fix before applying
Suggested change
def delete_execution_phase(self, execution: FlyteWorkflowExecution, phase: str):
"""Delete a phase from executions.
:param execution: workflow execution to delete phase from
:param phase: phase to delete
"""
self.client.delete_execution_phase(execution.id.project, execution.id.domain, phase)
def delete_execution_phase(self, execution: FlyteWorkflowExecution, phase: str):
"""Delete a phase from executions.
:param execution: workflow execution to delete phase from
:param phase: phase to delete - must be a valid execution phase
:raises ValueError: if phase is not a valid execution phase
"""
valid_phases = {"RUNNING", "SUCCEEDED", "FAILED", "ABORTED"}
if phase not in valid_phases:
raise ValueError(f"Invalid phase {phase}. Must be one of {valid_phases}")
self.client.delete_execution_phase(execution.id.project, execution.id.domain, phase)

Code Review Run #3b0d37

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them


############
# Projects #
############
Expand Down
Loading