Kenny/type metadata #370

kennyworkman · 2021-12-02T06:14:41Z

Strip Annotations from TaskTemplate#minor

Strips any annotation from type so runtime task structure remains the same size

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

re:

welcome · 2021-12-02T06:14:43Z

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
Sign off your commits (Reference: DCO Guide).

kennyworkman · 2021-12-02T06:16:50Z

pkg/compiler/transformers/k8s/node.go

@@ -221,6 +221,18 @@ func buildTasks(tasks []*core.CompiledTask, errs errors.CompileErrors) map[commo
 				errs.Collect(errors.NewValueCollisionError(taskID, "Id", taskID))
 			}

+			// We don't want Annotation data available at task runtime for performance.
+			if flyteTask.Template != nil &&


i'm going to be honest this is pretty gross, but don't know the most semantic way of dereferencing nested pointers here given that the whole struct is received as a big chunk from admin

EngHabu

One more change that needs to happen in the compiler is the type check (that checks whether a given output can be passed down as a given input type).. this check relies on the serialized representation of LiteralType here and having annotations in the output but not the input (or vis versa) will fail such validation I believe...

Just list we "nil" the metadata right before that line, we should nil the Annotations (just for checking)...

EngHabu · 2021-12-03T17:33:42Z

pkg/compiler/transformers/k8s/node.go

+				flyteTask.Template.Interface != nil &&
+				flyteTask.Template.Interface.Inputs != nil &&
+				flyteTask.Template.Interface.Inputs.Variables != nil {
+				for _, v := range flyteTask.Template.Interface.Inputs.Variables {


We should probably do the same with outputs for consistency/completeness sake....

addressed with new commit

kennyworkman · 2021-12-09T04:08:46Z

@EngHabu not sure how you can verify the tests pass with github actions alone because the flyteidl source code structure doesnt play well with go mod

i just did this locally to confirm that everything passes with new flyteidl
replace github.com/flyteorg/flyteidl => /Users/kenny/latch/flyteorg/flyteidl

EngHabu · 2021-12-09T05:32:48Z

@EngHabu not sure how you can verify the tests pass with github actions alone because the flyteidl source code structure doesnt play well with go mod

i just did this locally to confirm that everything passes with new flyteidl

replace github.com/flyteorg/flyteidl => /Users/kenny/latch/flyteorg/flyteidl

You can do this instead:

replace github.com/flyteorg/flyteidl => github.com/<fork>flyteidl <Sha>

And this should work fine in CI

kennyworkman · 2021-12-09T06:43:33Z

@EngHabu good to go

codecov · 2021-12-10T00:08:29Z

Codecov Report

Merging #370 (27bd35b) into master (6f4475e) will increase coverage by 0.02%.
The diff coverage is 83.33%.

EngHabu

I don't see this addressed in the PR... please let me know if you want to discuss further

pkg/compiler/transformers/k8s/node.go

go.mod

EngHabu · 2021-12-10T00:24:26Z

Mind deleting pkg/compiler/transformers/k8s/.node_test.go.swp from the PR as well?

Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

EngHabu

Looks good to me!

go.mod

Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

welcome · 2022-02-08T22:37:00Z

Congrats on merging your first pull request! 🎉

* feat: strip annotation from node template Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net> * feat: strip annotation tests Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net> * fix: strip outputs + test Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net> * fix: temp flyteidl for tests Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net> * fix: tmp update flyteidl Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net> * fix: range nil ok Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net> * fix: strip annotation in type compare Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net> * fix: rm .swp Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net> * fix: new flyteidl Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

kennyworkman requested review from EngHabu and kumare3 as code owners December 2, 2021 06:14

kennyworkman commented Dec 2, 2021

View reviewed changes

kennyworkman force-pushed the kenny/type-metadata branch from 249db0b to 43a557e Compare December 2, 2021 06:17

EngHabu reviewed Dec 3, 2021

View reviewed changes

kennyworkman force-pushed the kenny/type-metadata branch from be5d36b to efb424d Compare December 9, 2021 04:09

kennyworkman force-pushed the kenny/type-metadata branch from fe0f37d to 3e96de6 Compare December 9, 2021 06:43

EngHabu reviewed Dec 10, 2021

View reviewed changes

pkg/compiler/transformers/k8s/node.go Outdated Show resolved Hide resolved

EngHabu reviewed Dec 10, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

kennyworkman added 8 commits December 9, 2021 20:11

feat: strip annotation from node template

9e63248

Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

feat: strip annotation tests

ef54fa4

Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

fix: strip outputs + test

ff73e20

Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

fix: temp flyteidl for tests

9b1f7a3

Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

fix: tmp update flyteidl

7696d9c

Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

fix: range nil ok

dcf40e9

Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

fix: strip annotation in type compare

9a54cdd

Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

fix: rm .swp

f8be0a6

Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

kennyworkman force-pushed the kenny/type-metadata branch from 55364a1 to f8be0a6 Compare December 10, 2021 04:11

EngHabu previously approved these changes Dec 10, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

kennyworkman added 2 commits December 9, 2021 20:22

Merge branch 'master' into kenny/type-metadata

1cb7f4d

Merge branch 'master' into kenny/type-metadata

83afa90

kennyworkman dismissed EngHabu’s stale review via 83afa90 February 8, 2022 16:44

kennyworkman added 2 commits February 8, 2022 09:28

fix: new flyteidl

188c91e

Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

Merge remote-tracking branch 'real/master' into kenny/type-metadata

27bd35b

Signed-off-by: Kenny Workman <kennyworkman@sbcglobal.net>

eapolinario approved these changes Feb 8, 2022

View reviewed changes

EngHabu approved these changes Feb 8, 2022

View reviewed changes

EngHabu merged commit 7e8bc5f into flyteorg:master Feb 8, 2022

kennyworkman deleted the kenny/type-metadata branch February 8, 2022 22:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kenny/type metadata #370

Kenny/type metadata #370

kennyworkman commented Dec 2, 2021 •

edited by EngHabu

welcome bot commented Dec 2, 2021

kennyworkman Dec 2, 2021

EngHabu left a comment

EngHabu Dec 3, 2021

kennyworkman Dec 9, 2021

kennyworkman commented Dec 9, 2021

EngHabu commented Dec 9, 2021

kennyworkman commented Dec 9, 2021

codecov bot commented Dec 10, 2021 •

edited

EngHabu left a comment

EngHabu commented Dec 10, 2021

EngHabu left a comment

welcome bot commented Feb 8, 2022

Kenny/type metadata #370

Kenny/type metadata #370

Conversation

kennyworkman commented Dec 2, 2021 • edited by EngHabu

Strip Annotations from TaskTemplate#minor

Type

Are all requirements met?

Complete description

welcome bot commented Dec 2, 2021

kennyworkman Dec 2, 2021

Choose a reason for hiding this comment

EngHabu left a comment

Choose a reason for hiding this comment

EngHabu Dec 3, 2021

Choose a reason for hiding this comment

kennyworkman Dec 9, 2021

Choose a reason for hiding this comment

kennyworkman commented Dec 9, 2021

EngHabu commented Dec 9, 2021

kennyworkman commented Dec 9, 2021

codecov bot commented Dec 10, 2021 • edited

Codecov Report

EngHabu left a comment

Choose a reason for hiding this comment

EngHabu commented Dec 10, 2021

EngHabu left a comment

Choose a reason for hiding this comment

welcome bot commented Feb 8, 2022

kennyworkman commented Dec 2, 2021 •

edited by EngHabu

codecov bot commented Dec 10, 2021 •

edited