-
Notifications
You must be signed in to change notification settings - Fork 70
Add Fusion.print, which prints the transforms as well as the math #5499
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
Description
|
| Relevant files | |||
|---|---|---|---|
| Enhancement |
|
PR Reviewer Guide
Here are some key observations to aid the review process:
| 🧪 No relevant tests |
| ⚡ Recommended focus areas for review |
Missing Return Value Handling
print method is defined to return None according to its docstring, but it directly calls f.print(std::cout) without explicit handling of return values or side effects. This should be validated to ensure consistency with the intended API behavior and documentation. |
| Returns | ||
| ------- | ||
| None |
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 saw other print_*s return a string instead. I personally found it confusing to have to type print twice, but let me know what you think. One option may be to have an __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.
Returning a string is nice, so you can search through it in tests.
I thought about renaming print_* to something more clear, since string are returned instead being printed to stdout.
You could map CPP print to __repr__ since it is more verbose than print_math. Then, do print(repr(fd.fusion)).
Greptile OverviewGreptile SummaryAdded a new Python binding method Key changes:
Issues found:
Confidence Score: 3/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Python
participant PythonBinding as Python Binding (runtime.cpp)
participant Fusion as Fusion C++ Object
participant IrPrinter
participant IrTransformPrinter
participant stdout
Python->>PythonBinding: fusion.print()
PythonBinding->>Fusion: f.print(std::cout)
Fusion->>stdout: Write "Inputs:"
Fusion->>stdout: Write input tensors
Fusion->>stdout: Write "Outputs:"
Fusion->>stdout: Write output tensors
Fusion->>stdout: Write "\n%kernel {\n"
Fusion->>IrPrinter: op_exprs.handle(this)
IrPrinter->>stdout: Write math expressions
Fusion->>stdout: Write "\nTransformPrinter : \n"
Fusion->>IrTransformPrinter: t_exprs.handle(this)
IrTransformPrinter->>stdout: Write tensor transforms
Fusion->>stdout: Write "} // %kernel\n"
Fusion-->>PythonBinding: Return ostream reference
PythonBinding-->>Python: Return None
|
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.
1 file reviewed, 1 comment
| )") | ||
| .def( | ||
| "print", | ||
| [](Fusion& f) { f.print(std::cout); }, |
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.
style: inconsistent return behavior - print_math and print_kernel both return strings by capturing output in a stringstream, but this new print method prints directly to stdout and returns None. Consider making it consistent:
| [](Fusion& f) { f.print(std::cout); }, | |
| [](Fusion& f) { | |
| std::stringstream ss; | |
| DebugStreamGuard dsg(ss); | |
| f.print(ss); | |
| return ss.str(); | |
| }, |
No description provided.