Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

FlyteAdmin will always add base_exec_id unless it is already added #616

Closed
wants to merge 2 commits into from

Conversation

kumare3
Copy link
Contributor

@kumare3 kumare3 commented Sep 22, 2023

TL;DR

Automatically adds base_exec_id label with the value of current execution or a previous base_exec_id if it exists.

Reasons:

  1. Make it possible to retrieve all executions launched by the same base execution id (even recursively)
  2. users could group executions using their own base exec id
  3. flytectl get executions or remote list executions can use this label as a filter to retrieve high level progress of all subworkflows

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Reasons:
1. Make it possible to retrieve all executions launched by the same base execution id (even recursively)
2. users could group executions using their own base exec id
3. flytectl get executions or remote list executions can use this label as a filter to retrieve high level progress of all subworkflows

Signed-off-by: Ketan Umare <ketan.umare@gmail.com>
@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Patch coverage: 75.00% and project coverage change: +1.56% 🎉

Comparison is base (dc8cc9d) 59.01% compared to head (2697fc7) 60.57%.

❗ Current head 2697fc7 differs from pull request most recent head 44d1ba6. Consider uploading reports for the commit 44d1ba6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #616      +/-   ##
==========================================
+ Coverage   59.01%   60.57%   +1.56%     
==========================================
  Files         171      171              
  Lines       16468    13450    -3018     
==========================================
- Hits         9719     8148    -1571     
+ Misses       5899     4451    -1448     
- Partials      850      851       +1     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
pkg/manager/impl/execution_manager.go 73.07% <75.00%> (+2.54%) ⬆️

... and 157 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Ketan Umare <ketan.umare@gmail.com>
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

LGTM

@wild-endeavor wild-endeavor changed the title #minor FlyteAdmin will always add base_exec_id unless it is already added FlyteAdmin will always add base_exec_id unless it is already added Sep 22, 2023
@wild-endeavor
Copy link
Contributor

This isn't a minor change right?

@@ -1687,6 +1690,19 @@ func (m *ExecutionManager) addProjectLabels(ctx context.Context, projectName str
return initialLabels, nil
}

// Adds base execution label to execution labels. Base execution label is ignored if a corresponding label is set on the execution already.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Adds base execution label to execution labels. Base execution label is ignored if a corresponding label is set on the execution already.
// addBaseExecutionLabel adds base execution label to execution labels. Base execution label is ignored if a corresponding label is set on the execution already.

WorkflowClosure = "workflow_closure"
ParentID = "parent_id"
WorkflowClosure = "workflow_closure"
BaseExecutionIDLabelKey = "base_exec_id"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call it parent_exec_id? just to keep names consistent... I understand that for "self" execution that will sound weird... 🤷🏽 or maybe keep them as two separate labels?

@eapolinario
Copy link
Contributor

Hi, we are moving all Flyte development to a monorepo. In order to help the transition period, we're moving open PRs to monorepo automatically and your PR was moved to flyteorg/flyte#4140. Notice that if there are any conflicts in the resulting PR they most likely happen due to the change in the import path of the flyte components.

@eapolinario eapolinario closed this Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants