[dynamo][guards] Consider tensors as immutable for dict tag matches #139560

anijain2305 · 2024-11-02T20:10:19Z

Stack from ghstack (oldest at bottom):

-> [dynamo][guards] Consider tensors as immutable for dict tag matches #139560

This is a bug on the main exposed by #139476

We have dict tag optimization where if the dict tag does not change, we
skip guards on all the items of the dict that are "immutable". We
considered tensors as immutable in such scenarios. This is critical for
guard eval performance, because generally users dont change their
parameters.

If I try to remove this optimization, we see slowdowns, e.g, 3.03x to
2.95x on conv_mixer TIMM benchamrk.

So, I am adding a flag which keeps the current state but allows the
users to remove this optimization. Not ideal, but given how serious guard eval perf has to be,
we are in the gray are of unsoundness vs performance tradeoff.

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames

This is a bug on the main exposed by #139476 We have dict tag optimization where if the dict tag does not change, we skip guards on all the items of the dict that are "immutable". We considered tensors as immutable in such scenarios. This is critical for guard eval performance, because generally users dont change their parameters. If I try to remove this optimization, we see slowdowns, e.g, 3.03x to 2.95x on conv_mixer TIMM benchamrk. So, I am adding a flag which keeps the current state but allows the users to remove this optimization. [ghstack-poisoned]

pytorch-bot · 2024-11-02T20:10:22Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

[DomainsOnly] Jobs fail with GLIBC version not found

✅ No Failures

As of commit 2ff50ef with merge base 41e4d88 ():
💚 Looks good so far! There are no failures yet. 💚

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

This is a bug on the main exposed by #139476 We have dict tag optimization where if the dict tag does not change, we skip guards on all the items of the dict that are "immutable". We considered tensors as immutable in such scenarios. This is critical for guard eval performance, because generally users dont change their parameters. If I try to remove this optimization, we see slowdowns, e.g, 3.03x to 2.95x on conv_mixer TIMM benchamrk. So, I am adding a flag which keeps the current state but allows the users to remove this optimization. ghstack-source-id: 8e654f5 Pull Request resolved: #139560

torch/csrc/dynamo/guards.cpp

…g matches" This is a bug on the main exposed by #139476 We have dict tag optimization where if the dict tag does not change, we skip guards on all the items of the dict that are "immutable". We considered tensors as immutable in such scenarios. This is critical for guard eval performance, because generally users dont change their parameters. If I try to remove this optimization, we see slowdowns, e.g, 3.03x to 2.95x on conv_mixer TIMM benchamrk. So, I am adding a flag which keeps the current state but allows the users to remove this optimization. Not ideal, but given how serious guard eval perf has to be, we are in the gray are of unsoundness vs performance tradeoff. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

This is a bug on the main exposed by #139476 We have dict tag optimization where if the dict tag does not change, we skip guards on all the items of the dict that are "immutable". We considered tensors as immutable in such scenarios. This is critical for guard eval performance, because generally users dont change their parameters. If I try to remove this optimization, we see slowdowns, e.g, 3.03x to 2.95x on conv_mixer TIMM benchamrk. So, I am adding a flag which keeps the current state but allows the users to remove this optimization. ghstack-source-id: be7e9d8 Pull Request resolved: #139560

…g matches" This is a bug on the main exposed by #139476 We have dict tag optimization where if the dict tag does not change, we skip guards on all the items of the dict that are "immutable". We considered tensors as immutable in such scenarios. This is critical for guard eval performance, because generally users dont change their parameters. If I try to remove this optimization, we see slowdowns, e.g, 3.03x to 2.95x on conv_mixer TIMM benchamrk. So, I am adding a flag which keeps the current state but allows the users to remove this optimization. Not ideal, but given how serious guard eval perf has to be, we are in the gray are of unsoundness vs performance tradeoff. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

This is a bug on the main exposed by #139476 We have dict tag optimization where if the dict tag does not change, we skip guards on all the items of the dict that are "immutable". We considered tensors as immutable in such scenarios. This is critical for guard eval performance, because generally users dont change their parameters. If I try to remove this optimization, we see slowdowns, e.g, 3.03x to 2.95x on conv_mixer TIMM benchamrk. So, I am adding a flag which keeps the current state but allows the users to remove this optimization. ghstack-source-id: 86d390f Pull Request resolved: #139560

anijain2305 · 2024-11-03T21:48:08Z

@pytorchbot merge

pytorchmergebot · 2024-11-03T21:49:46Z

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

…ytorch#139560) This is a bug on the main exposed by pytorch#139476 We have dict tag optimization where if the dict tag does not change, we skip guards on all the items of the dict that are "immutable". We considered tensors as immutable in such scenarios. This is critical for guard eval performance, because generally users dont change their parameters. If I try to remove this optimization, we see slowdowns, e.g, 3.03x to 2.95x on conv_mixer TIMM benchamrk. So, I am adding a flag which keeps the current state but allows the users to remove this optimization. Not ideal, but given how serious guard eval perf has to be, we are in the gray are of unsoundness vs performance tradeoff. Pull Request resolved: pytorch#139560 Approved by: https://github.com/jansel

ZainRizvi · 2024-11-05T16:20:49Z

@pytorchbot revert -c ghfirst -m "Sorry but this seems to be breaking internal tests. Please see D65430317 for more details"

pytorchmergebot · 2024-11-05T16:22:20Z

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

…atches (#139560)" This reverts commit e6ff07f. Reverted #139560 on behalf of https://github.com/ZainRizvi due to Sorry but this seems to be breaking internal tests. Please see D65430317 for more details ([comment](#139560 (comment)))

pytorchmergebot · 2024-11-05T16:22:33Z

@anijain2305 your PR has been successfully reverted.

pytorchmergebot · 2024-11-05T18:06:34Z

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

…ytorch#139560) This is a bug on the main exposed by pytorch#139476 We have dict tag optimization where if the dict tag does not change, we skip guards on all the items of the dict that are "immutable". We considered tensors as immutable in such scenarios. This is critical for guard eval performance, because generally users dont change their parameters. If I try to remove this optimization, we see slowdowns, e.g, 3.03x to 2.95x on conv_mixer TIMM benchamrk. So, I am adding a flag which keeps the current state but allows the users to remove this optimization. Not ideal, but given how serious guard eval perf has to be, we are in the gray are of unsoundness vs performance tradeoff. Pull Request resolved: pytorch#139560 Approved by: https://github.com/jansel

anijain2305 · 2024-11-19T17:36:08Z

@pytorchbot revert -m "internal test failures" -c nosignal

pytorchmergebot · 2024-11-19T17:37:35Z

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

pytorchmergebot · 2024-11-19T17:37:47Z

@anijain2305 your PR has been successfully reverted.

…atches (#139560)" This reverts commit b09eb6e. Reverted #139560 on behalf of https://github.com/anijain2305 due to internal test failures ([comment](#139560 (comment)))

aorenste · 2024-11-19T21:03:23Z

Edit - A clean rebuild seems to have fixed it.

This backout seems to have caused a bunch of tests to fail.

anijain2305 · 2024-11-20T01:31:33Z

relanding in #141085

…atches (pytorch#139560)" This reverts commit b09eb6e. Reverted pytorch#139560 on behalf of https://github.com/anijain2305 due to internal test failures ([comment](pytorch#139560 (comment)))

…ytorch#139560) This is a bug on the main exposed by pytorch#139476 We have dict tag optimization where if the dict tag does not change, we skip guards on all the items of the dict that are "immutable". We considered tensors as immutable in such scenarios. This is critical for guard eval performance, because generally users dont change their parameters. If I try to remove this optimization, we see slowdowns, e.g, 3.03x to 2.95x on conv_mixer TIMM benchamrk. So, I am adding a flag which keeps the current state but allows the users to remove this optimization. Not ideal, but given how serious guard eval perf has to be, we are in the gray are of unsoundness vs performance tradeoff. Pull Request resolved: pytorch#139560 Approved by: https://github.com/jansel

…atches (pytorch#139560)" This reverts commit e6ff07f. Reverted pytorch#139560 on behalf of https://github.com/ZainRizvi due to Sorry but this seems to be breaking internal tests. Please see D65430317 for more details ([comment](pytorch#139560 (comment)))

…ytorch#139560) This is a bug on the main exposed by pytorch#139476 We have dict tag optimization where if the dict tag does not change, we skip guards on all the items of the dict that are "immutable". We considered tensors as immutable in such scenarios. This is critical for guard eval performance, because generally users dont change their parameters. If I try to remove this optimization, we see slowdowns, e.g, 3.03x to 2.95x on conv_mixer TIMM benchamrk. So, I am adding a flag which keeps the current state but allows the users to remove this optimization. Not ideal, but given how serious guard eval perf has to be, we are in the gray are of unsoundness vs performance tradeoff. Pull Request resolved: pytorch#139560 Approved by: https://github.com/jansel

…atches (pytorch#139560)" This reverts commit b09eb6e. Reverted pytorch#139560 on behalf of https://github.com/anijain2305 due to internal test failures ([comment](pytorch#139560 (comment)))

…atches (pytorch#139560)" This reverts commit e6ff07f. Reverted pytorch#139560 on behalf of https://github.com/ZainRizvi due to Sorry but this seems to be breaking internal tests. Please see D65430317 for more details ([comment](pytorch#139560 (comment)))

…ytorch#139560) This is a bug on the main exposed by pytorch#139476 We have dict tag optimization where if the dict tag does not change, we skip guards on all the items of the dict that are "immutable". We considered tensors as immutable in such scenarios. This is critical for guard eval performance, because generally users dont change their parameters. If I try to remove this optimization, we see slowdowns, e.g, 3.03x to 2.95x on conv_mixer TIMM benchamrk. So, I am adding a flag which keeps the current state but allows the users to remove this optimization. Not ideal, but given how serious guard eval perf has to be, we are in the gray are of unsoundness vs performance tradeoff. Pull Request resolved: pytorch#139560 Approved by: https://github.com/jansel

…atches (pytorch#139560)" This reverts commit b09eb6e. Reverted pytorch#139560 on behalf of https://github.com/anijain2305 due to internal test failures ([comment](pytorch#139560 (comment)))

…atches (#141085) Reland - #139560 As mentioned in #130341, using `static py::object` can lead to segfaults. I suspect this is the reason for the import system error seen internally (https://www.internalfb.com/sevmanager/view/469592). In this PR, I am removing the `static` part. This is fine and also the right thing to do because this will catch if user changes the flag in the same process for compiling two different functions. Unfortunately, there is no easy way to trigger this segfault, so I can't write a test. Pull Request resolved: #141085 Approved by: https://github.com/jansel Co-authored-by: William Wen <williamwen@meta.com>

…atches (pytorch#141085) Reland - pytorch#139560 As mentioned in pytorch#130341, using `static py::object` can lead to segfaults. I suspect this is the reason for the import system error seen internally (https://www.internalfb.com/sevmanager/view/469592). In this PR, I am removing the `static` part. This is fine and also the right thing to do because this will catch if user changes the flag in the same process for compiling two different functions. Unfortunately, there is no easy way to trigger this segfault, so I can't write a test. Pull Request resolved: pytorch#141085 Approved by: https://github.com/jansel Co-authored-by: William Wen <williamwen@meta.com>

…atches (#141085) Reland - #139560 As mentioned in #130341, using `static py::object` can lead to segfaults. I suspect this is the reason for the import system error seen internally (https://www.internalfb.com/sevmanager/view/469592). In this PR, I am removing the `static` part. This is fine and also the right thing to do because this will catch if user changes the flag in the same process for compiling two different functions. Unfortunately, there is no easy way to trigger this segfault, so I can't write a test. Pull Request resolved: #141085 Approved by: https://github.com/jansel Co-authored-by: William Wen <williamwen@meta.com>

…atches (pytorch#141085) Reland - pytorch#139560 As mentioned in pytorch#130341, using `static py::object` can lead to segfaults. I suspect this is the reason for the import system error seen internally (https://www.internalfb.com/sevmanager/view/469592). In this PR, I am removing the `static` part. This is fine and also the right thing to do because this will catch if user changes the flag in the same process for compiling two different functions. Unfortunately, there is no easy way to trigger this segfault, so I can't write a test. Pull Request resolved: pytorch#141085 Approved by: https://github.com/jansel Co-authored-by: William Wen <williamwen@meta.com>

…atches (#141085) Reland - #139560 As mentioned in #130341, using `static py::object` can lead to segfaults. I suspect this is the reason for the import system error seen internally (https://www.internalfb.com/sevmanager/view/469592). In this PR, I am removing the `static` part. This is fine and also the right thing to do because this will catch if user changes the flag in the same process for compiling two different functions. Unfortunately, there is no easy way to trigger this segfault, so I can't write a test. Pull Request resolved: #141085 Approved by: https://github.com/jansel Co-authored-by: William Wen <williamwen@meta.com>

pytorch-bot bot added ciflow/inductor module: dynamo labels Nov 2, 2024

anijain2305 requested review from jansel and ezyang November 2, 2024 20:10

anijain2305 added ciflow/trunk Trigger trunk jobs on your pull request topic: not user facing topic category labels Nov 2, 2024

jansel requested changes Nov 3, 2024

View reviewed changes

torch/csrc/dynamo/guards.cpp Show resolved Hide resolved

jansel approved these changes Nov 3, 2024

View reviewed changes

pytorchmergebot added the merging label Nov 3, 2024

pytorchmergebot added the Merged label Nov 4, 2024

pytorchmergebot closed this in e6ff07f Nov 4, 2024

pytorchmergebot removed the merging label Nov 4, 2024

anijain2305 mentioned this pull request Nov 4, 2024

[dynamo] "skip_guard_eval_unsafe" API for power users #139038

Closed

pytorchmergebot added Reverted ci-no-td Do not run TD on this PR labels Nov 5, 2024

pytorchmergebot closed this in b09eb6e Nov 5, 2024

pytorchmergebot removed the merging label Nov 5, 2024

pytorchmergebot reopened this Nov 19, 2024

anijain2305 mentioned this pull request Nov 20, 2024

[reland][dynamo][guards] Consider tensors as immutable for dict tag matches #141085

Closed

anijain2305 closed this Nov 20, 2024

github-actions bot deleted the gh/anijain2305/574/head branch December 20, 2024 02:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[dynamo][guards] Consider tensors as immutable for dict tag matches #139560

[dynamo][guards] Consider tensors as immutable for dict tag matches #139560

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[dynamo][guards] Consider tensors as immutable for dict tag matches #139560

[dynamo][guards] Consider tensors as immutable for dict tag matches #139560

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

🔗 Helpful Links

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

❗ 1 Active SEVs

✅ No Failures

Uh oh!

Uh oh!

Uh oh!

Merge started

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Merge started

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!