KEMBAR78
[graph partition] Add way to register custom rule by zou3519 · Pull Request #163310 · pytorch/pytorch · GitHub
Skip to content

Conversation

@zou3519
Copy link
Contributor

@zou3519 zou3519 commented Sep 19, 2025

This PR adds an experimental way to register a custom rule for if
inductor should partition the graph around an operator.

Test Plan:
- new test

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Sep 19, 2025

🔗 Helpful Links

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

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

⏳ No Failures, 1 Pending

As of commit d13ce51 with merge base f4c33cd (image):
💚 Looks good so far! There are no failures yet. 💚

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

zou3519 added a commit that referenced this pull request Sep 19, 2025
This PR adds an experimental way to register a custom rule for if
inductor should partition the graph around an operator.

Test Plan:
- new test

ghstack-source-id: 6627a21
Pull Request resolved: #163310
This PR adds an experimental way to register a custom rule for if
inductor should partition the graph around an operator.

Test Plan:
- new test

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben

[ghstack-poisoned]
zou3519 added a commit that referenced this pull request Sep 19, 2025
This PR adds an experimental way to register a custom rule for if
inductor should partition the graph around an operator.

Test Plan:
- new test

ghstack-source-id: e1ac4aa
Pull Request resolved: #163310
Copy link

@ProExpertProg ProExpertProg left a comment

Choose a reason for hiding this comment

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

This looks good for dynamically deciding whether to partition nodes for custom ops! The only other thing here would be data-dependent dynamic sizes

@zou3519 zou3519 added ciflow/trunk Trigger trunk jobs on your pull request topic: not user facing topic category labels Sep 19, 2025
`register_should_partition_rule` is currently private and experimental.
Use at your own risk.
"""
assert isinstance(op, (torch._ops.OpOverload, torch._ops.HigherOrderOperator))
Copy link
Contributor

Choose a reason for hiding this comment

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

When would this be torch._ops.HigherOrderOperator ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no clue lol. It is very likely that the HOP part of this doesn't work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to assert against HOP to start. There's not many HOPs that would be here anyways (they're not FallbackKernel)

should_partition_fn = _custom_should_partition_fns[operator]
fx_node = ir_node.get_origin_node()
assert fx_node is not None
_, fake_args, fake_kwargs = (
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, this wont always succeed.. assert on first returned arg ? tbh not sure why i didnt just make this optional return..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I didn't realize the first returned arg was a bool for if this succeeded or not

should_partition_fn = _custom_should_partition_fns[operator]
fx_node = ir_node.get_origin_node()
assert fx_node is not None
_, fake_args, fake_kwargs = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: if the operator doesnt have fixed require strides, it's possible the strides of the inputs that will be passed in at runtime are different than the strides in fake_args fake_kwargs. Not sure it really matters though, non-blocking for land.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yike

Copy link
Contributor

Choose a reason for hiding this comment

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

@IvanKobzarev has an example showing how to get the args with correct strides here: https://github.com/pytorch/pytorch/pull/162377/files#diff-8dfc175f817a04eb10c183509564129a91f32db31190e7b4ad3fd9653337c6f6R3672. but again not landing blocking.

_P = ParamSpec("_P")


_custom_should_partition_fns: weakref.WeakKeyDictionary[
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be anyway of resetting this ? it's fine if not since this is very particular use case and vllm can do it manually. also note this wont be part of fx graph cache but i guess that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

easiest way is torch._inductor.scheduler._custom_should_partition_fns.reset() lol

I'm not sure

@zou3519 zou3519 added this to the 2.9.0 milestone Sep 19, 2025
This PR adds an experimental way to register a custom rule for if
inductor should partition the graph around an operator.

Test Plan:
- new test

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben

[ghstack-poisoned]
zou3519 added a commit that referenced this pull request Sep 19, 2025
This PR adds an experimental way to register a custom rule for if
inductor should partition the graph around an operator.

Test Plan:
- new test

ghstack-source-id: 98eb0f1
Pull Request resolved: #163310
@zou3519
Copy link
Contributor Author

zou3519 commented Sep 19, 2025

@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

zou3519 added a commit that referenced this pull request Sep 20, 2025
This PR adds an experimental way to register a custom rule for if
inductor should partition the graph around an operator.

Test Plan:
- new test

Pull Request resolved: #163310
Approved by: https://github.com/ProExpertProg, https://github.com/BoyuanFeng, https://github.com/eellison
ghstack dependencies: #162117, #162307, #162651
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
This PR adds an experimental way to register a custom rule for if
inductor should partition the graph around an operator.

Test Plan:
- new test

Pull Request resolved: pytorch#163310
Approved by: https://github.com/ProExpertProg, https://github.com/BoyuanFeng, https://github.com/eellison
ghstack dependencies: pytorch#162117, pytorch#162307, pytorch#162651
cleonard530 pushed a commit to cleonard530/pytorch that referenced this pull request Sep 22, 2025
This PR adds an experimental way to register a custom rule for if
inductor should partition the graph around an operator.

Test Plan:
- new test

Pull Request resolved: pytorch#163310
Approved by: https://github.com/ProExpertProg, https://github.com/BoyuanFeng, https://github.com/eellison
ghstack dependencies: pytorch#162117, pytorch#162307, pytorch#162651
huydhn pushed a commit that referenced this pull request Sep 23, 2025
This PR adds an experimental way to register a custom rule for if
inductor should partition the graph around an operator.

Test Plan:
- new test

Pull Request resolved: #163310
Approved by: https://github.com/ProExpertProg, https://github.com/BoyuanFeng, https://github.com/eellison
ghstack dependencies: #162117, #162307, #162651
dsashidh pushed a commit to dsashidh/pytorch that referenced this pull request Sep 26, 2025
This PR adds an experimental way to register a custom rule for if
inductor should partition the graph around an operator.

Test Plan:
- new test

Pull Request resolved: pytorch#163310
Approved by: https://github.com/ProExpertProg, https://github.com/BoyuanFeng, https://github.com/eellison
ghstack dependencies: pytorch#162117, pytorch#162307, pytorch#162651
@github-actions github-actions bot deleted the gh/zou3519/1201/head branch October 20, 2025 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants