#patch: Add in task execution event fields #119

katrogan · 2021-03-16T00:12:31Z

Signed-off-by: Katrina Rogan [email protected]

TL;DR

Add in task execution event fields

Add phase reason and task type to TaskExecutionEvent
Add generated_name to TaskExecutionMetadata
Add a ManagedResourceInfo message for capturing resource token name (and future FRM attributes)

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

flyteorg/flyte#325

Follow-up issue

NA

Signed-off-by: Katrina Rogan <[email protected]>

EngHabu · 2021-03-16T00:14:15Z

protos/flyteidl/event/event.proto

+    string generated_name = 1;
+
+    // Includes information about how resource token allocation (if applicable).
+    ManagedResourceInfo managed_resource_info = 2;


should this be repeated?

perhaps i'm misunderstanding but isn't this only determined once per execution?

A plugin can request multiple resource allocations during execution... our plugins only request one resource at most because we didn't need more... but the API allows plugins to request more

@EngHabu got it, thanks. updated

protos/flyteidl/event/event.proto

Signed-off-by: Katrina Rogan <[email protected]>

EngHabu · 2021-03-16T18:15:27Z

protos/flyteidl/event/event.proto

+// executions for a project namespace.
+message ManagedResourceInfo {
+    // Unique resource ID used to identify this execution when allocating a token.
+    string token                         = 1;


Should we call this allocation_token ?

Should we capture more of the request? https://github.com/flyteorg/flyteplugins/blob/master/go/tasks/pluginmachinery/core/resource_manager.go#L82
the resource namespace maybe?

sure done @EngHabu

Signed-off-by: Katrina Rogan <[email protected]>

kumare3 · 2021-03-16T23:21:46Z

protos/flyteidl/event/event.proto

@@ -167,4 +187,11 @@ message TaskExecutionMetadata {
        INTERRUPTIBLE = 1;
    }
    InstanceClass instance_class = 16;
+
+    // Generated unique name for this task execution used by the backend.
+    string generated_name = 1;


should we call it resource_name?
As for Qubole/AWS Batch etc, it may not be the generated_name?
Or we can have both - but that is confusing!

Signed-off-by: Katrina Rogan <[email protected]>

kumare3 · 2021-03-16T23:38:54Z

protos/flyteidl/event/event.proto

@@ -151,10 +151,40 @@ message TaskExecutionEvent {
    // The version field should be incremented when metadata changes across the duration of an individual phase.
    uint32 phase_version                  = 12;    

+    // If there is an explanation for this phase transition, the reason will capture it.


Suggested change

// If there is an explanation for this phase transition, the reason will capture it.

// An optional explanation for the phase transition

kumare3 · 2021-03-16T23:39:33Z

protos/flyteidl/event/event.proto

+    // A predefined yet extensible Task type identifier. If the task definition is already registered in flyte admin
+    // this type will be identical, but not all task executions necessarily use pre-registered definitions and this
+    // type is useful to render the task in the UI, filter task executions, etc.
+    string task_type                      = 14;


should we also add the actual execution plugin-ID used?

Signed-off-by: Katrina Rogan <[email protected]>

kumare3 · 2021-03-16T23:45:08Z

protos/flyteidl/event/event.proto

@@ -167,4 +197,14 @@ message TaskExecutionMetadata {
        INTERRUPTIBLE = 1;
    }
    InstanceClass instance_class = 16;
+
+    // Contains various identifiers for resources used during execution.


nit: can we order them by the field id? sorry!

@kumare3 done

Signed-off-by: Katrina Rogan <[email protected]>

katrogan · 2021-03-17T18:06:54Z

friendly ping @EngHabu @kumare3

kumare3 · 2021-03-17T20:15:07Z

protos/flyteidl/event/event.proto

 // Holds metadata around how a task was executed.
 // TODO(katrogan): Extend to include freeform fields (https://github.com/flyteorg/flyte/issues/325).
 message TaskExecutionMetadata {
+


Can we remove the todo on the message? indicates task -
flyteorg/flyte#325

oh yes good catch, done

kumare3 · 2021-03-17T20:18:07Z

protos/flyteidl/event/event.proto

+    // this type will be identical, but not all task executions necessarily use pre-registered definitions and this
+    // type is useful to render the task in the UI, filter task executions, etc.
+    string task_type                      = 14;
+


@katrogan another question, why is task_type / reason not metadata? Maybe a clearer definition in the distinction

So I like the documentation at the beginning of this proto message: Plugin specific execution event information i.e. attributes of the event itself. In this case task_type is an attribute of the execution and reason goes along with phase and phase version as part of the actual event update, right? I see metadata is more for identifying/discerning how a task was executed rather than artifacts or products of the event itself.

protos/flyteidl/event/event.proto

Signed-off-by: Katrina Rogan <[email protected]>

katrogan · 2021-03-18T16:32:06Z

one last (?) ping @kumare3 @EngHabu

EngHabu

👍

Add in task execution event fields

22f01c7

Signed-off-by: Katrina Rogan <[email protected]>

EngHabu reviewed Mar 16, 2021

View reviewed changes

katrogan added 3 commits March 15, 2021 17:29

derp derp

88a8b21

Signed-off-by: Katrina Rogan <[email protected]>

comments

3b4442c

Signed-off-by: Katrina Rogan <[email protected]>

repeated

3f567c5

Signed-off-by: Katrina Rogan <[email protected]>

katrogan changed the title ~~Add in task execution event fields~~ Feat: Add in task execution event fields Mar 16, 2021

katrogan changed the title ~~Feat: Add in task execution event fields~~ Patch: Add in task execution event fields Mar 16, 2021

katrogan changed the title ~~Patch: Add in task execution event fields~~ #Patch: Add in task execution event fields Mar 16, 2021

EngHabu changed the title ~~#Patch: Add in task execution event fields~~ #patch: Add in task execution event fields Mar 16, 2021

EngHabu previously approved these changes Mar 16, 2021

View reviewed changes

review comments

76bcb10

Signed-off-by: Katrina Rogan <[email protected]>

katrogan dismissed EngHabu’s stale review via 76bcb10 March 16, 2021 19:25

EngHabu previously approved these changes Mar 16, 2021

View reviewed changes

kumare3 reviewed Mar 16, 2021

View reviewed changes

comment

90d40d5

Signed-off-by: Katrina Rogan <[email protected]>

katrogan dismissed EngHabu’s stale review via 90d40d5 March 16, 2021 23:24

refactor

0fd60c4

Signed-off-by: Katrina Rogan <[email protected]>

kumare3 reviewed Mar 16, 2021

View reviewed changes

plugin identifier too

577cd08

Signed-off-by: Katrina Rogan <[email protected]>

kumare3 reviewed Mar 16, 2021

View reviewed changes

katrogan added 3 commits March 16, 2021 16:48

reorder

02af7a8

Signed-off-by: Katrina Rogan <[email protected]>

regen

c40ff1b

Signed-off-by: Katrina Rogan <[email protected]>

add untracked files

2d15457

Signed-off-by: Katrina Rogan <[email protected]>

kumare3 reviewed Mar 17, 2021

View reviewed changes

EngHabu reviewed Mar 17, 2021

View reviewed changes

protos/flyteidl/event/event.proto Outdated Show resolved Hide resolved

regen

0f5a69f

Signed-off-by: Katrina Rogan <[email protected]>

kumare3 previously approved these changes Mar 17, 2021

View reviewed changes

comment, regen

1c32610

Signed-off-by: Katrina Rogan <[email protected]>

katrogan dismissed kumare3’s stale review via 1c32610 March 17, 2021 23:11

kumare3 approved these changes Mar 18, 2021

View reviewed changes

EngHabu approved these changes Mar 18, 2021

View reviewed changes

katrogan merged commit f7abcb0 into master Mar 18, 2021

eapolinario pushed a commit that referenced this pull request Sep 8, 2023

#patch: Add in task execution event fields (#119)

d466b98

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#patch: Add in task execution event fields #119

#patch: Add in task execution event fields #119

katrogan commented Mar 16, 2021

EngHabu Mar 16, 2021

katrogan Mar 16, 2021

EngHabu Mar 16, 2021

katrogan Mar 16, 2021

EngHabu Mar 16, 2021

EngHabu Mar 16, 2021

katrogan Mar 16, 2021

kumare3 Mar 16, 2021

katrogan Mar 16, 2021

kumare3 Mar 16, 2021

katrogan Mar 16, 2021

kumare3 Mar 16, 2021

kumare3 Mar 16, 2021

katrogan Mar 16, 2021

katrogan commented Mar 17, 2021

kumare3 Mar 17, 2021

katrogan Mar 17, 2021

kumare3 Mar 17, 2021

katrogan Mar 17, 2021

katrogan commented Mar 18, 2021

EngHabu left a comment

	// If there is an explanation for this phase transition, the reason will capture it.
	// An optional explanation for the phase transition

#patch: Add in task execution event fields #119

#patch: Add in task execution event fields #119

Conversation

katrogan commented Mar 16, 2021

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katrogan commented Mar 17, 2021

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

katrogan commented Mar 18, 2021

EngHabu left a comment

Choose a reason for hiding this comment