Update make_artifact to use value type for Union #342

kamalesh0406 · 2022-11-19T02:41:51Z

This PR fixes issue #235, where we display the json summary for the list instead of the actual contents of the list. This code will work well for Optional Types, but it's not perfect.

Suppose the user defines a type as typing.Union[typing.List[int], typing.Tuple[int]] or typing.Union[int, float] etc, we will not get the correct output. However, to fix this we would need to check the type of the value summary and choose the view based on it. I am hesitant to do this because checking types in Typescript for objects that were defined in Python seems weird to me at least.

The ideal solution would be to figure out the value type and return the correct type from the backend instead of the Union type. I would like to get your views on whether I should go for this backend fix instead. Thanks.

neutralino1 · 2022-11-20T23:58:28Z

You make a good point @kamalesh0406. Ideally, we would have access to the actual type of the object, instead of Union.

This is may require some messing around with how we create artifacts. Currently, we always use the function's type hint as artifact type. That's evidently suboptimal for Union types. One option would be to always take the actual value's type here:
https://github.com/sematic-ai/sematic/blob/main/sematic/db/models/factories.py#L145
although we have to make sure this has no unintended consequences.

Another option is to store the type's serialization as part of the JSON summary (I believe we do this for other types, such as List, and dataclass). If the first option works, I would prefer that.

Thoughts @augray, @tscurtu ?

tscurtu · 2022-11-21T08:37:00Z

Storing the Artifact with explicit type information sounds like the optimal solution to me.

One complication / thing we should keep in mind:

Currently we explicitly cast func inputs and outputs to their declared type hint types, but we plan to remove this behavior because it forces us to be rigorous when it comes to every single type, and provide a way to instantiate every single type, limiting support of external types. We intend to merely replace it with a check that the type can be casted.

The removal would lead to an increase in the number of effective types, due to polymorphism and duck typing. Taking this effective un-cast type as the artifact's type could therefore result in handling types which don't have supported UI representations. In this case we could / would need to fall back to the declared type hint again, but we gain hit the ambiguous Union type.

A premature solution proposal would be to have can_cast_type return the effective supported type instead of True, and use that as the Artifact type. For most types, this would be themselves. For Union, it would be one of the member types.

chance-an · 2022-11-21T13:26:36Z

sematic/ui/src/types/Types.tsx

+ if (containsNoneType) {
+ const otherTypes: TypeParamRepr[] = typeRepr[2].args.filter((argsTypeParamRep) => argsTypeParamRep.type[1]!=="NoneType");
+
+ //Union Types in Python should not contain more than two types. Therefore, the otherTypes array should be of length 1.


Union Types in Python should not contain more than two types The said is probably true for the Optional type.

But I am not sure it is always true for a generic Union type. It seems it is allowed to have more than two types.

Eg

Union[Union[int, str], float] == Union[int, str, float]

Agree with Chance--you can have Union of arbitrarily many types in python.

Right, I should have mentioned it as Optional Types in the comments. I will correct it in my next revision. I agree that for the actual Union type we can have multiple possible types as input.

chance-an · 2022-11-21T13:33:17Z

sematic/ui/src/types/Types.tsx

+function UnionValueView(props: ValueViewProps) {
+ const typeRepr = props.typeRepr as AliasTypeRepr;
+
+ if (!typeRepr[2].args) {


Since typeRepr[2].args appears several times, shall we assign it to a variable to give it a meaningful name?

e.g.

let [, , {args: typeArgs} ] = typeRepr

Yup this sounds good. I will fix it in my next revision.

chance-an · 2022-11-21T13:43:08Z

sematic/ui/src/types/Types.tsx

+ return ValueView(props);
+ }
+
+ const noneTypeArgs: TypeParamRepr[] = typeRepr[2].args.filter((argsTypeParamRep) => argsTypeParamRep.type[1]==="NoneType");


or alternatively

.filter(({type:[, typeName]}) => typeName === "NoneType");

chance-an · 2022-11-21T13:48:11Z

For Union, it would be one of the member types.

I like this idea. If some server side processing could reduce it to the exact member type. It is much simpler.

chance-an · 2022-11-21T13:50:42Z

Storing the Artifact with explicit type information sounds like the optimal solution to me

If storage cost is of concern, we can store the artifacts with a type id. The FE could download the specific typing from the API using that id separately.

augray · 2022-11-21T19:17:27Z

One option would be to always take the actual value's type here:
https://github.com/sematic-ai/sematic/blob/main/sematic/db/models/factories.py#L145
although we have to make sure this has no unintended consequences.

This is viable ONLY if we have a reliable way of identifying which type it is, using the declared type hint rather than just type(value). Why? Because the type hint contains information we will need. Consider this:

@sematic.func
def foo(x: Union[List[str], List[int]]) -> None:
    # ...
 
foo(["a", "b", "c"])  # Call A
foo([1, 2, 3])  # Call B

If we just stored type(x), we would wind up storing list for both "Call A" and "Call B", whereas what we should store is List[str] for "Call A" and List[int] for "Call B". I think we will likely want/need Union to be a "first class citizen" of our typing library, and have support for inspecting which one of the union-ed types the value actually corresponds to.

augray · 2022-11-21T19:20:33Z

I think we will likely want/need Union to be a "first class citizen" of our typing library

Given this, it will be a non-trivial change. Perhaps we should discuss in Discord to align on a solution?

kamalesh0406 · 2022-11-21T22:29:01Z

Sure we can discuss on discord about this. But I agree we would need a non-trivial solution for this since this problem is confined to Union types. I feel we would need special helper functions to help us figure out the correct type for the value for Union types, which can be used only for updating the json_summary ?

kamalesh0406 · 2022-11-27T04:24:01Z

I have updated the PR such that we store the actual value type in the artifact instead of using the type hint. I will add a unit test once I get the initial comments on it.

kamalesh0406 · 2022-11-27T04:27:42Z

sematic/types/types/union.py

+def is_union(type_: Any) -> bool:
+ origin_type = get_origin_type(type_)
+
+ return origin_type==Union


I want to know if this equality check is fine with everyone. The other choice would be to use a can_cast here between the input and union type. But based on my understanding, the can_cast function checks if we can cast the type to any of the types specified inside Union and not the Union type itself. Do correct me if I am wrong here.

I think this works, also is works here.

Also, add spaces around the operator:

origin_type is Union

Sure I will update it.

neutralino1

We're almost there, thank you.

neutralino1 · 2022-11-28T18:53:55Z

sematic/types/types/union.py

+def is_union(type_: Any) -> bool:
+ origin_type = get_origin_type(type_)
+
+ return origin_type==Union


I think this works, also is works here.

Also, add spaces around the operator:

origin_type is Union

sematic/db/models/factories.py

chance-an

Great work! Thank you for making the change.

sematic/db/models/tests/test_factories.py

kamalesh0406 · 2023-01-04T09:34:54Z

@chance-an @neutralino1 can you merge it as well, I do not have write access..

tscurtu · 2023-01-04T09:42:44Z

Thank you, @kamalesh0406!

This PR fixes issue sematic-ai#235, where we display the json summary for the list instead of the actual contents of the list. This code will work well for Optional Types, but it's not perfect. Suppose the user defines a type as `typing.Union[typing.List[int], typing.Tuple[int]]` or `typing.Union[int, float]` etc, we will not get the correct output. However, to fix this we would need to check the type of the value summary and choose the view based on it. I am hesitant to do this because checking types in Typescript for objects that were defined in Python seems weird to me at least. The ideal solution would be to figure out the value type and return the correct type from the backend instead of the `Union` type. I would like to get your views on whether I should go for this backend fix instead. Thanks.

tscurtu added bug Something isn't working types Typing system and validation labels Nov 21, 2022

chance-an reviewed Nov 21, 2022

View reviewed changes

tscurtu added the ui UI related work label Nov 22, 2022

kamalesh0406 force-pushed the kamalesh0406/fix-optional-input-display branch from a997150 to 5d1e825 Compare November 27, 2022 04:21

kamalesh0406 changed the title ~~Add UnionValueView to fix issue displaying optional types~~ Update make_artifact to use value type for Union Nov 27, 2022

kamalesh0406 commented Nov 27, 2022

View reviewed changes

kamalesh0406 requested a review from augray November 27, 2022 04:28

neutralino1 requested changes Nov 28, 2022

View reviewed changes

Update make_artifact to use value type for Union

a037b70

kamalesh0406 force-pushed the kamalesh0406/fix-optional-input-display branch from 5d1e825 to a037b70 Compare December 2, 2022 17:25

kamalesh0406 requested review from neutralino1 and removed request for augray December 2, 2022 17:26

Merge branch 'main' into kamalesh0406/fix-optional-input-display

c523103

chance-an approved these changes Jan 3, 2023

View reviewed changes

sematic/db/models/tests/test_factories.py Show resolved Hide resolved

neutralino1 approved these changes Jan 3, 2023

View reviewed changes

tscurtu merged commit 58e4afb into sematic-ai:main Jan 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update make_artifact to use value type for Union #342

Update make_artifact to use value type for Union #342

kamalesh0406 commented Nov 19, 2022

neutralino1 commented Nov 20, 2022

tscurtu commented Nov 21, 2022 •

edited

Loading

chance-an Nov 21, 2022 •

edited

Loading

augray Nov 21, 2022

kamalesh0406 Nov 21, 2022

chance-an Nov 21, 2022

kamalesh0406 Nov 21, 2022

chance-an Nov 21, 2022

chance-an commented Nov 21, 2022

chance-an commented Nov 21, 2022

augray commented Nov 21, 2022

augray commented Nov 21, 2022

kamalesh0406 commented Nov 21, 2022

kamalesh0406 commented Nov 27, 2022

kamalesh0406 Nov 27, 2022

neutralino1 Nov 28, 2022

kamalesh0406 Nov 30, 2022

neutralino1 left a comment

neutralino1 Nov 28, 2022

chance-an left a comment

kamalesh0406 commented Jan 4, 2023

tscurtu commented Jan 4, 2023

Update make_artifact to use value type for Union #342

Update make_artifact to use value type for Union #342

Conversation

kamalesh0406 commented Nov 19, 2022

neutralino1 commented Nov 20, 2022

tscurtu commented Nov 21, 2022 • edited Loading

chance-an Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chance-an commented Nov 21, 2022

chance-an commented Nov 21, 2022

augray commented Nov 21, 2022

augray commented Nov 21, 2022

kamalesh0406 commented Nov 21, 2022

kamalesh0406 commented Nov 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neutralino1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chance-an left a comment

Choose a reason for hiding this comment

kamalesh0406 commented Jan 4, 2023

tscurtu commented Jan 4, 2023

tscurtu commented Nov 21, 2022 •

edited

Loading

chance-an Nov 21, 2022 •

edited

Loading