-
Notifications
You must be signed in to change notification settings - Fork 446
[BugFix] Fix JITKernel export_library bug #1699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 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 PR adds runtime module export validation and initialization to the kernel library export process. It verifies the compiled runtime module exists, creates the target directory recursively, exports the module artifact, and logs the export path. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tilelang/jit/kernel.py`:
- Around line 645-649: The code should validate the kernel_file before
performing path operations and calling
self.artifact.rt_mod.export_library(kernel_file); add a guard in the function
containing kernel_file (check that kernel_file is a non-empty, non-whitespace
string) and raise a clear ValueError (consistent with export_ptx/export_sass
behavior) if invalid, then proceed with os.path.dirname/kernel dir creation and
the call to export_library; reference kernel_file and
self.artifact.rt_mod.export_library to locate the change.
🧹 Nitpick comments (1)
tilelang/jit/kernel.py (1)
640-643: Optional: encapsulate the export precondition errorRuff (TRY003) flags long inline messages; consider a small custom exception (or at least a module-level constant) so the message is centralized and reusable.
♻️ Possible refactor
+class RuntimeModuleUnavailableError(AttributeError): + def __init__(self) -> None: + super().__init__( + 'Runtime module is not available. Please compile the kernel with `execution_backend="tvm_ffi"` before exporting.' + ) + @@ - if self.artifact is None or self.artifact.rt_mod is None: - raise AttributeError( - 'Runtime module is not available. Please compile the kernel with `execution_backend="tvm_ffi"` before exporting.' - ) + if self.artifact is None or self.artifact.rt_mod is None: + raise RuntimeModuleUnavailableError()
| dir_path = os.path.dirname(kernel_file) | ||
| if dir_path: | ||
| os.makedirs(dir_path, exist_ok=True) | ||
|
|
||
| self.artifact.rt_mod.export_library(kernel_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate kernel_file before path ops
export_ptx/export_sass reject empty paths; export_library should do the same to avoid confusing export errors with an empty string.
✅ Suggested guard
def export_library(self, kernel_file: str) -> None:
@@
- dir_path = os.path.dirname(kernel_file)
+ if not kernel_file:
+ raise ValueError("kernel_file must be provided to export library")
+
+ dir_path = os.path.dirname(kernel_file)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dir_path = os.path.dirname(kernel_file) | |
| if dir_path: | |
| os.makedirs(dir_path, exist_ok=True) | |
| self.artifact.rt_mod.export_library(kernel_file) | |
| if not kernel_file: | |
| raise ValueError("kernel_file must be provided to export library") | |
| dir_path = os.path.dirname(kernel_file) | |
| if dir_path: | |
| os.makedirs(dir_path, exist_ok=True) | |
| self.artifact.rt_mod.export_library(kernel_file) |
🤖 Prompt for AI Agents
In `@tilelang/jit/kernel.py` around lines 645 - 649, The code should validate the
kernel_file before performing path operations and calling
self.artifact.rt_mod.export_library(kernel_file); add a guard in the function
containing kernel_file (check that kernel_file is a non-empty, non-whitespace
string) and raise a clear ValueError (consistent with export_ptx/export_sass
behavior) if invalid, then proceed with os.path.dirname/kernel dir creation and
the call to export_library; reference kernel_file and
self.artifact.rt_mod.export_library to locate the change.
This pull request improves the robustness and usability of the kernel export functionality in
tilelang/jit/kernel.py. The main change is to add error handling and ensure the target directory exists before exporting the compiled kernel library.Kernel export improvements:
AttributeErrorifself.artifactorself.artifact.rt_modis not available, with a clear error message instructing the user to compile the kernel withexecution_backend="tvm_ffi"before exporting.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.