flyte icon indicating copy to clipboard operation
flyte copied to clipboard
verified publisher icon flyteorg

[BUG] Fix ArrayNode not reporting terminal state in TaskExecutionEvent

Open pvditt opened this issue 1 year ago • 4 comments

Describe the bug

ArrayNode currently reports events identically to maptask plugin where we encapsulate subNode executions within the ExternalResourceInfo field in TaskExecutionEvent proto message. The ExternalResourceInfo proto message has a phase field, which corresponds to the phase of the subNode. The top-level node phase is reported using (and displayed in the UI from) the NodeExecutionEvent. So there is a task phase on the TaskExecutionEvent that is not currently being used.

However, this phase is incorrect. We attempt to set it on terminal states here , but this code is never reached because terminal states return prematurely. We need to ensure that the phase on the TaskExecutionEvent is correctly set on terminal phases.

Expected behavior

task execution phases match terminal state for array nodes

Additional context to reproduce

No response

Screenshots

No response

Are you sure this issue hasn't been raised already?

  • [X] Yes

Have you read the Code of Conduct?

  • [X] Yes

pvditt avatar Mar 29 '24 01:03 pvditt

@runllm is there a pull request that fixes this issue?

cw75 avatar Apr 22 '24 22:04 cw75

To address the issue of ArrayNode not reporting the terminal state in TaskExecutionEvent, a pull request titled "[Bug] correctly set task execution phase for terminal array node" (PR #5136) is currently open. This PR aims to ensure the correct phase is set in task execution events for terminal ArrayNodes. It is still under review and not yet merged.

Would you like more details on the changes proposed in this pull request?

Sources:

This is an AI-generated response and your feedback is appreciated! Please leave a 👍 if this is helpful and 👎 if it is not.

Mention @runllm to ask follow-up questions. See runllm.com for more!

runllm[bot] avatar Apr 22 '24 22:04 runllm[bot]

~fixes: https://github.com/flyteorg/flyte/pull/5680 https://github.com/flyteorg/flyte/pull/5681 https://github.com/flyteorg/flyte/pull/5451~ oops - this is the incorrect issue for these

pvditt avatar Aug 22 '24 07:08 pvditt

~@cw75 we just upstreamed a few of the fixes. Will close this after https://github.com/flyteorg/flyte/pull/5681 is merged~

pvditt avatar Aug 22 '24 07:08 pvditt