Python: refactor type.Type.* -> type.*?

[awf]

Post #882, all type constructors are static methods on class Type in module type.
Clients then

from type import Type
...
ty = Type.Tensor(...)
...
assert isinstance(ty, Type)

We could lift them to module level to make the API

import type
...
ty = type.Tensor(...)
...
assert isinstance(ty, type.Type)

What are the pros and cons of such a change?

[cgravill]

n.b. coordinate with RLO https://github.com/awf/knossos/blob/8b53d51d3a738f131ba289b9fc782370664b421a/src/rlo/sparser.py if we do make this change

[cgravill]

Here's the best "community expectations" I could find:

https://softwareengineering.stackexchange.com/questions/171296/staticmethod-vs-module-level-function
https://stackoverflow.com/questions/11788195/module-function-vs-staticmethod-vs-classmethod-vs-no-decorators-which-idiom-is

[cgravill]

Another datapont, pylint as configured https://github.com/awf/knossos/pull/1627#issuecomment-868670330 objects to to the classproperty approach - pylint probably needs some more guidance somehow to treat it like staticfunction

src/rlo/type.py:21:4: E0213: Method should have "self" as first argument (no-self-argument)
src/rlo/type.py:21:4: R0201: Method could be a function (no-self-use)

[acl33]

I quite like the namespacing of "Type.Float". Also, I like that before #882, every "Type.Float" returned the same instance, whereas IIUC, after #882, each read of Type.Float returns a new instance. (Can we combine @classproperty with @cachedproperty ??)

But, trying to construct a simple example, akin to the situation before #882:

class Foo:
  def __init__(self, s):
    self.s = s
  def __str__(self):
    return f"Foo<{self.s}>"
Foo.ex = Foo("ex")
print(Foo.ex)

running this prints "Foo" as expected. mypy complains, as expected:

test.py:6: error: "Type[Foo]" has no attribute "ex"
test.py:7: error: "Type[Foo]" has no attribute "ex"
Found 2 errors in 1 file (checked 1 source file)

However, the following patch makes mypy happy:

@@ -1,3 +1,4 @@
 class Foo:
+  ex: "Foo"
   def __init__(self, s):
     self.s = s

is that another option, or do other people dislike the namespacing of the old approach (i.e. #882 is not just about mypy) ?

[cgravill]

Yes, the change will have caused there to be more allocation for e.g. Type.Float.

That looks like another reasonable alternative. As a variation of that, that's more typed (when in Type do as the Typers do...):

class Type
    # ...
    Float: "Type"

    # ...

# After
Type.Float = Type("Float")

Is much closer to the original code and achieves much of the same - mypy / pylance pass, tests pass. Keeps performance of the "primitives" e.g. Type.Float. Has the downside of separating the attribute from it's intended definition and risk of someone doing one without the other, and also away from the other constructors e.g. Type.Tensor

[cgravill]

I'm very happy to do this latest version, sorts out types, passes pylint but also happy with the larger change of module functions if folks prefer that.

[acl33]

I admit to a slight cosmetic preference for Type.Float but we shouldn't let that override more important concerns - what do others think @awf @ryotatomioka ?

[ryotatomioka]

I think I prefer either type.Type.* -> type.* or the last one proposed by Alan. Classproperty seems too complicated for the goal (make mypy happy)

[cgravill]

We've merged @acl33 suggestion as a simpler option than classproperty, can potentially still move to type.* approach suggested by @sarahnlewis at a later date.

[awf]

Closed by #909 ?

[cgravill]

Closed by #909 ?

Depends if we want to consider the more disruptive type.* change that avoids splitting initializing the attributes and defining them.

With #909 it all type checks, passes tests, passes pylint (well, the equivalent change in RLO) so I'm happy to consider it closed.

[acl33]

Thanks @cgravill !
So on the question of type.Type.* -> type.*, just a quick observation:

from ksc.type import Integer, Float

would mean int and float were python types (used in type annotations, for example) and Integer and Float were ksc types (python objects). I guess that's OK. In contrast:

from ksc import type

probably not so good, as that shadows the builtin function type.