-
Notifications
You must be signed in to change notification settings - Fork 52
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
[User Schedule] Add print functions for TensorViews and User Scheduled Fusions #2282
Conversation
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 seems really useful. Thanks! I'll let others chime in on the Python interface itself, but could you also create some documentation? Do we already have a doc for the Python interface? If so, I suppose that should be updated.
@@ -2777,6 +2781,20 @@ void initNvFuserPythonBindings(PyObject* module) { | |||
py::class_<FusionDefinition::SchedOperators> nvf_sched( | |||
fusion_def, "SchedOperators"); | |||
nvf_sched.def(py::init<FusionDefinition*>()); | |||
nvf_sched.def( | |||
"to_string", |
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.
nitpick: let's try to hide this by naming it properly with a leading _
like the other print things.
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 wrap the print functions for FusionDefinition
in nvfuser/__init__.py
like https://github.com/NVIDIA/Fuser/blob/main/nvfuser/__init__.py#L246-L253. I'm not sure if we need the _
if it is a public function for this case.
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.
sorry for nitpicking on this one. I wasn't sure that our intention is to have this as a public API. It's a bit confusing for the to_string
on a pybind Tensor
class, since we do have __repr__
for the bind class, but this to_string
is actually printing out the TensorView.
But that's just a nitpicking. I'm find with keeping this as-is.
@@ -593,6 +593,10 @@ void initNvFuserPythonBindings(PyObject* module) { | |||
"_fusion_ir", | |||
[](FusionDefinition& self) { return self.fusionIr(); }, | |||
py::return_value_policy::reference) | |||
.def( | |||
"_user_schedule_ir", |
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.
IIUC, userScheduleIr can be safely called after a single execution with a custom schedule.
But we can also safely call it outside of def schedule(self):
as well, my question is, is that an intended usage? Otherwise, if we are only supposed to use it inside schedule
, we might as well keep this method under nvf_sched
instead?
I'm not pushing strongly against this, but if we decide to keep the API as-is, I'd highly recommend relaxing the NVF_CHECK above with proper warning/recommendation on proper usage.
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.
It could be useful to print fusion IR outside of the schedule
function. I'll keep it and replace the NVF_CHECK with a string message.
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 moved the method to nvf_sched
since user_sched_
is reset after finalize_schedule
is called and is not available outside of def schedule
.
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.
LGTM, some minor comments on user experience but let's clean up in following PRs.
@@ -2777,6 +2781,20 @@ void initNvFuserPythonBindings(PyObject* module) { | |||
py::class_<FusionDefinition::SchedOperators> nvf_sched( | |||
fusion_def, "SchedOperators"); | |||
nvf_sched.def(py::init<FusionDefinition*>()); | |||
nvf_sched.def( | |||
"to_string", |
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.
sorry for nitpicking on this one. I wasn't sure that our intention is to have this as a public API. It's a bit confusing for the to_string
on a pybind Tensor
class, since we do have __repr__
for the bind class, but this to_string
is actually printing out the TensorView.
But that's just a nitpicking. I'm find with keeping this as-is.
@@ -228,6 +228,18 @@ std::string FusionDefinition::fusionIr() { | |||
return ss.str(); | |||
} | |||
|
|||
std::string FusionDefinition::userScheduleIr() { | |||
NVF_CHECK(id().has_value(), "Invalid fusion definition!"); |
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.
IIUC, vvv
class SomeFusion(FusionDefinition):
def definition(self):
# create some fusion
def schedule(self):
print(self._user_schedule_ir())
fd = SomeFusion()
fd.user_schedule_ir() # it would assert here with `Invalid fusion definition`?
one more nitpicking, this really isn't your problem, but we can probably use a better error message and probably a warning rather than an assert. I think all the other FusionDefinition methods defined in nvfuser/__init__.py
suffers the same.
Keep it as-is in this PR. But I think we can use a clean up PR afterwards. Wdyt @kevinstephano
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.
One last question: What's the story with version bump?
Since we are only adding new APIs here, I'm OK with keeping the version as-is and bump then towards the end of the stacked PRs. (and pretend that they do not exist).
One other thing is, let's add a test as well.
I updated the version and add some tests. |
!build |
…d Fusions (#2282) This PR adds two functions for debugging user scheduled fusions in python frontend. `std::string FusionDefinition::userScheduleIr()` returns the user scheduled fusion IR. Usage: ```python class SomeFusion(FusionDefinition): def definition(self): # create some fusion def schedule(self): print(fd.sched.user_schedule_ir()) ``` `to_string(python_frontend::Tensor)` returns the string representation for its corresponding TensorView. Usage: ```python print(fd.sched.to_string(some_tensor)) ```
This PR adds two functions for debugging user scheduled fusions in python frontend.
std::string FusionDefinition::userScheduleIr()
returns the user scheduled fusion IR.Usage:
to_string(python_frontend::Tensor)
returns the string representation for its corresponding TensorView.Usage:
PR List: