Remove LiteralTypeForLiteral by creating IsInstance function
#5909
Conversation
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5909 +/- ##
==========================================
- Coverage 37.02% 36.96% -0.06%
==========================================
Files 1317 1317
Lines 132523 132564 +41
==========================================
- Hits 49066 49007 -59
- Misses 79211 79305 +94
- Partials 4246 4252 +6
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 <michaellai901026@gmail.com>
|
@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. |
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
Mecoli1219
changed the title
LiteralTypeForLiteral by creating IsInstance functionLiteralTypeForLiteral by creating IsInstance function
|
Updated |
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
Code Review Agent Run #410df5Actionable Suggestions - 9
Additional Suggestions - 7
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
| if lit == nil { | ||
| return true |
There was a problem hiding this comment.
Consider handling the case where lit.GetScalar() is called after checking lit == nil. If lit is not nil but lit.GetScalar() returns nil, this could lead to a nil pointer dereference in line 223.
Code suggestion
Check the AI-generated fix before applying
}
+ scalar := lit.GetScalar()
+ if scalar == nil {
+ return false
+ }
- _, ok := lit.GetScalar().GetValue().(*core.Scalar_NoneType)
+ _, ok := scalar.GetValue().(*core.Scalar_NoneType)
Code Review Run #410df5
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| func (t mapInstanceChecker) isInstance(lit *core.Literal) bool { | ||
| if _, ok := lit.GetValue().(*core.Literal_Map); !ok { | ||
| return false | ||
| } | ||
| for _, x := range lit.GetMap().GetLiterals() { | ||
| if !IsInstance(x, t.literalType.GetMapValueType()) { | ||
| return false |
There was a problem hiding this comment.
Consider adding validation for nil map literals in isInstance(). Currently, a nil map would cause a panic when accessing GetMap().GetLiterals().
Code suggestion
Check the AI-generated fix before applying
@@ -247,6 +247,10 @@ func (t mapInstanceChecker) isInstance(lit *core.Literal) bool {
if _, ok := lit.GetValue().(*core.Literal_Map); !ok {
return false
}
+ if lit.GetMap() == nil {
+ return false
+ }
+
for _, x := range lit.GetMap().GetLiterals()
Code Review Run #410df5
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| } | ||
| scalar := lit.GetScalar() |
There was a problem hiding this comment.
Consider handling the case where GetOffloadedMetadata() returns nil before accessing GetInferredType() to avoid potential nil pointer dereference.
Code suggestion
Check the AI-generated fix before applying
@@ -288,2 +288,6 @@
+ case *core.Literal_OffloadedMetadata:
+ if l.GetOffloadedMetadata() != nil {
+ return l.GetOffloadedMetadata().GetInferredType()
+ }
+ return nil
Code Review Run #410df5
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| return true | ||
| } |
There was a problem hiding this comment.
Consider adding validation for v.StructuredDataset.GetMetadata().GetStructuredDatasetType() before returning true. The current implementation may allow invalid schema types.
Code suggestion
Check the AI-generated fix before applying
| return true | |
| } | |
| return t.literalType.GetSchema() == nil | |
| } | |
| if v.StructuredDataset.GetMetadata().GetStructuredDatasetType() == nil { | |
| return t.literalType.GetSchema() == nil | |
| } |
Code Review Run #410df5
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| func getInstanceChecker(t *core.LiteralType) instanceChecker { | ||
| switch t.GetType().(type) { | ||
| case *core.LiteralType_CollectionType: | ||
| return collectionInstanceChecker{ | ||
| literalType: t, | ||
| } | ||
| case *core.Literal_Map: | ||
| return &core.LiteralType{ | ||
| Type: &core.LiteralType_MapValueType{ | ||
| MapValueType: literalTypeForLiterals(maps.Values(l.GetMap().GetLiterals())), | ||
| }, | ||
| case *core.LiteralType_MapValueType: | ||
| return mapInstanceChecker{ | ||
| literalType: t, | ||
| } | ||
| case *core.LiteralType_Blob: | ||
| return blobInstanceChecker{ | ||
| literalType: t, | ||
| } | ||
| case *core.LiteralType_Schema: | ||
| return schemaInstanceChecker{ | ||
| literalType: t, | ||
| } | ||
| case *core.LiteralType_UnionType: | ||
| return unionInstanceChecker{ | ||
| literalType: t, | ||
| } | ||
| case *core.LiteralType_StructuredDatasetType: | ||
| return structuredDatasetInstanceChecker{ | ||
| literalType: t, | ||
| } | ||
| default: | ||
| if isNoneType(t) { | ||
| return noneInstanceChecker{} | ||
| } | ||
|
|
||
| return trivialInstanceChecker{ | ||
| literalType: t, | ||
| } | ||
| case *core.Literal_OffloadedMetadata: | ||
| return l.GetOffloadedMetadata().GetInferredType() | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Consider adding error handling for unknown literal types in getInstanceChecker. Currently, it returns a trivialInstanceChecker as default which may not be appropriate for all unknown types.
Code suggestion
Check the AI-generated fix before applying
-
+ // Validate if type is supported
+ if !isSupportedType(t) {
+ panic(fmt.Sprintf("unsupported literal type: %v", t))
+ }
return trivialInstanceChecker{
literalType: t,
}
Code Review Run #410df5
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| @@ -104,7 +103,7 @@ func ReadLargeLiteral(ctx context.Context, datastore *storage.DataStore, | |||
|
|
|||
| // OffloadLargeLiteral offloads the large literal if meets the threshold conditions | |||
| func OffloadLargeLiteral(ctx context.Context, datastore *storage.DataStore, dataReference storage.DataReference, | |||
| toBeOffloaded *idlcore.Literal, literalOffloadingConfig config.LiteralOffloadingConfig) error { | |||
| toBeOffloaded *idlcore.Literal, inferredType *idlcore.LiteralType, literalOffloadingConfig config.LiteralOffloadingConfig) error { | |||
There was a problem hiding this comment.
Consider validating the literalOffloadingConfig parameter before using it in the function. The function uses configuration values without validating if they are valid (e.g. MinSizeInMBForOffloading should not be negative).
Code suggestion
Check the AI-generated fix before applying
@@ -106,6 +106,11 @@
toBeOffloaded *idlcore.Literal, inferredType *idlcore.LiteralType, literalOffloadingConfig config.LiteralOffloadingConfig) error {
+ if literalOffloadingConfig.MinSizeInMBForOffloading < 0 || literalOffloadingConfig.MaxSizeInMBForOffloading < 0 {
+ return fmt.Errorf("invalid offloading config: min/max sizes cannot be negative")
+ }
+ if literalOffloadingConfig.MinSizeInMBForOffloading > literalOffloadingConfig.MaxSizeInMBForOffloading {
+ return fmt.Errorf("invalid offloading config: min size cannot be greater than max size")
+ }
literalSizeBytes := int64(proto.Size(toBeOffloaded))
Code Review Run #410df5
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| lt := LiteralTypeForLiteral(literals) | ||
|
|
||
| assert.True(t, proto.Equal(expectedLt, lt)) | ||
| assert.True(t, IsInstance(literals, expectedLt)) |
There was a problem hiding this comment.
Consider using proto.Equal() for comparing protobuf messages instead of IsInstance(). proto.Equal() is the recommended way to compare protobuf messages for equality as it handles all protobuf specific comparison edge cases.
Code suggestion
Check the AI-generated fix before applying
- assert.True(t, IsInstance(literals, expectedLt))
+ assert.True(t, proto.Equal(LiteralTypeForLiteral(literals), expectedLt))
@@ -379,1 +379,1 @@
- assert.True(t, IsInstance(literals, expectedLt))
+ assert.True(t, proto.Equal(LiteralTypeForLiteral(literals), expectedLt))
@@ -419,1 +419,1 @@
- assert.True(t, IsInstance(literals, expectedLt))
+ assert.True(t, proto.Equal(LiteralTypeForLiteral(literals), expectedLt))
Code Review Run #410df5
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| expectedErrorCode: errors.InvalidArrayLength, | ||
| expectedContainedErrorMsg: "no input array provided", |
There was a problem hiding this comment.
The error code and message have been changed from IDLNotFoundErr with message "Failed to validate literal type" to InvalidArrayLength with message "no input array provided". Consider if this change aligns with the actual error scenario being tested. Similar issues were also found in:
- flytepropeller/pkg/compiler/transformers/k8s/inputs.go (line 38-39)
- flytepropeller/pkg/controller/nodes/catalog/datacatalog/transformer.go (line 56-57)
Code suggestion
Check the AI-generated fix before applying
| expectedErrorCode: errors.InvalidArrayLength, | |
| expectedContainedErrorMsg: "no input array provided", | |
| expectedErrorCode: errors.IDLNotFoundErr, | |
| expectedContainedErrorMsg: "Failed to validate literal type", |
Code Review Run #410df5
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| @@ -595,7 +593,7 @@ func (a *arrayNodeHandler) Handle(ctx context.Context, nCtx interfaces.NodeExecu | |||
| // use the OffloadLargeLiteralKey to create {OffloadLargeLiteralKey}_offloaded_metadata.pb file in the datastore. | |||
| // Update the url in the outputLiteral with the offloaded url and also update the size of the literal. | |||
| offloadedOutputFile := v1alpha1.GetOutputsLiteralMetadataFile(outputLiteralKey, nCtx.NodeStatus().GetOutputDir()) | |||
| if err := common.OffloadLargeLiteral(ctx, nCtx.DataStore(), offloadedOutputFile, outputLiteral, a.literalOffloadingConfig); err != nil { | |||
| if err := common.OffloadLargeLiteral(ctx, nCtx.DataStore(), offloadedOutputFile, outputLiteral, outputLiteralTypes[outputLiteralKey], a.literalOffloadingConfig); err != nil { | |||
There was a problem hiding this comment.
Consider adding validation for outputLiteralTypes[outputLiteralKey] to ensure the map access is safe. The code assumes the key exists in the map which could lead to a panic if the key is missing.
Code suggestion
Check the AI-generated fix before applying
| if err := common.OffloadLargeLiteral(ctx, nCtx.DataStore(), offloadedOutputFile, outputLiteral, outputLiteralTypes[outputLiteralKey], a.literalOffloadingConfig); err != nil { | |
| outputType, exists := outputLiteralTypes[outputLiteralKey] | |
| if !exists { | |
| return fmt.Errorf("output type not found for key %v", outputLiteralKey) | |
| } | |
| if err := common.OffloadLargeLiteral(ctx, nCtx.DataStore(), offloadedOutputFile, outputLiteral, outputType, a.literalOffloadingConfig); err != nil { |
Code Review Run #410df5
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
Code Review Agent Run #f57dfdActionable Suggestions - 1
Review Details
AI Code Review powered by |
| outputLiteralTypes[name] = &idlcore.LiteralType{ | ||
| Type: &idlcore.LiteralType_CollectionType{ | ||
| CollectionType: outputs.GetVariables()[name].GetType(), | ||
| }, | ||
| } |
There was a problem hiding this comment.
Consider adding validation to ensure outputs.GetVariables()[name] exists before accessing its type to avoid potential nil pointer dereference.
Code suggestion
Check the AI-generated fix before applying
| outputLiteralTypes[name] = &idlcore.LiteralType{ | |
| Type: &idlcore.LiteralType_CollectionType{ | |
| CollectionType: outputs.GetVariables()[name].GetType(), | |
| }, | |
| } | |
| if variable, ok := outputs.GetVariables()[name]; ok { | |
| outputLiteralTypes[name] = &idlcore.LiteralType{ | |
| Type: &idlcore.LiteralType_CollectionType{ | |
| CollectionType: variable.GetType(), | |
| }, | |
| } | |
| } |
Code Review Run #f57dfd
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
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 releaseSummary by Bito
This PR implements a significant change in Flyte's type checking system by replacing LiteralTypeForLiteral with IsInstance function. The changes involve refactoring the array handler's type checking mechanism in FlytePropeller, focusing on outputLiteralTypes initialization and collection type assignments. The modification simplifies type checking logic and removes the need to guess LiteralTypes from Literals, while adding safety checks for map key existence. These changes affect both FlyteAdmin and FlytePropeller components.Unit tests added: True
Estimated effort to review (1-5, lower is better): 5