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

Track RetryAttempt and Phase of ExternalResources #231

Merged
merged 13 commits into from
Jan 24, 2022

Conversation

hamersaw
Copy link
Contributor

@hamersaw hamersaw commented Jan 3, 2022

TL;DR

Populating RetryAttempt and Phase on ExternalResources to track array subtasks.

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

Complete description

Created a new ExternalResources struct which contains metadata on, you may have guessed, external resources which replaces the Metadata protobuf stored in TaskInfo. This decision was made for two reasons (1) the phase maintained within flyteplugins for each subtask is a flyteplugins core.Phase rather than the TaskExecution.Phase from flyteidl (which is reported in the TaskExecutionEvent) and (2) in flytepropeller all the Metadata fields except external resources are being overwritten, plus we need to convert between the flyteplugin and flyteidl phases.

Tracking Issue

flyteorg/flyte#1838

Follow-up issue

NA

@codecov
Copy link

codecov bot commented Jan 4, 2022

Codecov Report

Merging #231 (922d020) into master (590813b) will increase coverage by 0.02%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
+ Coverage   62.42%   62.45%   +0.02%     
==========================================
  Files         142      142              
  Lines        8833     8845      +12     
==========================================
+ Hits         5514     5524      +10     
+ Misses       2819     2818       -1     
- Partials      500      503       +3     
Flag Coverage Δ
unittests 62.05% <75.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
go/tasks/pluginmachinery/core/phase.go 24.05% <ø> (ø)
go/tasks/pluginmachinery/webapi/example/plugin.go 11.11% <0.00%> (+0.58%) ⬆️
go/tasks/plugins/array/awsbatch/launcher.go 57.89% <50.00%> (-1.73%) ⬇️
go/tasks/plugins/array/k8s/monitor.go 63.97% <72.72%> (+0.64%) ⬆️
go/tasks/plugins/array/core/state.go 51.17% <76.92%> (+2.69%) ⬆️
go/tasks/plugins/array/awsbatch/monitor.go 63.85% <100.00%> (+0.44%) ⬆️
go/tasks/plugins/hive/execution_state.go 71.74% <100.00%> (-0.18%) ⬇️
go/tasks/plugins/k8s/sagemaker/utils.go 71.54% <100.00%> (-0.24%) ⬇️
go/tasks/plugins/presto/execution_state.go 50.65% <100.00%> (-0.33%) ⬇️
go/tasks/plugins/webapi/athena/plugin.go 17.85% <100.00%> (-1.45%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 590813b...922d020. Read the comment docs.

@hamersaw hamersaw marked this pull request as ready for review January 4, 2022 21:14
EngHabu
EngHabu previously approved these changes Jan 19, 2022
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Looks good to me... Before we merge though. Can you build docker image for propeller with these changes so we can test on demo?

@hamersaw hamersaw merged commit 0637c34 into master Jan 24, 2022
@hamersaw hamersaw deleted the feature/task-external-resource-phase branch January 24, 2022 20:35
@hamersaw hamersaw mentioned this pull request Jan 28, 2022
8 tasks
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* replaced Metadata proto in TaskInfo with ExternalResource array

Signed-off-by: Daniel Rammer <[email protected]>

* added ExternalResource documentation comments

Signed-off-by: Daniel Rammer <[email protected]>

* setting retry attempt on external resources

Signed-off-by: Daniel Rammer <[email protected]>

* fixed unit tests and lint issues

Signed-off-by: Daniel Rammer <[email protected]>

* tracking RetryAttempt for k8s array plugin using a CompactArray

Signed-off-by: Daniel Rammer <[email protected]>

* added a few comments

Signed-off-by: Daniel Rammer <[email protected]>

* setting RetryAttempts for awsbatch subtasks

Signed-off-by: Daniel Rammer <[email protected]>

* fixed unit tests and lint issues

Signed-off-by: Daniel Rammer <[email protected]>

* populating external resource index with original index

Signed-off-by: Daniel Rammer <[email protected]>

* updated comments

Signed-off-by: Daniel Rammer <[email protected]>

* updated flyteidl version

Signed-off-by: Daniel Rammer <[email protected]>
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.

2 participants