csp.const does not resolve type of heterogeneous containers properly

[AdamGlustein]

Describe the bug

If you have multiple typed elements in a container, we are currently only resolving the type based on the first element. This leads to incorrect behaviour.

To Reproduce

      def g():
          u = csp.const([1,'abc']) # resolved as typing.List[int]
          csp.print('u', u)
      
      print(csp.run(g, starttime=datetime(2020,1,1), endtime=timedelta()))

Gives

 TypeError: Conversions.h:fromPython<long int>:222:TypeError: Invalid int type, expected long (int) got str

Expected behavior

The code above should print [1, 'abc']

Error Message

See above

Runtime Environment

0.0.5
3.12.4 | packaged by conda-forge | (main, Jun 17 2024, 10:23:07) [GCC 12.3.0]
linux

Additional context

[AdamGlustein]

The fix for this would be to check the type of each element and if the container is heterogeneous, assign it the container type (i.e. list or set rather than typing.List[int]).

Pros: it fixes the bug here
Cons: it adds overhead to graph building since we need to iterate on every element in the container. Also, it would be hard to get truly correct; we can only really consider the first element as a "base" and check issubclass on the others.

[robambalu]

This is a tough decision. The current behavior was a conscious decision. The thought process is that having heterogenous containers ( that are native types ) is very rare. I would say over 99.9% of lists in this context would be heterogenous.
For that .1%, the overhead would be to iterate over every list passed into every node / graph which is type checked and

• For resolving tvars, go through the heavy resolution logic + add logic find the most common base ( or just nope out and convert to untyped as you said )
• For type checking, iterating all elements and checking the type

In some existing cases this can add a considerable amount of overhead to graph building time. So we need to consider whether its worth the overhead in the general case in order to account for the edge case. ( Note that your example can be made to work be resolving T up front with csp.using( T = list )( [ 1, 'abc' ] )

Having said all this, I also agree that the current behavior is pretty bad in the example presented

[robambalu]

@ptomecek for thoughts as well

[AdamGlustein]

Finding the common base type is actually quite simple now that I think of it so we should do that (if we choose to pursue this).

[ptomecek]

I'm not convinced it's that simple, but we do have logic already for finding common base types in the TVar handling logic (along with caching of some of this resolution), so we could potentially re-use that logic.

[ptomecek]

Also to the point @robambalu made, I think inputs are almost always uniform, and when they are not and it doesn't work, calling csp.using is pretty straightforward.

[AdamGlustein]

How are they going to know to use csp.using? That requires pretty advanced csp knowledge. There's no easy way for us to catch this and give them a good error message unless we check the values, and at that point we can just solve the problem.

I'm only advocating to do this for type resolution, not type checking. I don't think it's very common for users to be providing massive lists as const inputs. I don't think there's many other places in the code where we actually resolve the type of a container and assign it the annotation (i.e. in curve users have to explicitly state the type as an argument).

[AdamGlustein]

Another fun part of this is that a) fails but b) works.

a)

def g():
    u = csp.const([1, 1.2])
    csp.print('u', u)

b)

def g():
    u = csp.const([1.2, 1])
    csp.print('u', u)