[BUG] Launch plan fails to parse large integer

[maximsmol]

Describe the bug
When running flytectl create --admin.endpoint $ADMIN_URL --admin.insecure execution -p 1 -d development --execFile lp.yaml

using a launchplan with the number 8888888 as an integer parameter,

Error: failed to parse integer value: strconv.ParseInt: parsing "8.888888e+06": invalid syntax

Expected behavior
Launch plan executes parses without an error

Additional context
To Reproduce
Workflow definition:

from enum import Enum
from typing import List

from dataclasses_json import dataclass_json
from flytekit import task, workflow

class Color(Enum):
    RED = "red"
    GREEN = "green"
    BLUE = "blue"


@task
def say_hello(name: str) -> str:
    return f"hello {name}"


@workflow
def my_wf(
    argInt: int,
    argFloat: float,
    argString: str,
    argBool: bool,
    argList: List[int],
    argListListBool: List[List[bool]],
    argEnum: Color,
):
    res = say_hello(name=str(argInt))
    return (argInt, argFloat, argString, argBool, argList, argListListBool, argEnum)

Launch plan:

inputs:
  {
    "argListListBool": [[true, true], [false], [true, false, true]],
    "argList": [123, 456],
    "argInt": 8888888,
    "argBool": false,
    "argFloat": 2312.1312313,
    "argString": "test",
  }
targetDomain: "development"
targetProject: "1"
version: "v5"
workflow: "myapp.workflows.example.my_wf"

Workaround
Replacing "argInt": 8888888 with "argInt": "8888888"

Note
Smaller numbers (such as 888) work fine

P.S.

Not sure by which component this is caused - flyte, flyteadmin, or flytectl - so feel free to move this issue around to a different repo

[welcome]

Thank you for opening your first issue here! 🛠

[katrogan]

@pmahindrakar-oss mind taking a look at this one? this is a valid int input on flyteconsole so i think the execConfig parsing might be breaking somewhere

[pmahindrakar-oss]

Hi @katrogan / @maximsmol ,
I checked the code for this and heres the reason it fails for large numbers.

The json decoder decode the json numbers as float64
https://cs.opensource.google/go/go/+/go1.16.6:src/encoding/json/decode.go;l=96

bool, for JSON booleans
float64, for JSON numbers
string, for JSON strings
[]interface{}, for JSON arrays
map[string]interface{}, for JSON objects
nil for JSON null

Now when Golang uses fmt.Sprintf(“%v”,v) on such values, it automatically converts %g
https://stackoverflow.com/questions/48337330/how-to-print-float-as-string-in-golang-without-scientific-notation

Eg :
v interface{}
v= 8888888
s= Fmt.Sprintf(“%v”, v)
S----> 8.888888e+06

Also we are using fmt.Sprintf(“%v”,v) since the input parameter can be of any type and its converted from string representation here
https://github.com/flyteorg/flyteidl/blob/2537302a1c19f17a0a2834fb9db1d38a4a6eeeba/clients/go/coreutils/literals.go#L491
Want to avoid any type check in flyteidl code and hence the workaround used to read its as string seems right over here.

Let me know if you all agree or if theres a better way

[maximsmol]

Since 8.888888e+06 is just a different (scientific) notation for an integer, imo the correct solution is this:

• Parse the value as float to avoid the limitations of the Go parser
• Verify the value is an integer by checking truncate(x) == x
• Store truncate(x) cast to an integer type as the final parameter value

There should by definition be no difference in behavior between equivalent notations for the same value

---

The fact that strings are accepted for non-string types is a bug if anything and should probably trigger a type error

[katrogan]

Want to avoid any type check in flyteidl code and hence the workaround used to read its as string seems right over here.

I don't think we should be reading it as a string because Flyte is built around type-support and this breaks it, no?

+1 to @maximsmol's suggestions, especially since the user-defined input itself isn't using a different or less conventional notation and we're failing due to go parsing quirks

[pmahindrakar-oss]

Thanks @maximsmol for the suggestion . I have implemented the same here flyteorg/flyteidl#202
@katrogan i agree we should preserve the type support and we do this to most extent while trying to convert it to the mapped type of the param and fail it if it cannot be converted. This one is a corner case and has been handled now. Please have a look.

Also @maximsmol for your suggestion on not allowing to pass it as string at all would require a custom unmarshaller where we compare the unmarshalled json field and check against the flyte object type which might not be trivial with current implementation but will explore more on it.

What we currently have done is allow json to use it default unmarshalling and then enforce the flytes type system as a post step

[kumare3]

@pmahindrakar-oss / @maximsmol can we close this issue?

[pmahindrakar-oss]

Yes this is fixed in the latest release .