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

[flytectl][MSGPACK IDL] Gate feature by setting ENV #5976

Merged
merged 3 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions flytecopilot/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ require (
github.com/prometheus/client_model v0.6.1 // indirect
github.com/prometheus/common v0.53.0 // indirect
github.com/prometheus/procfs v0.15.1 // indirect
github.com/shamaton/msgpack/v2 v2.2.2 // indirect
github.com/sirupsen/logrus v1.9.3 // indirect
github.com/spf13/afero v1.8.2 // indirect
github.com/spf13/cast v1.4.1 // indirect
Expand Down
2 changes: 2 additions & 0 deletions flytecopilot/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,8 @@ github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFR
github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8=
github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4=
github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
github.com/shamaton/msgpack/v2 v2.2.2 h1:GOIg0c9LV04VwzOOqZSrmsv/JzjNOOMxnS/HvOHGdgs=
github.com/shamaton/msgpack/v2 v2.2.2/go.mod h1:6khjYnkx73f7VQU7wjcFS9DFjs+59naVWJv1TB7qdOI=
github.com/sirupsen/logrus v1.9.3 h1:dueUQJ1C2q9oE3F7wvmSGAaVtTmUizReu6fjN8uqzbQ=
github.com/sirupsen/logrus v1.9.3/go.mod h1:naHLuLoDiP4jHNo9R0sCBMtWGeIprob74mVsIT4qYEQ=
github.com/spaolacci/murmur3 v0.0.0-20180118202830-f09979ecbc72 h1:qLC7fQah7D6K1B0ujays3HV9gkFtllcxhzImRR7ArPQ=
Expand Down
3 changes: 3 additions & 0 deletions flyteidl/clients/go/coreutils/extract_literal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package coreutils

import (
"os"
"testing"
"time"

Expand Down Expand Up @@ -125,6 +126,7 @@ func TestFetchLiteral(t *testing.T) {
})

t.Run("Generic", func(t *testing.T) {
os.Setenv(FlyteUseOldDcFormat, "true")
literalVal := map[string]interface{}{
"x": 1,
"y": "ystringvalue",
Expand All @@ -150,6 +152,7 @@ func TestFetchLiteral(t *testing.T) {
for key, val := range expectedStructVal.Fields {
assert.Equal(t, val.Kind, extractedStructValue.Fields[key].Kind)
}
os.Unsetenv(FlyteUseOldDcFormat)
})

t.Run("Generic Passed As String", func(t *testing.T) {
Expand Down
36 changes: 30 additions & 6 deletions flyteidl/clients/go/coreutils/literals.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,24 @@
"encoding/json"
"fmt"
"math"
"os"
"reflect"
"strconv"
"strings"
"time"

"github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/core"
"github.com/flyteorg/flyte/flytestdlib/storage"
"github.com/golang/protobuf/jsonpb"
"github.com/golang/protobuf/ptypes"
structpb "github.com/golang/protobuf/ptypes/struct"
"github.com/pkg/errors"
"github.com/shamaton/msgpack/v2"

"github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/core"
"github.com/flyteorg/flyte/flytestdlib/storage"
)

const MESSAGEPACK = "msgpack"
const FlyteUseOldDcFormat = "FLYTE_USE_OLD_DC_FORMAT"

func MakePrimitive(v interface{}) (*core.Primitive, error) {
switch p := v.(type) {
Expand Down Expand Up @@ -561,12 +565,32 @@
strValue = fmt.Sprintf("%.0f", math.Trunc(f))
}
if newT.Simple == core.SimpleType_STRUCT {
useOldFormat := strings.ToLower(os.Getenv(FlyteUseOldDcFormat))
if _, isValueStringType := v.(string); !isValueStringType {
byteValue, err := json.Marshal(v)
if err != nil {
return nil, fmt.Errorf("unable to marshal to json string for struct value %v", v)
if useOldFormat == "1" || useOldFormat == "t" || useOldFormat == "true" {
byteValue, err := json.Marshal(v)
if err != nil {
return nil, fmt.Errorf("unable to marshal to json string for struct value %v", v)
}

Check warning on line 574 in flyteidl/clients/go/coreutils/literals.go

View check run for this annotation

Codecov / codecov/patch

flyteidl/clients/go/coreutils/literals.go#L573-L574

Added lines #L573 - L574 were not covered by tests
strValue = string(byteValue)
} else {
byteValue, err := msgpack.Marshal(v)
if err != nil {
return nil, fmt.Errorf("unable to marshal to msgpack bytes for struct value %v", v)
}

Check warning on line 580 in flyteidl/clients/go/coreutils/literals.go

View check run for this annotation

Codecov / codecov/patch

flyteidl/clients/go/coreutils/literals.go#L579-L580

Added lines #L579 - L580 were not covered by tests
return &core.Literal{
Value: &core.Literal_Scalar{
Scalar: &core.Scalar{
Value: &core.Scalar_Binary{
Binary: &core.Binary{
Value: byteValue,
Tag: MESSAGEPACK,
},
},
},
},
}, nil
}
strValue = string(byteValue)
}
}
lv, err := MakeLiteralForSimpleType(newT.Simple, strValue)
Expand Down
66 changes: 66 additions & 0 deletions flyteidl/clients/go/coreutils/literals_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package coreutils

import (
"fmt"
"os"
"reflect"
"strconv"
"testing"
Expand All @@ -14,6 +15,7 @@ import (
"github.com/golang/protobuf/ptypes"
structpb "github.com/golang/protobuf/ptypes/struct"
"github.com/pkg/errors"
"github.com/shamaton/msgpack/v2"
"github.com/stretchr/testify/assert"

"github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/core"
Expand Down Expand Up @@ -455,6 +457,7 @@ func TestMakeLiteralForType(t *testing.T) {
})

t.Run("Generic", func(t *testing.T) {
os.Setenv(FlyteUseOldDcFormat, "true")
literalVal := map[string]interface{}{
"x": 1,
"y": "ystringvalue",
Expand All @@ -480,6 +483,69 @@ func TestMakeLiteralForType(t *testing.T) {
for key, val := range expectedStructVal.Fields {
assert.Equal(t, val.Kind, extractedStructValue.Fields[key].Kind)
}
os.Unsetenv(FlyteUseOldDcFormat)
})

t.Run("SimpleBinary", func(t *testing.T) {
// We compare the deserialized values instead of the raw msgpack bytes because Go does not guarantee the order
// of map keys during serialization. This means that while the serialized bytes may differ, the deserialized
// values should be logically equivalent.

var literalType = &core.LiteralType{Type: &core.LiteralType_Simple{Simple: core.SimpleType_STRUCT}}
v := map[string]interface{}{
"a": int64(1),
"b": 3.14,
"c": "example_string",
"d": map[string]interface{}{
"1": int64(100),
"2": int64(200),
},
"e": map[string]interface{}{
"a": int64(1),
"b": 3.14,
},
"f": []string{"a", "b", "c"},
}

val, err := MakeLiteralForType(literalType, v)
assert.NoError(t, err)

msgpackBytes, err := msgpack.Marshal(v)
assert.NoError(t, err)

literalVal := &core.Literal{
Value: &core.Literal_Scalar{
Scalar: &core.Scalar{
Value: &core.Scalar_Binary{
Binary: &core.Binary{
Value: msgpackBytes,
Tag: MESSAGEPACK,
},
},
},
},
}

expectedLiteralVal, err := ExtractFromLiteral(literalVal)
assert.NoError(t, err)
actualLiteralVal, err := ExtractFromLiteral(val)
assert.NoError(t, err)

// Check if the extracted value is of type *core.Binary (not []byte)
expectedBinary, ok := expectedLiteralVal.(*core.Binary)
assert.True(t, ok, "expectedLiteralVal is not of type *core.Binary")
actualBinary, ok := actualLiteralVal.(*core.Binary)
assert.True(t, ok, "actualLiteralVal is not of type *core.Binary")

// Now check if the Binary values match
var expectedVal, actualVal map[string]interface{}
err = msgpack.Unmarshal(expectedBinary.Value, &expectedVal)
assert.NoError(t, err)
err = msgpack.Unmarshal(actualBinary.Value, &actualVal)
assert.NoError(t, err)

// Finally, assert that the deserialized values are equal
assert.Equal(t, expectedVal, actualVal)
})

t.Run("ArrayStrings", func(t *testing.T) {
Expand Down
1 change: 1 addition & 0 deletions flyteidl/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ require (
github.com/mitchellh/mapstructure v1.5.0
github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c
github.com/pkg/errors v0.9.1
github.com/shamaton/msgpack/v2 v2.2.2
github.com/spf13/pflag v1.0.5
github.com/stretchr/testify v1.9.0
golang.org/x/net v0.27.0
Expand Down
2 changes: 2 additions & 0 deletions flyteidl/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ github.com/prometheus/procfs v0.15.1/go.mod h1:fB45yRUv8NstnjriLhBQLuOUt+WW4BsoG
github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8=
github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4=
github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
github.com/shamaton/msgpack/v2 v2.2.2 h1:GOIg0c9LV04VwzOOqZSrmsv/JzjNOOMxnS/HvOHGdgs=
github.com/shamaton/msgpack/v2 v2.2.2/go.mod h1:6khjYnkx73f7VQU7wjcFS9DFjs+59naVWJv1TB7qdOI=
github.com/sirupsen/logrus v1.4.2/go.mod h1:tLMulIdttU9McNUspp0xgXVQah82FyeX6MwdIuYE2rE=
github.com/sirupsen/logrus v1.9.3 h1:dueUQJ1C2q9oE3F7wvmSGAaVtTmUizReu6fjN8uqzbQ=
github.com/sirupsen/logrus v1.9.3/go.mod h1:naHLuLoDiP4jHNo9R0sCBMtWGeIprob74mVsIT4qYEQ=
Expand Down
Loading