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

Instrument ArrayNode #550

Merged
merged 68 commits into from
Jul 31, 2023
Merged

Instrument ArrayNode #550

merged 68 commits into from
Jul 31, 2023

Conversation

hamersaw
Copy link
Contributor

@hamersaw hamersaw commented Apr 6, 2023

TL;DR

Instrumenting support for ArrayNodes to provide a functionally complete map task experience.

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

^^^

Tracking Issue

flyteorg/flyte#1131

Follow-up issue

NA

Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw hamersaw marked this pull request as ready for review July 19, 2023 15:13
Copy link
Contributor

@kumare3 kumare3 left a comment

Choose a reason for hiding this comment

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

To be honest this is a crazy big code change and it looks pretty good. Boy though, when you look at some of the repeated verbosity in golang haha. But, mostly looks great to me, couple small comments.

Since this is behind a feature gate, i think this is safe to go

@hamersaw hamersaw merged commit 8446baf into master Jul 31, 2023
14 checks passed
@hamersaw hamersaw deleted the feature/array-node branch July 31, 2023 14:05
eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
* updated flyteidl to local to get ArrayNode

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

* added boilerplate to support ArrayNode

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

* pushing forward

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

* refactored node executor interfaces to fix dependency cycle

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

* refactoring almost complete

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

* refactor complete

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

* supporting environment variables

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

* minimum viable product

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

* update print statements for debugging

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

* massive refactor fixing NodeExecutionContext override for ArrayNode

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

* refactoring TODOs

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

* subnode retries working

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

* parallelism working

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

* cache and cache_serialize working - first new functionality in maptask

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

* adding implementation notes

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

* removed eventing from subtasks

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

* adding correct requirements

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

* working end-2-end with flytekit

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

* reporting output directory on success

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

* fixed output directory append

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

* mocking TaskTemplate interface to enable caching

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

* capture failure reasons

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

* wrapped up abort and finalize functionality

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

* mocking initialization events

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

* sending all events

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

* minor refactoring of debug prints and formatting

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

* intratask checkpointing working

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

* support for  and

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

* setting node log ids correctly

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

* reporting cache status

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

* correctly setting subnode abort phase

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

* removing dead code

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

* cleaned up most random TODO items

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

* refactored into new files

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

* refactoring for ArrayNode unit tests

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

* refactored for unit testing to allow creation of NodeExecutor in array package

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

* first unit test for handling ArrayNodePhaseNone

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

* most of executing unit tests completed

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

* finished executing unit tests

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

* finished succeeding unit tests

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

* wrote failing phase unit tests

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

* moving towards complete unit_test success

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

* unit tests passing

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

* fixed lint issues

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

* updated flyteidl dep

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

* added unit tests for Abort

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

* adding unit test for Finalize

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

* added utils unit tests

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

* moved state structs to handler package

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

* added docs

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

* cleaned up abort event reporting

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

* fixed RecordNodeEvent unit tests

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

* removed taskEventRecorder from nodes package

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

* adding interface checking for arraynode

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

* added transform unit test

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

* fixed input bindings issue

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

* fixed unit tests

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

* fixed unit tests

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

* go generate

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

* addressing random TODO

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

* fixed unit tests

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

* addressing pr comments

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

---------

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