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

[Inductor] Cache generated user defined triton kernels on tensor dtype and non tensor parameters #112752

Closed
wants to merge 8 commits into from

Conversation

…e and non tensor parameters

[ghstack-poisoned]
Copy link

pytorch-bot bot commented Nov 2, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/112752

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (3 Unrelated Failures)

As of commit 6cf317d with merge base 7715b47 (image):

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

oulgen added a commit that referenced this pull request Nov 2, 2023
…e and non tensor parameters

ghstack-source-id: 0fa5e4ecdbbd5f82e4bbbe1d04de8424d6fa2f50
Pull Request resolved: #112752
@oulgen
Copy link
Contributor Author

oulgen commented Nov 2, 2023

The cache key needs to be dtype of all tensors and values of all non tensors since our triton autotune implementation assumes every input is implicitly in "keys" list.

@oulgen oulgen added ciflow/trunk Trigger trunk jobs on your pull request topic: not user facing topic category labels Nov 2, 2023
…tensor dtype and non tensor parameters"

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler

[ghstack-poisoned]
original_name = kernel.__name__

cache_key = [original_name]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we run into a (rather weird, but possible) edge case that the user defines two functionally different Triton kernels in different scopes with the same name and things added to the cache_key below?

Copy link
Contributor Author

@oulgen oulgen Nov 3, 2023

Choose a reason for hiding this comment

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

This caching is for the same fx graph so I dont think it is possible for user to unambiguously define two kernels with same name.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the kernel name is enough for a cache key.

from file1 import my_cool_triton_kernel as k1
from file2 import my_cool_triton_kernel as k2

or

import file1
import file2
...
file1.call_my_cool_triton_kernel(...)
file2.call_my_cool_triton_kernel(...)

@aakhundov
Copy link
Contributor

... since our triton autotune implementation assumes every input is implicitly in "keys" list.

Does our autotune implementation not assume that the key doesn't exist? If the key is taken into account, how is it used? Thanks!

@oulgen
Copy link
Contributor Author

oulgen commented Nov 3, 2023

Does our autotune implementation not assume that the key doesn't exist? If the key is taken into account, how is it used? Thanks!

our autotune implementation assumes that all non tensor arguments to kernel are part of the key (because we specialize on all non tensor values -- this is how dynamo works)

…tensor dtype and non tensor parameters"

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler

[ghstack-poisoned]
original_name = kernel.__name__

cache_key = [original_name]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the kernel name is enough for a cache key.

from file1 import my_cool_triton_kernel as k1
from file2 import my_cool_triton_kernel as k2

or

import file1
import file2
...
file1.call_my_cool_triton_kernel(...)
file2.call_my_cool_triton_kernel(...)

…tensor dtype and non tensor parameters"

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler

[ghstack-poisoned]
@oulgen oulgen requested a review from jansel November 6, 2023 04:32
@oulgen
Copy link
Contributor Author

oulgen commented Nov 6, 2023

Swapped over to using code id, added a test case where kernel.__name__ is the same but we do not cache since code id does not match

…tensor dtype and non tensor parameters"

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler

[ghstack-poisoned]
original_name = kernel.__name__

# Distinguish between different functions using function id
cache_key = [id(kernel.fn)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that kernel.fn ends up being GC'ed in Python and we end up with an id that points to nothing? We've run into some situations in the past where something similar has caused issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zou3519 This is not a part of python I am familiar with. What would you recommend using in place of id?
In general, why would kernel.fn get GCed when we hold a reference to it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I read the code a bit more. We should be fine here. As long as we hold a reference to kernel.fn then it won't get GC'ed (and it looks like we keep one in the kernel_side_table).

In some other places in Dynamo (the allowed_functions dict) we use the id of a nested function as a key to a dictionary but don't store a reference to it, resulting in the object getting GC'ed

…tensor dtype and non tensor parameters"

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler

[ghstack-poisoned]
…tensor dtype and non tensor parameters"

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler

[ghstack-poisoned]
…tensor dtype and non tensor parameters"

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler

[ghstack-poisoned]
pytorchmergebot pushed a commit that referenced this pull request Nov 7, 2023
pytorchmergebot pushed a commit that referenced this pull request Nov 7, 2023
pytorchmergebot pushed a commit that referenced this pull request Nov 7, 2023
@facebook-github-bot facebook-github-bot deleted the gh/oulgen/23/head branch November 10, 2023 15:24
Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023
Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023
Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023
Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants