search

microsoft / knossos-ksc

Compiler with automatic differentiation
Other
45 stars 10 forks source link

RFC: Class property to enable type checking Python "Type" #882

Closed cgravill closed 3 years ago

cgravill commented 3 years ago

Related to AB#19478

I'm not sure this is worth the complexity but it allows type checking with mypy to succeed on type.py

The root problem is the recursive type definition for Type not being very well supported in Python.

On the plus side, this makes handling of e.g. Type.Float and Type.Tensor more consistent.

There are many alternatives, e.g. an auxiliary class after Type, keeping as we were. Thoughts?

sarahnlewis commented 3 years ago

Why do we want to make String etc. properties of class Type as well as instances? i.e., why don't we just put something like this in type.py:

class Type:
  # blah blah

String = Type("String")

and then wherever we need to use String, reference it as type.String (where type is a module) rather than Type.String?

awf commented 3 years ago

Why do we want to make String etc. properties of class Type as well as instances? i.e., why don't we just put something like this in type.py:

class Type:
  # blah blah

String = Type("String")

and then wherever we need to use String, reference it as type.String (where type is a module) rather than Type.String?

And then I presume we would move type.Type.Tensor to type.Tensor, and just import ksc.type... It's a fair suggestion. For now, this preserves consistency, and we can propose the wider refactor of type.Type.* -> type.* as a separate task. (#888)

cgravill commented 3 years ago

Thanks @sarahnlewis @awf yep we could shift the design, I was trying for a minimal effect on callers.

We'd also need to move e.g.

https://github.com/microsoft/knossos-ksc/blob/f7b73e70d647c608e95aa282955fd8cc85ac7ba8/src/python/ksc/type.py#L223-L236

I'm happy to try that route now instead of adding in this new utility function. I haven't spotted anything that requires associating onto the class.

cgravill commented 3 years ago

Also happy to go with suggestion to merge this, then consider the larger change later.

cgravill commented 3 years ago

We'd need to coordinate the case change (module vs class) into RLO e.g. https://github.com/awf/knossos/blob/8b53d51d3a738f131ba289b9fc782370664b421a/src/rlo/sparser.py so perhaps it makes sense to get this proposed change in, get more type checking working across the projects then make the further change with a type checker on our side?

cgravill commented 3 years ago

OK, I'll merge this as a type improvement step. Thanks @sarahnlewis for the suggestion on making this simpler overall, will continue discussion over on #888