[GitHub] [incubator-tvm] tkonolige commented on a change in pull request #6779: [WIP] Object Schema

[GitBox <gi...@apache.org>]

tkonolige commented on a change in pull request #6779:
URL: https://github.com/apache/incubator-tvm/pull/6779#discussion_r516161496



##########
File path: schema/python/schema/parser.py
##########
@@ -0,0 +1,123 @@
+from .registry import register, lookup
+from .ir import ObjectDef, ObjectRefDef, FieldDef
+from .codegen import generate
+from . import typing as ty
+
+import re
+import ast
+import inspect
+
+
+# root class
+class Object:
+    pass
+
+class ObjectRef:
+    pass
+
+_ObjectBaseDef = ObjectDef("Object", "ObjectRef", base=None)
+register(_ObjectBaseDef)
+
+_ObjectRefBaseDef = ObjectRefDef("ObjectRef", base=None,
+    internal=_ObjectBaseDef)
+register(_ObjectRefBaseDef)
+
+
+class Parser(ast.NodeVisitor):

Review comment:
       You actually can grab the dtype without parsing (this is what I was saying above):
   ```
   In [3]: class PrimExprNode():
      ...:     dtype: int
      ...:
   
   In [4]: PrimExprNode.__annotations__
   Out[4]: {'dtype': int}
   ```
   
   Having `default_sequal_reduce` be `True` when its base class has it `False` is a little confusing. Is there a reason for this? It seems like it breaks them assumptions people have when using inheritance.

##########
File path: schema/python/schema/ir.py
##########
@@ -0,0 +1,54 @@
+import json
+from . import typing as ty
+
+class SchemaIR(object):

Review comment:
       Personally, I cannot see a use case for having a base class here. Maybe we can add it in later if it is needed?

##########
File path: schema/python/schema/codegen.py
##########
@@ -0,0 +1,318 @@
+import re
+import subprocess
+from collections import namedtuple
+from .ir import *
+from .registry import lookup
+from . import typing as ty
+import json
+
+class ExprFunctor:

Review comment:
       It just seems that every place you call visit you know exactly what you are visiting (because there is no subclassing). For example, in
   ```
   for field in obj.fields:
       fields.append(fields_comment[field.name])
       fields.append(self.visit(field))
   ```
   we could just replace `self.visit(field)` with `self.visit_field_def(field)`. I believe the same is true of every other place `self.visit` is used.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org