tqchen commented on code in PR #717: URL: https://github.com/apache/tvm-ffi/pull/717#discussion_r3887478649
########## docs/concepts/structural_eq_hash.rst: ########## @@ -715,6 +715,14 @@ still managed by the framework — the custom callback only controls *which* sub-values are compared or hashed, *in what order*, and *with what* ``def_region`` flag. +A hook also serves every subclass of the type that registered it. A Review Comment: The main design pt here what do we view as TypeAttr, atm in https://github.com/apache/tvm-ffi/blob/main/include/tvm/ffi/c_api.h#L1307 we use the following simpler view ``` Conceptually, TypeAttr is a dynamic variant of TypeTraits as seen in C++/Rust. It behaves like c++ type_traits, so column[T] does not contain attributes from base classes. ``` From dispatcher side impl pov, not having to do parent fallback would simplify the low-level runtime and keeps reasoning minimal. And for certain behavior like serialization, parent behavior may not directly imply correct child behavior. On the other hand, iheritating certain attribute can be useful part of high-level ergonomics. One possible approach is to separate the concerns: at low-level contract do flat lookup, and in high-level registration, enable path to supply registration that auto registers parent fields with a given list. ########## src/ffi/extra/serialization.cc: ########## @@ -202,8 +202,9 @@ class ObjectGraphSerializer { if (obj->IsInstance<EnumObj>()) return static_cast<const EnumObj*>(obj)->_str_index; static reflection::TypeAttrColumn data_to_json = reflection::TypeAttrColumn(reflection::type_attr::kDataToJson); - if (data_to_json[value.type_index()] != nullptr) { - return data_to_json[value.type_index()].cast<Function>()(value); + AnyView to_json = data_to_json.GetInherited(value.type_index()); Review Comment: I know it is a debatable decision, i still think it is helpful for subclass to explicitly registers the attributes into the table at the ABI level. Whereas the TypeAttr serves as an attr table without boiling down into subclass decision. Subclass do not necessarily mean the carry over of semantics especially in presence of new fields. Doing so would simplify the code size and handling here. One trick that can perserve the same behavior, while being more efficient in the backend, is to be able auto re-register key parent functions in py_class initialization -- 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]
