[WIP][DO NOT MERGE] Remove LiteralTypeForLiteral by creating IsInstance function
#5909
Conversation
Signed-off-by: Mecoli1219 <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5909 +/- ##
===========================================
- Coverage 36.95% 22.23% -14.73%
===========================================
Files 1310 8 -1302
Lines 131464 796 -130668
===========================================
- Hits 48587 177 -48410
+ Misses 78656 591 -78065
+ Partials 4221 28 -4193
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Future-Outlier
changed the title
LiteralTypeForLiteral by creating IsInstance functionLiteralTypeForLiteral by creating IsInstance function
| if _, ok := lit.GetValue().(*core.Literal_Collection); !ok { | ||
| return false | ||
| } | ||
| for _, x := range lit.GetCollection().Literals { |
There was a problem hiding this comment.
If we add the tuple literal type but still represent the literal as a collection, how do we check if this collection is a tuple or not
There was a problem hiding this comment.
The type of Literal for tuple will be Literal_Collection, but the LiteralType won't. We'll have to create another new InstanceChecker (perhaps tupleInstanceChecker) to deal with that.
| func IsInstance(lit *core.Literal, t *core.LiteralType) bool { | ||
| instanceChecker := getInstanceChecker(t) | ||
|
|
||
| if lit.GetOffloadedMetadata() != nil { |
There was a problem hiding this comment.
i'd add a condition here to make sure the field is actually present.
There was a problem hiding this comment.
sorry incomplete review... will get back to this later.
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
|
@Future-Outlier I tested the IsInstance with the flytekit's unit tests and integration tests. I couldn't run eager in my environment, and it is not related to this update. Besides that, all test works successfully. |
Tracking issue
#5908
Why are the changes needed?
The current Tuple IDL design is complicated by this function since if we want to guess the
LiteralTypefromLiteral, we have to store the name of the tuple and the name of each field inside the tuple in theLiteral.However, guessing the
LiteralTypeis not necessary for Flyte since whenever we create a new literal, there must be a target type (maybe inputs or outputs, but they will always have a defined type). Also, almost all use cases ofLiteralTypeForLiteralare followed by a functionAreTypesCastable, we could simply combine these two functions into a function determining whether a Literal could be instantiated by a LiteralType. It is really similar toisinstance()function in Python.What changes were proposed in this pull request?
IsInstance()for the flytepropeller to check whether a Literal could be represented by a LiteralType.LiteralTypeForLiteraland update all necessary code in FlyteAdmin & FlytePropeller.LiteralTypeForLiteraltoIsInstanceHow was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Additional Concerns
Someone suggested that we shouldn't merge this PR before the release