gemini-code-assist[bot] commented on code in PR #49: URL: https://github.com/apache/tvm-ffi/pull/49#discussion_r2371464828
########## python/tvm_ffi/utils/prototype.py: ########## @@ -0,0 +1,52 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Helpers that mirros registered TypeInfo into Python dataclass syntax.""" + +from io import StringIO + +from ..core import TypeInfo + + +def prototype_py(type_info: TypeInfo) -> str: + """Generate a Python prototype of the given TypeInfo.""" + assert isinstance(type_info, TypeInfo) + assert type_info.parent_type_info is not None + + cls_name = type_info.type_key.rsplit(".", maxsplit=1)[-1] + parent_type_key = type_info.parent_type_info.type_key + io = StringIO() + print(f""" +from typing import Any +from tvm_ffi import dataclasses as D +from tvm_ffi import core + +PARENT_TYPE = core._lookup_type_info_from_type_key({parent_type_key}).type_cls +assert PARENT_TYPE is not None + +""") Review Comment:  There are a couple of issues in this section that will cause the generated code to be incorrect: 1. The `print` statement writes to `stdout` instead of the `io` buffer, so the generated preamble will be missing from the returned string. You should add `file=io`. 2. The `parent_type_key` on line 37 is not quoted, which will result in invalid Python syntax. It should be formatted as a string literal, for example by using `{parent_type_key!r}`. ```suggestion print(f""" from typing import Any from tvm_ffi import dataclasses as D from tvm_ffi import core PARENT_TYPE = core._lookup_type_info_from_type_key({parent_type_key!r}).type_cls assert PARENT_TYPE is not None """, file=io) ``` ########## python/tvm_ffi/utils/prototype.py: ########## @@ -0,0 +1,52 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Helpers that mirros registered TypeInfo into Python dataclass syntax.""" + +from io import StringIO + +from ..core import TypeInfo + + +def prototype_py(type_info: TypeInfo) -> str: + """Generate a Python prototype of the given TypeInfo.""" + assert isinstance(type_info, TypeInfo) + assert type_info.parent_type_info is not None + + cls_name = type_info.type_key.rsplit(".", maxsplit=1)[-1] + parent_type_key = type_info.parent_type_info.type_key + io = StringIO() + print(f""" +from typing import Any +from tvm_ffi import dataclasses as D +from tvm_ffi import core + +PARENT_TYPE = core._lookup_type_info_from_type_key({parent_type_key}).type_cls +assert PARENT_TYPE is not None + +""") + print(f"@tvm.dataclasses.c_class({type_info.type_key!r})", file=io) + print(f"class {cls_name}(PARENT_TYPE):", file=io) + for field in type_info.fields: + print(f" {field.name}: Any", file=io) + for method in type_info.methods: + name = method.name + if method.is_static == 0: + print(" @staticmethod", file=io) + print(f" def {name}(*args): ...", file=io) + else: + print(f" def {name}(self, *args): ...", file=io) Review Comment:  The logic for identifying static methods is inverted. `method.is_static` is a boolean, but `if method.is_static == 0:` checks if it's false. This results in `@staticmethod` being applied to instance methods. The condition should be `if method.is_static:`. ```suggestion if method.is_static: print(" @staticmethod", file=io) print(f" def {name}(*args): ...", file=io) else: print(f" def {name}(self, *args): ...", file=io) ``` ########## src/ffi/object.cc: ########## @@ -84,18 +84,18 @@ class TypeTable { type_acenstors_data.resize(type_depth); // copy over parent's type information for (int32_t i = 0; i < parent->type_depth; ++i) { - type_acenstors_data[i] = parent->type_acenstors[i]; + type_acenstors_data[i] = parent->type_ancestors[i]; } // set last type information to be parent type_acenstors_data[parent->type_depth] = parent; } - // initialize type info: no change to type_key and type_acenstors fields + // initialize type info: no change to type_key and type_ancestors fields // after this line this->type_index = type_index; this->type_depth = type_depth; this->type_key = TVMFFIByteArray{this->type_key_data.data(), this->type_key_data.length()}; this->type_key_hash = std::hash<String>()(this->type_key_data); - this->type_acenstors = type_acenstors_data.data(); + this->type_ancestors = type_acenstors_data.data(); Review Comment:  There's a naming inconsistency here. While `this->type_ancestors` is now correctly spelled, it's being assigned from `type_acenstors_data`, which still contains the typo. For better code clarity and maintainability, the `type_acenstors_data` member of the `Entry` struct should be renamed to `type_ancestors_data` at its declaration and throughout its usages. ```suggestion this->type_ancestors = type_ancestors_data.data(); ``` ########## python/tvm_ffi/cython/type_info.pxi: ########## @@ -109,6 +109,10 @@ class TypeInfo: type_cls: Optional[type] type_index: int type_key: str + type_ancestors: list[int] fields: list[TypeField] methods: list[TypeMethod] parent_type_info: Optional[TypeInfo] + + def __post_init__(self): + print(f"Type `{self.type_index} @ `{self.type_key}`: ancestors={self.type_ancestors}") Review Comment:  This `__post_init__` method contains a `print` statement that appears to be for debugging. This will write to standard output whenever a `TypeInfo` object is created, which is likely not desirable for a library. Please consider removing this debugging code. ########## python/tvm_ffi/utils/prototype.py: ########## @@ -0,0 +1,52 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Helpers that mirros registered TypeInfo into Python dataclass syntax.""" + +from io import StringIO + +from ..core import TypeInfo + + +def prototype_py(type_info: TypeInfo) -> str: + """Generate a Python prototype of the given TypeInfo.""" + assert isinstance(type_info, TypeInfo) + assert type_info.parent_type_info is not None + + cls_name = type_info.type_key.rsplit(".", maxsplit=1)[-1] + parent_type_key = type_info.parent_type_info.type_key + io = StringIO() + print(f""" +from typing import Any +from tvm_ffi import dataclasses as D +from tvm_ffi import core + +PARENT_TYPE = core._lookup_type_info_from_type_key({parent_type_key}).type_cls +assert PARENT_TYPE is not None + +""") + print(f"@tvm.dataclasses.c_class({type_info.type_key!r})", file=io) Review Comment:  The generated code imports `tvm_ffi.dataclasses` as `D`, but this line uses `@tvm.dataclasses.c_class`. This will cause a `NameError` as `tvm` is not imported. It should use the alias `D`. ```suggestion print(f"@D.c_class({type_info.type_key!r})", file=io) ``` -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
