[Bug] correctly set task execution phase for terminal array node #5136

pvditt · 2024-03-29T01:00:56Z

Tracking issue

#5135

Why are the changes needed?

Task execution phases are not set correctly for successful nor failed ArrayNodes

What changes were proposed in this pull request?

ensure correct phase is set in task execution events

How was this patch tested?

Setup process

ran a workflow with a map task that failed and one that succeeded. Confirmed that the taskExecution closure phase was correctly set.

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

Docs link

Summary by Bito

This PR addresses a bug in the array node handler related to task execution phase settings. The changes implement proper phase setting in task execution events and streamline the abort handler by removing redundant failure event recording. The modifications enhance state reporting accuracy for both successful and failed terminal states, while improving test coverage for phase transitions. The update includes comprehensive test cases for verifying task execution phases across different array node states.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 2

Signed-off-by: Paul Dittamo <[email protected]>

codecov · 2024-03-29T01:05:41Z

Codecov Report

Attention: Patch coverage is 40.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 37.02%. Comparing base (b010747) to head (8e4f4fa).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
...ytepropeller/pkg/controller/nodes/array/handler.go	40.00%	7 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5136   +/-   ##
=======================================
  Coverage   37.02%   37.02%           
=======================================
  Files        1317     1317           
  Lines      132523   132544   +21     
=======================================
+ Hits        49066    49080   +14     
- Misses      79211    79218    +7     
  Partials     4246     4246

Flag	Coverage Δ
unittests-datacatalog	`51.58% <ø> (ø)`
unittests-flyteadmin	`54.25% <ø> (ø)`
unittests-flytecopilot	`30.99% <ø> (ø)`
unittests-flytectl	`62.29% <ø> (ø)`
unittests-flyteidl	`7.23% <ø> (ø)`
unittests-flyteplugins	`53.85% <ø> (ø)`
unittests-flytepropeller	`42.66% <40.00%> (+0.02%)`	⬆️
unittests-flytestdlib	`55.13% <ø> (-0.06%)`	⬇️

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

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

Signed-off-by: Paul Dittamo <[email protected]>

flyte-bot · 2025-01-10T07:48:02Z

Code Review Agent Run #e4c107

Actionable Suggestions - 2

flytepropeller/pkg/controller/nodes/array/handler.go - 2
- Consider more graceful error handling · Line 132-134
- Consider validating eventRecorder before use · Line 473-474

Review Details

Files reviewed - 1 · Commit Range: 344bde3..6cc769f
- flytepropeller/pkg/controller/nodes/array/handler.go
Files skipped - 0
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-01-10T07:50:29Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Bug Fix - Fix Array Node Task Execution Phase Reporting	- `handler.go` - Added proper task execution phase handling for aborted and failed states - `handler_test.go` - Updated tests to verify correct task execution phase settings

flyte-bot · 2025-01-10T07:50:31Z

flytepropeller/pkg/controller/nodes/array/handler.go

+		if !eventsErr.IsAlreadyExists(err) {
+			logger.Errorf(ctx, "ArrayNode event recording failed: [%s]", err.Error())
+			return err


Consider more graceful error handling

Consider handling the error case more gracefully by logging the error and continuing execution rather than returning early. The current implementation may cause unnecessary aborts.

Code suggestion

Check the AI-generated fix before applying

Suggested change

if !eventsErr.IsAlreadyExists(err) {

logger.Errorf(ctx, "ArrayNode event recording failed: [%s]", err.Error())

return err

if eventsErr.IsAlreadyExists(err) {

return nil

}

Code Review Run #e4c107

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

flyte-bot · 2025-01-10T07:50:32Z

flytepropeller/pkg/controller/nodes/array/handler.go

+		// ensure task_execution set to failed - this should already be sent by the abort handler
+		if err := eventRecorder.finalize(ctx, nCtx, idlcore.TaskExecution_FAILED, 0, a.eventConfig); err != nil {


Consider validating eventRecorder before use

Consider checking if eventRecorder is not nil before calling finalize(). The current code assumes eventRecorder is always initialized.

Code suggestion

Check the AI-generated fix before applying

Suggested change

// ensure task_execution set to failed - this should already be sent by the abort handler

if err := eventRecorder.finalize(ctx, nCtx, idlcore.TaskExecution_FAILED, 0, a.eventConfig); err != nil {

// ensure task_execution set to failed - this should already be sent by the abort handler

if eventRecorder == nil {

logger.Errorf(ctx, "ArrayNode eventRecorder is nil")

return handler.UnknownTransition, fmt.Errorf("eventRecorder is nil")

}

if err := eventRecorder.finalize(ctx, nCtx, idlcore.TaskExecution_FAILED, 0, a.eventConfig); err != nil {

Code Review Run #e4c107

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

hamersaw

Any unit tests we should be adding? It looks like we will send some new events now right?

pvditt · 2025-01-10T18:36:30Z

Any unit tests we should be adding? It looks like we will send some new events now right?

@hamersaw I'll upstream unit tests for array node and then add tests in this PR

Signed-off-by: Paul Dittamo <[email protected]>

flyte-bot · 2025-01-13T09:33:27Z

Code Review Agent Run #5e4929

Actionable Suggestions - 2

flytepropeller/pkg/controller/nodes/array/handler_test.go - 2
- Consider using constants for test phases · Line 220-221
- Consider declaring err variable explicitly · Line 298-298

Review Details

Files reviewed - 2 · Commit Range: 6cc769f..8e4f4fa
- flytepropeller/pkg/controller/nodes/array/handler.go
- flytepropeller/pkg/controller/nodes/array/handler_test.go
Files skipped - 0
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-01-13T09:37:02Z

flytepropeller/pkg/controller/nodes/array/handler_test.go

+			subNodePhases:                  []v1alpha1.NodePhase{v1alpha1.NodePhaseSucceeded, v1alpha1.NodePhaseRunning, v1alpha1.NodePhaseNotYetStarted},
+			subNodeTaskPhases:              []core.Phase{core.PhaseSuccess, core.PhaseRunning, core.PhaseUndefined},


Consider using constants for test phases

Consider using constants or enums for the node phases and task phases arrays to improve readability and maintainability. The array literals []v1alpha1.NodePhase{...} and []core.Phase{...} could be defined as test constants.

Code suggestion

Check the AI-generated fix before applying

@@ -203,6 +203,16 @@ func TestAbort(t *testing.T) { +var ( + testSubNodePhases = []v1alpha1.NodePhase{v1alpha1.NodePhaseSucceeded, v1alpha1.NodePhaseRunning, v1alpha1.NodePhaseNotYetStarted} + testSubNodeTaskPhases = []core.Phase{core.PhaseSuccess, core.PhaseRunning, core.PhaseUndefined} +) @@ -220,8 +220,8 @@ func TestAbort(t *testing.T) { - subNodePhases: []v1alpha1.NodePhase{v1alpha1.NodePhaseSucceeded, v1alpha1.NodePhaseRunning, v1alpha1.NodePhaseNotYetStarted}, - subNodeTaskPhases: []core.Phase{core.PhaseSuccess, core.PhaseRunning, core.PhaseUndefined}, + subNodePhases: testSubNodePhases, + subNodeTaskPhases: testSubNodeTaskPhases,

Code Review Run #5e4929

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

flyte-bot · 2025-01-13T09:37:03Z

flytepropeller/pkg/controller/nodes/array/handler_test.go

@@ -279,12 +295,13 @@ func TestAbort(t *testing.T) {
 			nCtx := createNodeExecutionContext(dataStore, eventRecorder, nil, literalMap, &arrayNodeSpec, arrayNodeState, 0, workflowMaxParallelism)

 			// evaluate node
-			err := arrayNodeHandler.Abort(ctx, nCtx, "foo")
+			err = arrayNodeHandler.Abort(ctx, nCtx, "foo")


Consider declaring err variable explicitly

Consider declaring err variable before using it. The current code reuses an existing err variable which could lead to shadowing issues.

Code suggestion

Check the AI-generated fix before applying

Suggested change

err = arrayNodeHandler.Abort(ctx, nCtx, "foo")

err := arrayNodeHandler.Abort(ctx, nCtx, "foo")

Code Review Run #5e4929

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

pvditt · 2025-01-13T17:00:09Z

@hamersaw ended up not needing to upstream tests

correctly set task execution phase for array node

344bde3

Signed-off-by: Paul Dittamo <[email protected]>

runllm bot mentioned this pull request Apr 22, 2024

[BUG] Fix ArrayNode not reporting terminal state in TaskExecutionEvent #5135

Closed

2 tasks

pvditt added 3 commits August 23, 2024 13:18

Merge branch 'master' into bug/arraynode-terminal-eventing

059bec4

Signed-off-by: Paul Dittamo <[email protected]>

clean up

0e0d7c4

Signed-off-by: Paul Dittamo <[email protected]>

Merge branch 'master' into bug/arraynode-terminal-eventing

6cc769f

Signed-off-by: Paul Dittamo <[email protected]>

pvditt marked this pull request as ready for review January 10, 2025 06:45

pvditt requested a review from hamersaw January 10, 2025 06:51

flyte-bot reviewed Jan 10, 2025

View reviewed changes

hamersaw previously approved these changes Jan 10, 2025

View reviewed changes

update unit tests

8e4f4fa

Signed-off-by: Paul Dittamo <[email protected]>

pvditt dismissed hamersaw’s stale review via 8e4f4fa January 13, 2025 08:30

flyte-bot reviewed Jan 13, 2025

View reviewed changes

pvditt requested a review from hamersaw January 13, 2025 16:59

hamersaw approved these changes Jan 13, 2025

View reviewed changes

pvditt merged commit be66530 into master Jan 13, 2025
53 checks passed

pvditt deleted the bug/arraynode-terminal-eventing branch January 13, 2025 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] correctly set task execution phase for terminal array node #5136

[Bug] correctly set task execution phase for terminal array node #5136

pvditt commented Mar 29, 2024 •

edited by flyte-bot

Loading

codecov bot commented Mar 29, 2024 •

edited

Loading

flyte-bot commented Jan 10, 2025 •

edited

Loading

Code Review Agent Run #e4c107

flyte-bot commented Jan 10, 2025 •

edited

Loading

Changelist by Bito

flyte-bot Jan 10, 2025

flyte-bot Jan 10, 2025

hamersaw left a comment

pvditt commented Jan 10, 2025

flyte-bot commented Jan 13, 2025 •

edited

Loading

Code Review Agent Run #5e4929

flyte-bot Jan 13, 2025

flyte-bot Jan 13, 2025

pvditt commented Jan 13, 2025

		// ensure task_execution set to failed - this should already be sent by the abort handler
		if err := eventRecorder.finalize(ctx, nCtx, idlcore.TaskExecution_FAILED, 0, a.eventConfig); err != nil {

		subNodePhases: []v1alpha1.NodePhase{v1alpha1.NodePhaseSucceeded, v1alpha1.NodePhaseRunning, v1alpha1.NodePhaseNotYetStarted},
		subNodeTaskPhases: []core.Phase{core.PhaseSuccess, core.PhaseRunning, core.PhaseUndefined},

	err = arrayNodeHandler.Abort(ctx, nCtx, "foo")
	err := arrayNodeHandler.Abort(ctx, nCtx, "foo")

[Bug] correctly set task execution phase for terminal array node #5136

[Bug] correctly set task execution phase for terminal array node #5136

Conversation

pvditt commented Mar 29, 2024 • edited by flyte-bot Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

Summary by Bito

codecov bot commented Mar 29, 2024 • edited Loading

Codecov Report

flyte-bot commented Jan 10, 2025 • edited Loading

Code Review Agent Run #e4c107

flyte-bot commented Jan 10, 2025 • edited Loading

Changelist by Bito

flyte-bot Jan 10, 2025

Choose a reason for hiding this comment

flyte-bot Jan 10, 2025

Choose a reason for hiding this comment

hamersaw left a comment

Choose a reason for hiding this comment

pvditt commented Jan 10, 2025

flyte-bot commented Jan 13, 2025 • edited Loading

Code Review Agent Run #5e4929

flyte-bot Jan 13, 2025

Choose a reason for hiding this comment

flyte-bot Jan 13, 2025

Choose a reason for hiding this comment

pvditt commented Jan 13, 2025

pvditt commented Mar 29, 2024 •

edited by flyte-bot

Loading

codecov bot commented Mar 29, 2024 •

edited

Loading

flyte-bot commented Jan 10, 2025 •

edited

Loading

flyte-bot commented Jan 10, 2025 •

edited

Loading

flyte-bot commented Jan 13, 2025 •

edited

Loading