-
Notifications
You must be signed in to change notification settings - Fork 15
Fix KernelAgent backend integration issues #205
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
base: main
Are you sure you want to change the base?
Conversation
| ''' | ||
| return kernel_code + wrapper_code | ||
|
|
||
| def _generate_kernel_file_path(self, folder_name: str, attempt: int) -> str: |
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.
so we had some existing setup directory utils IIRC, wouldn't this just duplicate stuff?
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.
We do have a similar function in the llm backend and this function is a duplicate for that.
This kernel_file_path is only used in the llm and kernel_agent backends because those backends requires generating kernels.
Since kernel_file_path is only used in the llm and kernel_agent backends, I could create a new class, GenerativeBackend, which inherits from Backend. I can move the shared functions (e.g., _generate_kernel_file_path) from both llm and kernel_agent into GenerativeBackend. Then, both the LLM and kernel_agent classes can inherit from GenerativeBackend instead of directly from Backend, which will help reduce duplication.
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.
would it not be possible to have the same function?
| f.write(kernel_code) | ||
| try: | ||
| compiled_kernel = self.compile_kernel_from_string(kernel_code, op_name, attempt=attempt) | ||
| self.compiled_kernels[op] = compiled_kernel |
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.
this function means something totally different now, we went from something like write_kernel_to_file to bind the compiled op? so maybe the function just needs a better name
|
|
||
| def generate_kernels(self, suite, daemon=True): | ||
| """Generate kernels for all operators in the suite with comprehensive feedback.""" | ||
| self.daemon = daemon |
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.
what is this doing? I'd have expected a daemon to be a service and not a boolean. Also generally boolean args in a function typically hint that you might need 2 seperate functions
| # Generate and save overall summary | ||
| self._write_overall_summary(successful_ops, total_ops) | ||
|
|
||
| def _write_summary( |
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.
I feel like a few stuff is getting mixed here
- What the eval is expected to output
- What kernelagent is expected to output
- What the specific integration of kernelagent and the eval is supposed to follow
For instance Sahan also had some extensive reporting code in the past, so I'd like to hear some ideas for how to clean all this up
This PR mainly solves KernelAgent integration issues with BackendBench.
Key changes:
--ops relu.defaultnow test onlyaten.relu.default, notaten.leaky_relu.default)Example Usage:
Command:
uv run python BackendBench/scripts/main.py --suite torchbench --backend kernel_agent --ops relu.default,leaky_relu.default --topn 5Result
Folder Structure Example: (some files are skipped for simplicity.)