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.

Sign up for GitHub

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

Jump to bottom

[inductor] refactor: device dispatch inside do_bench #125736

Closed
wants to merge 4 commits into from
Closed

[inductor] refactor: device dispatch inside do_bench #125736

wants to merge 4 commits into from

Conversation

jgong5
Copy link
Collaborator

@jgong5 jgong5 commented May 8, 2024 •
edited

@jgong5 jgong5 mentioned this pull request May 8, 2024
Closed
@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented May 8, 2024 •
edited

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit 1bff9d0 with merge base aaa2f93 (image):

BROKEN TRUNK - The following job 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.

@jgong5 jgong5 mentioned this pull request May 8, 2024
Closed
jgong5 added a commit that referenced this pull request May 8, 2024
@jgong5
[inductor] refactor: device dispatch inside do_bench
e0259c7 
ghstack-source-id: 4b1508e11f340fe03156ded9a9792ad7389b3420
Pull Request resolved: #125736
@jgong5
Update on "[inductor] refactor: device dispatch inside do_bench"
b421a02 
cc voznesenskym penguinwu EikanWang Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire chauhang

[ghstack-poisoned]
@jgong5 jgong5 added the topic: not user facing topic category label May 8, 2024
@jgong5 jgong5 requested a review from shunting314 May 8, 2024 03:38
@jgong5
Update on "[inductor] refactor: device dispatch inside do_bench"
ebd1531 
cc voznesenskym penguinwu EikanWang Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire chauhang

[ghstack-poisoned]
shunting314
shunting314 reviewed May 8, 2024
View reviewed changes
torch/_inductor/runtime/runtime_utils.py Outdated
return do_bench_gpu(lambda: fn(*fn_args, **fn_kwargs), **kwargs)


def do_bench_gpu(fn, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change *args to fn? I think the former way is more flexible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@jgong5
Update on "[inductor] refactor: device dispatch inside do_bench"
1bff9d0 
cc voznesenskym penguinwu EikanWang Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire chauhang

[ghstack-poisoned]
@jgong5 jgong5 requested a review from shunting314 May 9, 2024 05:31
@jgong5 jgong5 added the ciflow/trunk Trigger trunk jobs on your pull request label May 9, 2024
@jgong5
Copy link
Collaborator Author

jgong5 commented May 9, 2024

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@izaitsevfb
Copy link
Contributor

Hey @jgong5, please note that TestBench also needs to change:

pytorch/test/inductor/test_inductor_utils.py

Line 24 in a174c53

def test_do_bench(self): 

in test_do_bench
    res = do_bench(self._bench_fn)
TypeError: do_bench() missing 2 required positional arguments: 'fn_args' and 'fn_kwargs'

@msaroufim
Copy link
Member

This change also broke the ao CI https://github.com/pytorch/ao/actions/runs/9073922969/job/24931815576?pr=189#step:11:3465

jerryzh168 added a commit to jerryzh168/ao that referenced this pull request May 14, 2024
@jerryzh168
Fix CI after do_bench refactor in pytorch inductor
2c95b52 
Summary:
We are relying on some private APIs from inductor and a recent refactor: pytorch/pytorch#125736 broken the do_bench
API we rely on for autoquant, maybe we should use our own do_bench or rely on triton's directly?

Test Plan:
regression tests
python test/integration/test_integration.py -k test_autoquant_one_input_29_cuda

Reviewers:

Subscribers:

Tasks:

Tags:
@jerryzh168 jerryzh168 mentioned this pull request May 14, 2024
Merged
@jgong5
Copy link
Collaborator Author

jgong5 commented May 14, 2024

Hey @jgong5, please note that TestBench also needs to change:

Thanks for the reminder. Let me fix it with a PR. But may I know why it was not failing the CI?

@jgong5
Copy link
Collaborator Author

jgong5 commented May 14, 2024

This change also broke the ao CI https://github.com/pytorch/ao/actions/runs/9073922969/job/24931815576?pr=189#step:11:3465

Apology for that. Is this what @jerryzh168 is fixing with pytorch/ao#242?

msaroufim pushed a commit to pytorch/ao that referenced this pull request May 14, 2024
@jerryzh168
Fix CI after do_bench refactor in pytorch inductor (#242)
8059da6 
Summary:
We are relying on some private APIs from inductor and a recent refactor: pytorch/pytorch#125736 broken the do_bench
API we rely on for autoquant, maybe we should use our own do_bench or rely on triton's directly?

Test Plan:
regression tests
python test/integration/test_integration.py -k test_autoquant_one_input_29_cuda

Reviewers:

Subscribers:

Tasks:

Tags:
@jerryzh168
Copy link
Contributor

This change also broke the ao CI pytorch/ao/actions/runs/9073922969/job/24931815576?pr=189#step:11:3465

Apology for that. Is this what @jerryzh168 is fixing with pytorch/ao#242?

yeah this PR should fix the issue for torchao, but going forward I feel maybe we want to have a more public do_bench API that everyone can rely on

jgong5 added a commit that referenced this pull request May 14, 2024
@jgong5
[inductor][test] fix do_bench test due to #125736
a07eaff 
ghstack-source-id: 6b4ead58a123f33f8f38ec1c370456e775e0a0d2
Pull Request resolved: #126224
@jgong5
Copy link
Collaborator Author

jgong5 commented May 14, 2024

Hey @jgong5, please note that TestBench also needs to change:

Thanks for the reminder. Let me fix it with a PR. But may I know why it was not failing the CI?

Here is the PR to fix this: #126224. Please review. @izaitsevfb

lancerts pushed a commit to lancerts/ao that referenced this pull request May 17, 2024
@jerryzh168 @lancerts
Fix CI after do_bench refactor in pytorch inductor (pytorch#242)
15fa8ba 
Summary:
We are relying on some private APIs from inductor and a recent refactor: pytorch/pytorch#125736 broken the do_bench
API we rely on for autoquant, maybe we should use our own do_bench or rely on triton's directly?

Test Plan:
regression tests
python test/integration/test_integration.py -k test_autoquant_one_input_29_cuda

Reviewers:

Subscribers:

Tasks:

Tags:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shunting314 shunting314 shunting314 approved these changes

Assignees
No one assigned
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: inductor open source topic: not user facing topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jgong5 @pytorchmergebot @izaitsevfb @msaroufim @jerryzh168 @shunting314 @pytorchbot