Skip to content
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

Add nv_enable_options and nv_disable_options #1432

Merged
merged 14 commits into from
Dec 5, 2024
Merged

Add nv_enable_options and nv_disable_options #1432

merged 14 commits into from
Dec 5, 2024

Conversation

Priya2698
Copy link
Collaborator

@Priya2698 Priya2698 commented Nov 13, 2024

nvFuser version >= 0.2.23 will support setting NVFUSER_ENABLE/NVFUSER_DISABLE through the python frontend using _enable_options and _disable_options flags. This is to enable matmul/linear codegen for nvfuser through Thunder using flags nv_enable_options=["fuse_matmul"], nv_disable_options=["matmul_expr_eval"]
Issue: NVIDIA/Fuser#3022

nvFuser PR: NVIDIA/Fuser#3270

@Priya2698 Priya2698 marked this pull request as ready for review November 13, 2024 06:50
@mruberry mruberry requested a review from jjsjann123 November 13, 2024 16:08
@mruberry
Copy link
Collaborator

Maybe @jjsjann123 or @kevinstephano would like to review this?

Copy link
Collaborator

@jjsjann123 jjsjann123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick on the test. Code change looks good to me otherwise.

thunder/executors/nvfuserex_impl.py Outdated Show resolved Hide resolved
thunder/tests/test_nvfuser.py Show resolved Hide resolved
@t-vi
Copy link
Collaborator

t-vi commented Nov 18, 2024

What would be sane defaults here?
How will user know they need to deviate?
So here we are adding two options that users need to know about and then they need to know the magic strings to be filled in...

@Priya2698
Copy link
Collaborator Author

Priya2698 commented Nov 18, 2024

What would be sane defaults here? How will user know they need to deviate?

By default, no options need to be used.

The primary objective of these options is nvfuser development.
We can enable matmul/linear for nvfuser using nv_enable_matmul/linear flags. Within nvfuser, we can further control the scheduler used for these operators (i.e. whether we use ATen or nvfuser codegen) through NVFUSER_ENABLE/DISABLE environment flags.

This PR coupled with NVIDIA/Fuser#3270 allows us to enable nvfuser codegen for matmul/linear through Thunder using these options. This is to primarily support direct experimentation on nvfuser's matmul scheduler from Thunder.

@mruberry mruberry enabled auto-merge (squash) November 21, 2024 16:20
@mruberry
Copy link
Collaborator

What would be sane defaults here? How will user know they need to deviate? So here we are adding two options that users need to know about and then they need to know the magic strings to be filled in...

These are good questions. I think it's OK for now because these are just developer-facing options to continue experimentation. Maybe we should review how options are presented more generally, to clarify what is for practitioners vs. internal developers? Maybe developer options should be better documented somewhere, too?

@Priya2698
Copy link
Collaborator Author

Priya2698 commented Nov 21, 2024

What would be sane defaults here? How will user know they need to deviate? So here we are adding two options that users need to know about and then they need to know the magic strings to be filled in...

These are good questions. I think it's OK for now because these are just developer-facing options to continue experimentation. Maybe we should review how options are presented more generally, to clarify what is for practitioners vs. internal developers?

We could rename the above options to _nv_enable/disable_options to hint these are not user-facing options?

Maybe developer options should be better documented somewhere, too?

I can add a wiki to the nvfuser page listing our options and/or add those to error messages in nvfuser.
Not sure how Thunder may want to display/document this info.

@mruberry
Copy link
Collaborator

What would be sane defaults here? How will user know they need to deviate? So here we are adding two options that users need to know about and then they need to know the magic strings to be filled in...

These are good questions. I think it's OK for now because these are just developer-facing options to continue experimentation. Maybe we should review how options are presented more generally, to clarify what is for practitioners vs. internal developers?

We could rename the above options to _nv_enable/disable_options to hint these are not user-facing options?

That's a cool idea! Or maybe "developer_nv_enable" or something? Let's not rename anything just yet, but a different naming scheme could make this clearer.

Maybe developer options should be better documented somewhere, too?

I can add a wiki to the nvfuser page listing our options and/or add those to error messages in nvfuser.

Also a great idea!

Again: nothing required now. Maybe the Thunder github repo should havea wiki, too

@t-vi
Copy link
Collaborator

t-vi commented Dec 2, 2024

I wonder if it would be useful to think about making this an option for NVFuser now.
The global options are not good UX and we should not be adding features that need an entire wiki.

@mruberry
Copy link
Collaborator

mruberry commented Dec 2, 2024

I wonder if it would be useful to think about making this an option for NVFuser now. The global options are not good UX and we should not be adding features that need an entire wiki.

I think what we need to follow-up on is that this isn't UX, it's DevX, and maybe we need to do something to make that clearer going forward.

@lantiga
Copy link
Collaborator

lantiga commented Dec 3, 2024

Fair that it's DevX, but it will easily become UX if we don't pay attention. Like I can totally see these options percolate into reference models, benchmarks, etc just because they are there. So, we can merge, but then we will need to call out when these are actually used in the wild because it could spiral out of control.

Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's follow-up with how we present developer options. Maybe a technical discussion?

@mruberry mruberry merged commit 771d7dd into main Dec 5, 2024
41 checks passed
@mruberry mruberry deleted the pm/nvf_options branch December 5, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants