Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flytepropeller][flyteadmin] Compiler unknown literal type error handling #5651

Merged
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions flyteadmin/pkg/manager/impl/validation/execution_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package validation

import (
"context"
"fmt"
"regexp"
"strings"

Expand Down Expand Up @@ -107,6 +108,11 @@ func CheckAndFetchInputsForExecution(
default:
inputType = validators.LiteralTypeForLiteral(executionInputMap[name])
}
err := validators.ValidateLiteralType(inputType)
if err != nil {
return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument,
fmt.Sprintf("Failed to validate literal type for %s with err: %s", name, err))
}
if !validators.AreTypesCastable(inputType, expectedInput.GetVar().GetType()) {
return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid %s input wrong type. Expected %s, but got %s", name, expectedInput.GetVar().GetType(), inputType)
}
Expand Down
38 changes: 38 additions & 0 deletions flyteadmin/pkg/manager/impl/validation/execution_validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import (

var execConfig = testutils.GetApplicationConfigWithDefaultDomains()

const failedToValidateLiteralType = "Failed to validate literal type"

func TestValidateExecEmptyProject(t *testing.T) {
request := testutils.GetExecutionRequest()
request.Project = ""
Expand Down Expand Up @@ -209,6 +211,42 @@ func TestValidateExecEmptyInputs(t *testing.T) {
assert.EqualValues(t, expectedMap, actualInputs)
}

func TestValidateExecUnknownIDLInputs(t *testing.T) {
unsupportedLiteral := &core.Literal{
Value: &core.Literal_Scalar{
Scalar: &core.Scalar{},
},
}
defaultInputs := &core.ParameterMap{
Parameters: map[string]*core.Parameter{
"foo": {
Var: &core.Variable{
// 1000 means an unsupported type
Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: 1000}},
},
Behavior: &core.Parameter_Default{
Default: unsupportedLiteral,
},
},
},
}
userInputs := &core.LiteralMap{
Literals: map[string]*core.Literal{
"foo": unsupportedLiteral, // This will lead to a nil inputType
},
}

_, err := CheckAndFetchInputsForExecution(
userInputs,
nil,
defaultInputs,
)
assert.NotNil(t, err)

// Expected error message
assert.Contains(t, err.Error(), failedToValidateLiteralType)
}

func TestValidExecutionId(t *testing.T) {
err := CheckValidExecutionID("abcde123", "a")
assert.Nil(t, err)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package validation

import (
"context"
"fmt"

"google.golang.org/grpc/codes"

Expand Down Expand Up @@ -143,6 +144,11 @@ func checkAndFetchExpectedInputForLaunchPlan(
return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "unexpected fixed_input %s", name)
}
inputType := validators.LiteralTypeForLiteral(fixedInput)
err := validators.ValidateLiteralType(inputType)
if err != nil {
return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument,
fmt.Sprintf("Failed to validate literal type for %s with err: %s", name, err))
}
if !validators.AreTypesCastable(inputType, value.GetType()) {
return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument,
"invalid fixed_input wrong type %s, expected %v, got %v instead", name, value.GetType(), inputType)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,50 @@ func TestGetLpExpectedInvalidFixedInput(t *testing.T) {
assert.Nil(t, actualMap)
}

func TestGetLpExpectedInvalidFixedInputWithUnknownIDL(t *testing.T) {
unsupportedLiteral := &core.Literal{
Value: &core.Literal_Scalar{
Scalar: &core.Scalar{},
},
}
workflowVariableMap := &core.VariableMap{
Variables: map[string]*core.Variable{
"foo": {
Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: 1000}},
},
},
}
defaultInputs := &core.ParameterMap{
Parameters: map[string]*core.Parameter{
"foo": {
Var: &core.Variable{
// 1000 means an unsupported type
Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: 1000}},
},
Behavior: &core.Parameter_Default{
Default: unsupportedLiteral,
},
},
},
}
fixedInputs := &core.LiteralMap{
Literals: map[string]*core.Literal{
"foo": unsupportedLiteral, // This will lead to a nil inputType
},
}

_, err := checkAndFetchExpectedInputForLaunchPlan(
workflowVariableMap,
fixedInputs,
defaultInputs,
)

assert.NotNil(t, err)

// Expected error message
assert.Contains(t, err.Error(), failedToValidateLiteralType)
}

func TestGetLpExpectedNoFixedInput(t *testing.T) {
request := testutils.GetLaunchPlanRequest()
actualMap, err := checkAndFetchExpectedInputForLaunchPlan(
Expand Down
17 changes: 17 additions & 0 deletions flyteadmin/pkg/manager/impl/validation/signal_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import (
"context"
"fmt"

"google.golang.org/grpc/codes"

Expand Down Expand Up @@ -76,6 +77,22 @@
if err != nil {
return err
}
err = propellervalidators.ValidateLiteralType(valueType)
if err != nil {
return errors.NewFlyteAdminErrorf(codes.InvalidArgument,
pingsutw marked this conversation as resolved.
Show resolved Hide resolved
fmt.Sprintf("Failed to validate literal type for %s with err: %s", valueType, err))
}

if valueType == nil {
return errors.NewFlyteAdminErrorf(codes.InvalidArgument,
fmt.Sprintf(
"Invalid signal value for '%s'.\n"+
"Expected type: %s, but received: %s.\n"+
"Suggested solution: Ensure all Flyte images are updated to the latest version and try again.",
request.Value, lookupSignal.GetType().String(), valueType,
),
)

Check warning on line 94 in flyteadmin/pkg/manager/impl/validation/signal_validator.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/pkg/manager/impl/validation/signal_validator.go#L87-L94

Added lines #L87 - L94 were not covered by tests
}
if !propellervalidators.AreTypesCastable(lookupSignal.Type, valueType) {
return errors.NewFlyteAdminErrorf(codes.InvalidArgument,
"requested signal value [%v] is not castable to existing signal type [%v]",
Expand Down
48 changes: 48 additions & 0 deletions flyteadmin/pkg/manager/impl/validation/signal_validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -283,4 +283,52 @@ func TestValidateSignalUpdateRequest(t *testing.T) {
utils.AssertEqualWithSanitizedRegex(t,
"requested signal value [scalar:{ primitive:{ boolean:false } } ] is not castable to existing signal type [[8 1]]", ValidateSignalSetRequest(ctx, repo, request).Error())
})

t.Run("UnknownIDLType", func(t *testing.T) {
ctx := context.TODO()

// Define an unsupported literal type with a simple type of 1000
unsupportedLiteralType := &core.LiteralType{
Type: &core.LiteralType_Simple{
Simple: 1000, // Using 1000 as an unsupported type
},
}
unsupportedLiteralTypeBytes, _ := proto.Marshal(unsupportedLiteralType)

// Mock the repository to return a signal with this unsupported type
repo := repositoryMocks.NewMockRepository()
repo.SignalRepo().(*repositoryMocks.SignalRepoInterface).
OnGetMatch(mock.Anything, mock.Anything).Return(
models.Signal{
Type: unsupportedLiteralTypeBytes, // Set the unsupported type
},
nil,
)

// Set up the unsupported literal that will trigger the nil valueType condition
unsupportedLiteral := &core.Literal{
Value: &core.Literal_Scalar{
Scalar: &core.Scalar{},
},
}

request := admin.SignalSetRequest{
Id: &core.SignalIdentifier{
ExecutionId: &core.WorkflowExecutionIdentifier{
Project: "project",
Domain: "domain",
Name: "name",
},
SignalId: "signal",
},
Value: unsupportedLiteral, // This will lead to valueType being nil
}

// Invoke the function and check for the expected error
err := ValidateSignalSetRequest(ctx, repo, &request)
assert.NotNil(t, err)

// Expected error message
assert.Contains(t, err.Error(), failedToValidateLiteralType)
})
}
15 changes: 3 additions & 12 deletions flyteadmin/pkg/manager/impl/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -283,19 +283,10 @@ func validateParameterMap(inputMap *core.ParameterMap, fieldName string) error {
defaultValue := defaultInput.GetDefault()
if defaultValue != nil {
inputType := validators.LiteralTypeForLiteral(defaultValue)

if inputType == nil {
err := validators.ValidateLiteralType(inputType)
if err != nil {
return errors.NewFlyteAdminErrorf(codes.InvalidArgument,
fmt.Sprintf(
"Flyte encountered an issue while determining\n"+
"the type of the default value for Parameter '%s' in '%s'.\n"+
"Registered type: [%s].\n"+
"Flyte needs to support the latest FlyteIDL to support this type.\n"+
"Suggested solution: Please update all of your Flyte images to the latest version and "+
"try again.",
name, fieldName, defaultInput.GetVar().GetType().String(),
),
)
fmt.Sprintf("Failed to validate literal type for %s with err: %s", name, err))
}

if !validators.AreTypesCastable(inputType, defaultInput.GetVar().GetType()) {
Expand Down
11 changes: 1 addition & 10 deletions flyteadmin/pkg/manager/impl/validation/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -347,16 +347,7 @@ func TestValidateParameterMap(t *testing.T) {
err := validateParameterMap(&exampleMap, fieldName)
assert.Error(t, err)
fmt.Println(err.Error())
expectedErrMsg := fmt.Sprintf(
"Flyte encountered an issue while determining\n"+
"the type of the default value for Parameter '%s' in '%s'.\n"+
"Registered type: [%s].\n"+
"Flyte needs to support the latest FlyteIDL to support this type.\n"+
"Suggested solution: Please update all of your Flyte images to the latest version and "+
"try again.",
name, fieldName, exampleMap.Parameters[name].GetVar().GetType().String(),
)
assert.Equal(t, expectedErrMsg, err.Error())
assert.Contains(t, err.Error(), failedToValidateLiteralType)
})
}

Expand Down
3 changes: 2 additions & 1 deletion flytepropeller/pkg/compiler/errors/compiler_error_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ func TestErrorCodes(t *testing.T) {
UnrecognizedValue: NewUnrecognizedValueErr("", ""),
WorkflowBuildError: NewWorkflowBuildError(errors.New("")),
NoNodesFound: NewNoNodesFoundErr(""),
IDLTypeNotFoundError: NewIDLTypeNotFoundErr("", ""),
}

for key, value := range testCases {
Expand All @@ -48,6 +49,6 @@ func TestIncludeSource(t *testing.T) {

SetConfig(Config{IncludeSource: true})
e = NewCycleDetectedInWorkflowErr("", "")
assert.Equal(t, e.source, "compiler_error_test.go:50")
assert.Equal(t, e.source, "compiler_error_test.go:51")
SetConfig(Config{})
}
11 changes: 11 additions & 0 deletions flytepropeller/pkg/compiler/errors/compiler_errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ const (

// Field not found in the dataclass
FieldNotFoundError ErrorCode = "FieldNotFound"

// IDL not found when variable binding
IDLTypeNotFoundError ErrorCode = "IDLTypeNotFound"
)

func NewBranchNodeNotSpecified(branchNodeID string) *CompileError {
Expand Down Expand Up @@ -218,6 +221,14 @@ func NewMismatchingVariablesErr(nodeID, fromVar, fromType, toVar, toType string)
)
}

func NewIDLTypeNotFoundErr(nodeID, errMsg string) *CompileError {
return newError(
IDLTypeNotFoundError,
errMsg,
nodeID,
)
}

func NewMismatchingBindingsErr(nodeID, sinkParam, expectedType, receivedType string) *CompileError {
return newError(
MismatchingBindings,
Expand Down
10 changes: 10 additions & 0 deletions flytepropeller/pkg/compiler/transformers/k8s/inputs.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package k8s

import (
"fmt"

"k8s.io/apimachinery/pkg/util/sets"

"github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/core"
Expand Down Expand Up @@ -42,6 +44,14 @@ func validateInputs(nodeID common.NodeID, iface *core.TypedInterface, inputs cor
default:
inputType = validators.LiteralTypeForLiteral(inputVal)
}

err := validators.ValidateLiteralType(inputType)
if err != nil {
errMsg := fmt.Sprintf("Failed to validate literal type for %s with err: %s", inputVar, err)
errs.Collect(errors.NewIDLTypeNotFoundErr(nodeID, errMsg))
continue
}

if !validators.AreTypesCastable(inputType, v.Type) {
errs.Collect(errors.NewMismatchingTypesErr(nodeID, inputVar, v.Type.String(), inputType.String()))
continue
Expand Down
54 changes: 54 additions & 0 deletions flytepropeller/pkg/compiler/transformers/k8s/inputs_test.go
Original file line number Diff line number Diff line change
@@ -1 +1,55 @@
package k8s

import (
"testing"

"github.com/stretchr/testify/assert"

"github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/core"
"github.com/flyteorg/flyte/flytepropeller/pkg/compiler/common"
"github.com/flyteorg/flyte/flytepropeller/pkg/compiler/errors"
)

func TestValidateInputs_IDLTypeNotFound(t *testing.T) {
nodeID := common.NodeID("test-node")

iface := &core.TypedInterface{
Inputs: &core.VariableMap{
Variables: map[string]*core.Variable{
"input1": {
Type: &core.LiteralType{
Type: &core.LiteralType_Simple{
Simple: 1000,
},
},
},
},
},
}

inputs := core.LiteralMap{
Literals: map[string]*core.Literal{
"input1": nil, // Set this to nil to trigger the nil case
},
}

errs := errors.NewCompileErrors()
ok := validateInputs(nodeID, iface, inputs, errs)

assert.False(t, ok)
assert.True(t, errs.HasErrors())

idlNotFound := false
var errMsg string
for _, err := range errs.Errors().List() {
if err.Code() == "IDLTypeNotFound" {
idlNotFound = true
errMsg = err.Error()
break
}
}
assert.True(t, idlNotFound, "Expected IDLTypeNotFound error was not found in errors")

expectedContainedErrorMsg := "Failed to validate literal type"
assert.Contains(t, errMsg, expectedContainedErrorMsg)
}
Loading
Loading