[Refactor] Move dtypes.py from eager to language and add bits/bytes properties#1646
Conversation
…roperties - Move `tilelang/language/eager/dtypes.py` to `tilelang/language/dtypes.py` since dtype definitions are general-purpose, not eager-specific - Add `bytes` property to dtype (alias for `itemsize`) for convenience - Add type hints for `bits` and `bytes` properties in TYPE_CHECKING block - Create `tilelang/dtypes.py` for direct import via `from tilelang.dtypes import ...` - Update all import paths accordingly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
📝 WalkthroughWalkthroughThe changes reorganize the dtypes module structure by redirecting imports from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tilelang/dtypes.py (1)
1-3: New re-export module looks good, with minor cleanup opportunity.The new
tilelang/dtypes.pymodule successfully provides a convenient top-level import path for dtype definitions. Line 3's explicit imports are redundant since line 2 already imports everything via wildcard, but they serve as useful documentation for the primary public symbols.The static analysis hints suggest the
noqadirectives may be unnecessary if F401 (unused imports) is not enabled in your linter configuration. Consider removing them or verifying your linter settings.♻️ Optional cleanup of noqa directives
If F401 is not enabled in your linter, the noqa directives can be removed:
# Re-export from language.dtypes for convenient access via `from tilelang.dtypes import ...` -from tilelang.language.dtypes import * # noqa: F401, F403 -from tilelang.language.dtypes import dtype, AnyDType, get_tvm_dtype # noqa: F401 +from tilelang.language.dtypes import * # noqa: F403 +from tilelang.language.dtypes import dtype, AnyDType, get_tvm_dtypeAlternatively, if the explicit imports on line 3 are purely for documentation and your team prefers cleaner code, you could remove line 3 entirely since the wildcard already imports these symbols.
tilelang/__init__.py (1)
163-163: Import path update looks correct.The change from
from .language.eager import dtypestofrom .language import dtypescorrectly reflects the module reorganization.The static analysis tool suggests the
noqa: F401directive may be unnecessary. Ifdtypesis properly re-exported (e.g., via__all__or other means), the directive can be removed as cleanup.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
testing/python/language/test_tilelang_language_frontend_v2.pytilelang/__init__.pytilelang/dtypes.pytilelang/jit/adapter/tvm_ffi.pytilelang/language/allocate.pytilelang/language/dtypes.pytilelang/language/eager/__init__.pytilelang/language/eager/ast.pytilelang/language/eager/builder.py
🧰 Additional context used
🧬 Code graph analysis (2)
tilelang/jit/adapter/tvm_ffi.py (1)
tilelang/language/dtypes.py (1)
dtype(14-19)
tilelang/dtypes.py (1)
tilelang/language/dtypes.py (2)
dtype(14-19)get_tvm_dtype(236-239)
🪛 Ruff (0.14.10)
tilelang/__init__.py
163-163: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
tilelang/language/eager/__init__.py
2-2: from ..dtypes import * used; unable to detect undefined names
(F403)
tilelang/dtypes.py
2-2: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
3-3: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
- GitHub Check: Test for Python 3.12 with Nightly-ROCm-7.1 (on self-hosted-amd)
🔇 Additional comments (8)
tilelang/jit/adapter/tvm_ffi.py (1)
22-22: LGTM! Import path correctly updated.The import path change from
tilelang.language.eager.dtypestotilelang.language.dtypesaligns with the PR's objective to move dtype definitions to a more general-purpose location.tilelang/language/eager/ast.py (1)
18-18: LGTM! Relative import correctly updated.The change from
. import dtypesto.. import dtypescorrectly adjusts the relative import to reference the parent package, reflecting the move of dtypes fromtilelang.language.eagertotilelang.language.testing/python/language/test_tilelang_language_frontend_v2.py (1)
43-43: LGTM! Test import updated correctly.The import path for
_all_dtypeshas been correctly updated to reflect the new location of the dtypes module.tilelang/language/eager/builder.py (1)
30-30: LGTM! Relative import correctly adjusted.The change from
. import dtypes as dtto.. import dtypes as dtproperly updates the relative import to reference the parent package, consistent with the dtypes module relocation.tilelang/language/dtypes.py (2)
225-233: Implementation ofbytesproperty looks correct.The
bytesproperty is properly implemented as an alias foritemsize, which is a standard attribute on data types. The implementation follows Python property conventions correctly.
15-18: No changes needed —tvm.DataTypehas a nativebitsproperty.The
bitsproperty is provided bytvm.DataTypeand is widely used throughout the codebase (e.g.,DataType(dtype).bitsin tests and production code). Onlybytesrequired a custom implementation via__dtype_bytes__()since it wasn't available on the underlyingtvm.DataType. The type hints at lines 15-18 are accurate and complete.tilelang/language/allocate.py (1)
31-32: Import path updates are correct.The changes from
from .eager import dtypestofrom . import dtypesand fromfrom .eager.dtypes import dtypetofrom .dtypes import dtypecorrectly reflect the reorganization of the dtypes module from the eager subpackage to the language package level.tilelang/language/eager/__init__.py (1)
2-2: Re-export path updated correctly.The change from
from .dtypes import *tofrom ..dtypes import *properly redirects the re-export to the new centralized dtypes module location in the parent language package, maintaining backward compatibility for code importing fromtilelang.language.eager.Note: The F403 static analysis warning about wildcard imports is expected and acceptable in re-export modules like this one.
Summary
dtypes.pyfromtilelang/language/eager/totilelang/language/since dtype definitions are general-purpose, not eager-specificbytesproperty to dtype as a convenient alias foritemsizetilelang/dtypes.pyfor direct import viafrom tilelang.dtypes import ...Changes
language/dtypes.pylanguage/eager/dtypes.py, addedbytesproperty and type hintstilelang/dtypes.pylanguage/eager/*.py.to..language/allocate.pyjit/adapter/tvm_ffi.pytilelang/__init__.pyTest plan
from tilelang.dtypes import float32, int8, dtypeworksdtype.bitsanddtype.bytesproperties work correctlySummary by CodeRabbit
New Features
bitsandbytesproperties to dtype objects for convenient access to type size information.Refactor
✏️ Tip: You can customize this high-level summary in your review settings.