[AOTInductor] Add config to allow buffer mutation #126584
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/126584
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 ec70e96 with merge base 5fb11cd ( FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
This pull request was exported from Phabricator. Differential Revision: D57514729 |
muchulee8
force-pushed
the
export-D57514729
branch
from
e466565
to
ff1c943
Compare
Summary: Add an additional config to allow buffer mutation. For data that's greater than 2GB, we would need to set it as read-only, otherwise overflow would occur. This is a temporary solution since it won't handle cases that requires mutable data greater than 2GB. Test Plan: Included in commit. Differential Revision: D57514729
|
This pull request was exported from Phabricator. Differential Revision: D57514729 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D57514729 |
muchulee8
force-pushed
the
export-D57514729
branch
from
ff1c943
to
41c7209
Compare
Summary: Pull Request resolved: pytorch#126584 Add an additional config to allow buffer mutation. For data that's greater than 2GB, we would need to set it as read-only, otherwise overflow would occur. This is a temporary solution since it won't handle cases that requires mutable data greater than 2GB. Test Plan: Included in commit. Differential Revision: D57514729
|
This pull request was exported from Phabricator. Differential Revision: D57514729 |
Summary: Pull Request resolved: pytorch#126584 Add an additional config to allow buffer mutation. For data that's greater than 2GB, we would need to set it as read-only, otherwise overflow would occur. This is a temporary solution since it won't handle cases that requires mutable data greater than 2GB. Test Plan: Included in commit. Differential Revision: D57514729
muchulee8
force-pushed
the
export-D57514729
branch
from
41c7209
to
15bf6e2
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57514729 |
muchulee8
force-pushed
the
export-D57514729
branch
from
15bf6e2
to
1e9a36b
Compare
Summary: Pull Request resolved: pytorch#126584 Add an additional config to allow buffer mutation. For data that's greater than 2GB, we would need to set it as read-only, otherwise overflow would occur. This is a temporary solution since it won't handle cases that requires mutable data greater than 2GB. Test Plan: Included in commit. Differential Revision: D57514729
muchulee8
force-pushed
the
export-D57514729
branch
from
1e9a36b
to
cdf2f8d
Compare
Summary: Add an additional config to allow buffer mutation. For data that's greater than 2GB, we would need to set it as read-only, otherwise overflow would occur. This is a temporary solution since it won't handle cases that requires mutable data greater than 2GB. Test Plan: Included in commit. Differential Revision: D57514729
|
This pull request was exported from Phabricator. Differential Revision: D57514729 |
| @@ -1924,12 +1924,13 @@ def _compile_consts_linux(consts: bytes) -> str: | |||
| run_command_and_check(cmd) | |||
| log.debug("aot constant binary command: %s", cmd) | |||
|
|
|||
| # .data section is between .text and .bss. When the size of .data is large, | |||
| # during the linking, the relocation of .text against .bss may overflow. | |||
| # Rename it to .ldata so that it won't be in between the .text and .bss section | |||
There was a problem hiding this comment.
nit - we may still need this comment?
Summary: Add an additional config to allow buffer mutation. For data that's greater than 2GB, we would need to set it as read-only, otherwise overflow would occur. This is a temporary solution since it won't handle cases that requires mutable data greater than 2GB. Test Plan: Included in commit. Differential Revision: D57514729
muchulee8
force-pushed
the
export-D57514729
branch
from
cdf2f8d
to
a1fffe3
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57514729 |
muchulee8
force-pushed
the
export-D57514729
branch
from
a1fffe3
to
276ffed
Compare
Summary: Add an additional config to allow buffer mutation. For data that's greater than 2GB, we would need to set it as read-only, otherwise overflow would occur. This is a temporary solution since it won't handle cases that requires mutable data greater than 2GB. Test Plan: Included in commit. Differential Revision: D57514729
|
This pull request was exported from Phabricator. Differential Revision: D57514729 |
Summary: Add an additional config to allow buffer mutation. For data that's greater than 2GB, we would need to set it as read-only, otherwise overflow would occur. This is a temporary solution since it won't handle cases that requires mutable data greater than 2GB. Test Plan: Included in commit. Differential Revision: D57514729
muchulee8
force-pushed
the
export-D57514729
branch
from
276ffed
to
b498a01
Compare
Summary: Add an additional config to allow buffer mutation. For data that's greater than 2GB, we would need to set it as read-only, otherwise overflow would occur. This is a temporary solution since it won't handle cases that requires mutable data greater than 2GB. Test Plan: Included in commit. Differential Revision: D57514729
muchulee8
force-pushed
the
export-D57514729
branch
from
b498a01
to
ec70e96
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57514729 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D57514729 |
| @@ -744,6 +744,9 @@ class aot_inductor: | |||
| # rather than embedded into the data section. Needed to support 1B+ parameter models | |||
| force_mmap_weights: bool = False | |||
|
|
|||
| # flag to allow buffer mutation. This would remove the read-only property from buffers. | |||
| allow_buffer_mutation: bool = False | |||
There was a problem hiding this comment.
Can we do like "0" if is_fbcode() else "1"? (search for other examples in this file)
There was a problem hiding this comment.
I don't think this is the right thing, it's not only about fbcode.
If you take a look at the log and previous comment Intel folks made, they moved from .data to .ldata because it overflows to .bss. The overflow is already happening with cases in OSS world. It's only because the constants they have fits within .ldata, so the solution works for them. But if even larger constants comes in again, we will get more failures.
There was a problem hiding this comment.
I agree that diverging the behavior is not a good idea. How about this: instead of adding a config here, Inductor can detect if the model does buffer mutation (https://github.com/pytorch/pytorch/blob/655038687afd19a4a4c9371b77ff046fd6c84be1/torch/_inductor/lowering.py#L5078C25-L5078C41), then decides what section to use and add const_size checking to explicitly fail when size is over limit. We will work on a more comprehensive support in future, but this at least will notice user with a clear error msg.
Summary: Add an additional config to allow buffer mutation. For data that's greater than 2GB, we would need to set it as read-only, otherwise overflow would occur. This is a temporary solution since it won't handle cases that requires mutable data greater than 2GB. Test Plan: Included in commit. Differential Revision: D57514729 [ghstack-poisoned]
Summary: Add an additional config to allow buffer mutation. For data that's greater than 2GB, we would need to set it as read-only, otherwise overflow would occur. This is a temporary solution since it won't handle cases that requires mutable data greater than 2GB. Test Plan: Included in commit. Differential Revision: D57514729 ghstack-source-id: d311ef3d306a0d1fb912f5c67bcbfdd0ea5aef4b Pull Request resolved: #126667
|
@pytorchbot merge -f 'Landed internally' (Initiating merge automatically since Phabricator Diff has merged, using force because this PR might not pass merge_rules.json but landed internally) |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Summary:
Add an additional config to allow buffer mutation.
For data that's greater than 2GB, we would need to set it as read-only, otherwise overflow would occur.
This is a temporary solution since it won't handle cases that requires mutable data greater than 2GB.
Test Plan: Included in commit.
Differential Revision: D57514729
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @yf225 @chenyang78 @kadeng @ColinPeppler @amjames @desertfire @chauhang