Conversation
|
🤖 Hi @hikettei, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Summary of ChangesHello @hikettei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the foundational architecture for a new tensor operations framework, focusing on establishing a robust Intermediate Representation (IR) and a flexible backend abstraction. It introduces structured data type definitions, a comprehensive set of IR operations, and an initial CPU backend. The changes are a significant step towards enabling advanced features like polyhedral graph generation and deterministic loop fusion, as indicated by the 'WIP' status and the checklist in the description. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
🤖 I'm sorry @hikettei, but I was unable to process your request. Please see the logs for more details. |
There was a problem hiding this comment.
Code Review
This pull request lays the groundwork for a new tensor library, including data types, an intermediate representation (IR), and a backend dispatch system. While it's clearly a work-in-progress, there are several critical issues such as missing imports, undefined name errors, and incorrect class definitions that will prevent the code from running. My review focuses on addressing these fundamental correctness problems to make the code executable, and also includes suggestions to improve code structure and adhere to Python conventions.
caten/tensor.py
Outdated
| class ATen: | ||
| op: ATenOp # ATen is just a wrapper for ATenOp |
There was a problem hiding this comment.
ATen is instantiated as if it were a dataclass in the apply method, but it's not defined as one. This will cause a TypeError. You should decorate it with @dataclass. Additionally, ATenOp is not defined in this scope and should be referenced as ir.ATenOp.
| class ATen: | |
| op: ATenOp # ATen is just a wrapper for ATenOp | |
| @dataclass(frozen=True) | |
| class ATen: | |
| op: ir.ATenOp # ATen is just a wrapper for ATenOp |
caten/tensor.py
Outdated
| class ATen: | ||
| op: ATenOp # ATen is just a wrapper for ATenOp | ||
| @classmethod | ||
| def from_shape(cls, shape: List[ATenOp]): |
caten/tensor.py
Outdated
| def from_shape(cls, shape: List[ATenOp]): | ||
| return ir.Allocate(shape) # TODO | ||
|
|
||
| def apply(self, op: Callable, *args: List, **kwargs) -> ATen: return ATen(op=op(*args, **kwargs)) |
There was a problem hiding this comment.
The type hint *args: List is invalid syntax. To type variadic positional arguments, you can use *args: Any. It's also good practice to type **kwargs as **kwargs: Any.
| def apply(self, op: Callable, *args: List, **kwargs) -> ATen: return ATen(op=op(*args, **kwargs)) | |
| def apply(self, op: Callable, *args: Any, **kwargs: Any) -> ATen: return ATen(op=op(*args, **kwargs)) |
caten/tensor.py
Outdated
| from typing import Any, Optional, Tuple, Union, ClassVar | ||
| import os |
There was a problem hiding this comment.
Some necessary imports are missing. List and Callable should be imported from typing, and dataclass should be imported from dataclasses to be used as a decorator.
| from typing import Any, Optional, Tuple, Union, ClassVar | |
| import os | |
| from typing import Any, Optional, Tuple, Union, ClassVar, List, Callable | |
| from dataclasses import dataclass | |
| import os |
caten/ir.py
Outdated
|
|
||
| @dataclass(frozen=True) | ||
| class ATenOp(metaclass=ABCMeta): | ||
| args: List[AtenOp] |
caten/dtype.py
Outdated
| @@ -0,0 +1,25 @@ | |||
| from __future__ import annotations | |||
| from dataclasses import dataclass, fields | |||
caten/__init__.py
Outdated
| __all__ = [ | ||
| "dtype", | ||
| "helpers", | ||
| "ir", | ||
| "tensor" | ||
| ] |
There was a problem hiding this comment.
The __all__ list is missing key public APIs like Tensor. To provide a better user experience, it's recommended to export the main classes and functions that users are expected to use directly. This makes them discoverable and allows for imports like from caten import Tensor.
| __all__ = [ | |
| "dtype", | |
| "helpers", | |
| "ir", | |
| "tensor" | |
| ] | |
| __all__ = [ | |
| "dtype", | |
| "helpers", | |
| "ir", | |
| "tensor", | |
| "Tensor", | |
| "get_backend", | |
| "ATenSpec" | |
| ] |
caten/ir.py
Outdated
| def Var(): | ||
| pass |
| impl = DEVICE_TO_TENSOR.get(get_backend()) | ||
| if impl is None: raise ValueError(f"Unknown BACKEND={get_backend()}") |
There was a problem hiding this comment.
For efficiency and clarity, you can store the result of get_backend() in a variable to avoid calling it twice.
| impl = DEVICE_TO_TENSOR.get(get_backend()) | |
| if impl is None: raise ValueError(f"Unknown BACKEND={get_backend()}") | |
| backend = get_backend() | |
| impl = DEVICE_TO_TENSOR.get(backend) | |
| if impl is None: raise ValueError(f"Unknown BACKEND={backend}") |
caten/tensor.py
Outdated
| class range(): | ||
| pass | ||
|
|
||
| class when(): | ||
| pass |
There was a problem hiding this comment.
Uh oh!
There was an error while loading. Please reload this page.