Populate TaskNodeMetadata on CreateNodeExecutionModel to correctly display fast cache #483
Conversation
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
hamersaw
requested review from
katrogan,
wild-endeavor,
anandswaminathan,
EngHabu,
kumare3 and
pmahindrakar-oss
as code owners
Codecov Report
@@ Coverage Diff @@
## master #483 +/- ##
==========================================
+ Coverage 58.78% 60.37% +1.58%
==========================================
Files 170 170
Lines 16055 13155 -2900
==========================================
- Hits 9438 7942 -1496
+ Misses 5783 4379 -1404
Partials 834 834
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 155 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -132,6 +132,9 @@ func TestCreateNodeEvent(t *testing.T) { | |||
| StartedAt: occurredAtProto, | |||
| CreatedAt: occurredAtProto, | |||
| UpdatedAt: occurredAtProto, | |||
| TargetMetadata: &admin.NodeExecutionClosure_TaskNodeMetadata{ | |||
There was a problem hiding this comment.
why is this change necessary? we shouldn't be initializing the metadata in the closure unless it's sourced from the event itself, right?
There was a problem hiding this comment.
So I'm not sure I follow. Currently the TaskNodeMetadata from a node event is only used if when we UpdateNodeExecutionModel here. In the CreateNodeExecutionModel func (which is called on the first node node event, update on all subsequent) we just drop any information in the TaskNodeMetadata. This PR adds support for processing the TaskNodeMetadata in the CreateNodeExecutionModel function - which creates the initial model based on the first event. This change results in setting the TaskNodeMetadata in the CreateNodeEvent function. So I copied the TargetMetadata from the unit tests for updating the node model. Does this make sense? Or am I thinking about this wrong?
There was a problem hiding this comment.
Ah sorry, I didn't realize the event under test actually had a non-empty TargetMetadata - and we should therefore expect to see the closure here have an initialized TargetMetadata
Signed-off-by: Daniel Rammer <[email protected]>
…splay fast cache (#483) * adding cache information on first node event Signed-off-by: Daniel Rammer <[email protected]> * fixed unit tests Signed-off-by: Daniel Rammer <[email protected]> --------- Signed-off-by: Daniel Rammer <[email protected]>
TL;DR
Current the CacheStatus field on node executions is only set when updating an existing model. In the case of fast cache work, where only a single node execution event will be sent for a node (ie. cache hit / succeeded), this information is never set and as a result the "cache hit" icon is never displayed in the UI. This PR sets CacheStatus if it exists on the first node execution event.
Type
Are all requirements met?
Complete description
^^^
Tracking Issue
flyteorg/flyte#2886
Follow-up issue
NA