[flytepropeller] Support attribute access on promises

[ByronHsu]

See flyteorg/flyteidl#439

Tracking Issue

flyteorg/flyte#3864

[pingsutw]

we should be able to know the attribute type if the promise type is list or dict, right?

[ByronHsu]

What if the list contains nested union type?

For example, in type Dict[str, Union[str, Union[str, int]]], we pass x["a"], but we don't know if "a" mapped to str or Union[str, int]

[hamersaw]

We should be able to use the AreTypesCastable function here right? It should handle Union as either a source or destination type. IMO this feature needs type validation, it's something Flyte is very opinionated about and seems very sloppy to leave out.

[hamersaw]

Looks close to me. Great work - I know there are a TON of users interested in this feature!

[hamersaw]

@wild-endeavor is this necessarily true. if we have dataclass_json do we know the type?

[wild-endeavor]

@ByronHsu would you mind just trying to fill this in a bit? If it's really not doable, or if it has a bunch of edge cases, I think it's okay.

Walking through some examples. Let's say the simple case of

@task
def t1() -> List[str]:
    ...

@task
def t2(needs_str: str):
    ...

@workflow
def wf():
    res = t1()
    t2(needs_str=res[5])

In this case t1 will have a TypedInterface output of

        value {
          type {
            collection_type {
              simple: STRING
            }
          }
          description: "o0"
        }

and because it's a string it will type check against the string in t2. The same can be applied for nested lists and dictionaries. As long as they are expressed via collection_type and map_value_type they are relatively easily type-checkable. I assume this is what the code below is doing?

The dataclass case is more complicated

@dataclass_json
@dataclass
class MyDC(object):
    snapshotDate: datetime
    region: str

@task
def t1(needs_dt: datetime):
    ...

@task
def t2(needs_str: str):
    ...

@workflow
def wf(a: MyDC):
    t1(needs_dt=a.snapshotDate)
    t2(needs_str=a.region)

The reason it's more complicated is because the dataclass types are completely obscured (esp. since flyte idl currently doesn't support multi-variate map types). I assume this is why you're skipping checking in the simple/struct case.

Could you see if it's possible though to capture it? Can we

• Add a new field to LiteralType that is only relevant for the scalar case. Maybe just in LiteralType or in TypeStructure.
• Add a nested literal map of the types that's only present in the dataclass case. Just iterate through the fields in dataclass and recursively call the TypeEngine.
• Add the same checking logic in propeller as the normal map_value_type if it's a simple_struct and this new field is present.

What do you think? It will add to the correctness of this new feature. And it will make Dan happy. And in the end, that's what we're all really about.

[hamersaw]

@ByronHsu can you update this?

[eapolinario]

Hi, we are moving all Flyte development to a monorepo. In order to help the transition period, we're moving open PRs to monorepo automatically and your PR was moved to flyteorg/flyte#4150. Notice that if there are any conflicts in the resulting PR they most likely happen due to the change in the import path of the flyte components.