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

Move optimize indexing to use the class Bounds #104558

Closed
wants to merge 13 commits into from
Closed

Move optimize indexing to use the class Bounds #104558

wants to merge 13 commits into from

Conversation

lezcano
Copy link
Collaborator

@lezcano lezcano commented Jul 3, 2023 •
edited by pytorch-bot bot

Stack from ghstack (oldest at bottom):

This PR removes plenty of duplicated code. In particular, it removes the two repeated implementations of get_expr_range, which are superseded by the more correct bound_sympy.

The two duplicated get_expr_ranges were a result of an oversight in #100549.

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @ngimel @yf225 @chenyang78 @kadeng @muchulee8

@lezcano
Move optimize indexing to use the class Bounds
54d61e0 
This PR removes plenty of duplicated code
This was an oversight in #100549.

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Jul 3, 2023 •
edited

🔗 Helpful Links

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

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

❌ 1 New Failure

As of commit 0a359dc:

NEW FAILURE - The following job has failed:

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

This was referenced Jul 3, 2023
Closed
Closed
Closed
Closed
@lezcano lezcano requested a review from eellison July 3, 2023 22:32
@lezcano
Update on "Move optimize indexing to use the class Bounds"
a49944b 
This PR removes plenty of duplicated code
This was an oversight in #100549.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
@lezcano
Update on "Move optimize indexing to use the class Bounds"
b86cf74 
This PR removes plenty of duplicated code
This was an oversight in #100549.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
@lezcano
Update on "Move optimize indexing to use the class Bounds"
75e7ece 
This PR removes plenty of duplicated code
This was an oversight in #100549.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
@lezcano
Update on "Move optimize indexing to use the class Bounds"
1badbe5 
This PR removes plenty of duplicated code
This was an oversight in #100549.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
@lezcano
Update on "Move optimize indexing to use the class Bounds"
b2f2665 
This PR removes plenty of duplicated code
This was an oversight in #100549.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
@lezcano
Update on "Move optimize indexing to use the class Bounds"
1e4e481 
This PR removes plenty of duplicated code
This was an oversight in #100549.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
@lezcano
Update on "Move optimize indexing to use the class Bounds"
e6c2058 
This PR removes plenty of duplicated code
This was an oversight in #100549.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
@lezcano
Update on "Move optimize indexing to use the class Bounds"
6e5ac53 
This PR removes plenty of duplicated code. In particular, it removes the two repeated implementations of `get_expr_range`, which are superseded by the more correct `bound_sympy`.

The two duplicated `get_expr_range`s were a result of an oversight in #100549.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
@lezcano
Update on "Move optimize indexing to use the class Bounds"
5667350 
This PR removes plenty of duplicated code. In particular, it removes the two repeated implementations of `get_expr_range`, which are superseded by the more correct `bound_sympy`.

The two duplicated `get_expr_range`s were a result of an oversight in #100549.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
@lezcano
Copy link
Collaborator Author

lezcano commented Jul 5, 2023

@eellison these three PRs are ready for review.

@lezcano lezcano added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 6, 2023
@lezcano
Update on "Move optimize indexing to use the class Bounds"
50e37b6 
This PR removes plenty of duplicated code. In particular, it removes the two repeated implementations of `get_expr_range`, which are superseded by the more correct `bound_sympy`.

The two duplicated `get_expr_range`s were a result of an oversight in #100549.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng

[ghstack-poisoned]
@lezcano
Update on "Move optimize indexing to use the class Bounds"
e73f7c0 
This PR removes plenty of duplicated code. In particular, it removes the two repeated implementations of `get_expr_range`, which are superseded by the more correct `bound_sympy`.

The two duplicated `get_expr_range`s were a result of an oversight in #100549.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng

[ghstack-poisoned]
eellison
eellison approved these changes Jul 7, 2023
View reviewed changes
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

nice !!

torch/_inductor/bounds.py
@@ -88,7 +19,8 @@ class BoundVars:
def __init__(self, loop_body: LoopBody):
self.loop_body = loop_body
self.replacement_vals = {
k: ValueRanges(0, v) for k, v in loop_body.var_ranges.items()
k: ValueRanges(0, v if not free_symbols(v) else math.inf)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this duplicated with bound_sympy ? we are doing the same logic there no

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sort of. The thing is that here we are bounding operations of an FX graph, while in the other one we are bounding sympy expressions. The logic is pretty much the same though.

@lezcano
Update on "Move optimize indexing to use the class Bounds"
0a359dc 
This PR removes plenty of duplicated code. In particular, it removes the two repeated implementations of `get_expr_range`, which are superseded by the more correct `bound_sympy`.

The two duplicated `get_expr_range`s were a result of an oversight in #100549.

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

[ghstack-poisoned]
@lezcano
Copy link
Collaborator Author

lezcano commented Jul 7, 2023

@pytorchbot merge -f "unrelated error"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

syndicatefilms pushed a commit to syndicatefilms/rooper that referenced this pull request Jul 11, 2023
@lezcano
Move optimize indexing to use the class Bounds
a251159 
This PR removes plenty of duplicated code
This was an oversight in pytorch/pytorch#100549.

ghstack-source-id: c2016602bdc8467be120ea9f790ac5013c4a228d
Pull Request resolved: pytorch/pytorch#104558
@facebook-github-bot facebook-github-bot deleted the gh/Lezcano/203/head branch July 11, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eellison eellison eellison approved these changes

Assignees
No one assigned
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: inductor open source release notes: inductor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lezcano @pytorchmergebot @eellison @pytorchbot