gemini-code-assist[bot] commented on code in PR #263: URL: https://github.com/apache/tvm-ffi/pull/263#discussion_r2529299213
########## python/tvm_ffi/__init__.py: ########## @@ -60,6 +60,28 @@ # optional module to speedup dlpack conversion from . import _optional_torch_c_dlpack +# import the dtype literals +from ._dtype import ( + bool, + int8, + int16, + int32, + int64, + uint8, + uint16, + uint32, + uint64, + float64, + float32, + float16, + bfloat16, + float8_e4m3fn, + float8_e4m3fnuz, + float8_e5m2, + float8_e5m2fnuz, + float8_e8m0fnu, + float4_e2m1fnx2, +) Review Comment:  These newly exposed dtype literals should be added to the `__all__` list to make them part of the public API. This makes them discoverable via tools like `help()` and allows them to be imported with `from tvm_ffi import *`. This improves API consistency. Remember to use the non-shadowing name for the boolean dtype (e.g., `bool_`) once it's renamed. ########## python/tvm_ffi/__init__.py: ########## @@ -60,6 +60,28 @@ # optional module to speedup dlpack conversion from . import _optional_torch_c_dlpack +# import the dtype literals +from ._dtype import ( + bool, Review Comment:  Importing `bool` from `._dtype` shadows the Python built-in `bool` type. This can lead to very confusing and hard-to-debug errors for users of the library. For example, `isinstance(True, bool)` would evaluate to `False` if a user does `from tvm_ffi import bool`. It is strongly recommended to rename this dtype literal in `_dtype.py` to avoid shadowing a built-in (e.g. to `bool_`) and import the renamed literal here. ```suggestion bool_, ``` ########## tests/python/test_dtype.py: ########## @@ -164,6 +164,28 @@ def test_ml_dtypes_dtype_conversion() -> None: _check_dtype(np.dtype(ml_dtypes.float4_e2m1fn), 17, 4, 1) +def test_builtin_dtype_conversion() -> None: + _check_dtype(tvm_ffi.bool, 6, 8, 1) Review Comment:  This should be updated to use the non-shadowing `bool_` dtype literal, once it's renamed in `_dtype.py` and re-exported from `tvm_ffi/__init__.py`. ```suggestion _check_dtype(tvm_ffi.bool_, 6, 8, 1) ``` ########## python/tvm_ffi/_dtype.py: ########## @@ -321,3 +321,30 @@ def lanes(self) -> int: pass core._set_class_dtype(dtype) + +# list of common dtype literals in machine learning systems apps +# note that we can always cover more dtypes via explicit construction +# from dlpack data type tuple +bool = dtype("bool") Review Comment:  Defining a variable named `bool` shadows the Python built-in `bool` type. This is dangerous and can lead to confusing bugs for users of the library. Please rename it to avoid the name collision. A common convention, used by NumPy, is to add a trailing underscore, e.g., `bool_`. ```suggestion bool_ = dtype("bool") ``` -- 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]
