Skip to content

[FSDP] Add unsafe setattr gated by env var#96326

Closed
awgu wants to merge 7 commits intogh/awgu/344/basefrom
gh/awgu/344/head
Closed

[FSDP] Add unsafe setattr gated by env var#96326
awgu wants to merge 7 commits intogh/awgu/344/basefrom
gh/awgu/344/head

Conversation

@awgu
Copy link
Collaborator

@awgu awgu commented Mar 8, 2023

Stack from ghstack (oldest at bottom):

This adds the option to use an unsafe setattr for _use_sharded_views() and _use_unsharded_views() gated by the environment variable FSDP_USE_UNSAFE_SETATTR, where a value of 1 means to use the unsafe setattr. The unsafe option is disabled by default.

The unsafe setattr may be able to save CPU overhead and may be used to intentionally bypass setattr checks. Both _use_sharded_views() and _use_unsharded_views() must use the unsafe version or use the safe versions atomically.

@pytorch-bot
Copy link

pytorch-bot bot commented Mar 8, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit a68a119:
💚 Looks good so far! There are no failures yet. 💚

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

@pytorch-bot pytorch-bot bot added the release notes: distributed (fsdp) release notes category label Mar 8, 2023
@awgu awgu added the topic: not user facing topic category label Mar 8, 2023
This adds the option to use an unsafe `setattr` for `_use_sharded_views()` and `_use_unsharded_views()` gated by the environment variable `FSDP_USE_UNSAFE_SETATTR`, where a value of `1` means to use the unsafe `setattr`. The unsafe option is disabled by default.

The unsafe `setattr` may be able to save CPU overhead and may be used to intentionally bypass `setattr` checks. Both `_use_sharded_views()` and `_use_unsharded_views()` must use the unsafe version or use the safe versions atomically.

[ghstack-poisoned]
This adds the option to use an unsafe `setattr` for `_use_sharded_views()` and `_use_unsharded_views()` gated by the environment variable `FSDP_USE_UNSAFE_SETATTR`, where a value of `1` means to use the unsafe `setattr`. The unsafe option is disabled by default.

The unsafe `setattr` may be able to save CPU overhead and may be used to intentionally bypass `setattr` checks. Both `_use_sharded_views()` and `_use_unsharded_views()` must use the unsafe version or use the safe versions atomically.

[ghstack-poisoned]
This adds the option to use an unsafe `setattr` for `_use_sharded_views()` and `_use_unsharded_views()` gated by the environment variable `FSDP_USE_UNSAFE_SETATTR`, where a value of `1` means to use the unsafe `setattr`. The unsafe option is disabled by default.

The unsafe `setattr` may be able to save CPU overhead and may be used to intentionally bypass `setattr` checks. Both `_use_sharded_views()` and `_use_unsharded_views()` must use the unsafe version or use the safe versions atomically.

[ghstack-poisoned]
awgu pushed a commit that referenced this pull request Mar 8, 2023
ghstack-source-id: a854def
Pull Request resolved: #96326
@awgu awgu marked this pull request as ready for review March 8, 2023 20:49
Copy link
Contributor

@fegin fegin left a comment

Choose a reason for hiding this comment

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

LGTM, just a comment.

or a submodule chosen by the provided wrapping policy.
"""

# Environment variable to use unsafe `setattr()` for view setting
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have some description why we need to switch between UNSAFE and SAFER version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! Added an explanation.

This adds the option to use an unsafe `setattr` for `_use_sharded_views()` and `_use_unsharded_views()` gated by the environment variable `FSDP_USE_UNSAFE_SETATTR`, where a value of `1` means to use the unsafe `setattr`. The unsafe option is disabled by default.

The unsafe `setattr` may be able to save CPU overhead and may be used to intentionally bypass `setattr` checks. Both `_use_sharded_views()` and `_use_unsharded_views()` must use the unsafe version or use the safe versions atomically.

[ghstack-poisoned]
This adds the option to use an unsafe `setattr` for `_use_sharded_views()` and `_use_unsharded_views()` gated by the environment variable `FSDP_USE_UNSAFE_SETATTR`, where a value of `1` means to use the unsafe `setattr`. The unsafe option is disabled by default.

The unsafe `setattr` may be able to save CPU overhead and may be used to intentionally bypass `setattr` checks. Both `_use_sharded_views()` and `_use_unsharded_views()` must use the unsafe version or use the safe versions atomically.

[ghstack-poisoned]
This adds the option to use an unsafe `setattr` for `_use_sharded_views()` and `_use_unsharded_views()` gated by the environment variable `FSDP_USE_UNSAFE_SETATTR`, where a value of `1` means to use the unsafe `setattr`. The unsafe option is disabled by default.

The unsafe `setattr` may be able to save CPU overhead and may be used to intentionally bypass `setattr` checks. Both `_use_sharded_views()` and `_use_unsharded_views()` must use the unsafe version or use the safe versions atomically.

[ghstack-poisoned]
awgu pushed a commit that referenced this pull request Mar 8, 2023
ghstack-source-id: 0b5d8d2
Pull Request resolved: #96326
@awgu awgu added the ciflow/trunk Trigger trunk jobs on your pull request label Mar 8, 2023
@awgu
Copy link
Collaborator Author

awgu commented Mar 8, 2023

@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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: Not merging any PRs at the moment because there is a merge blocking https://github.com/pytorch/pytorch/labels/ci:%20sev issue open at:
#96371

Details for Dev Infra team Raised by workflow job

@awgu
Copy link
Collaborator Author

awgu commented Mar 9, 2023

@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

cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 12, 2023
This adds the option to use an unsafe `setattr` for `_use_sharded_views()` and `_use_unsharded_views()` gated by the environment variable `FSDP_USE_UNSAFE_SETATTR`, where a value of `1` means to use the unsafe `setattr`. The unsafe option is disabled by default.

The unsafe `setattr` may be able to save CPU overhead and may be used to intentionally bypass `setattr` checks. Both `_use_sharded_views()` and `_use_unsharded_views()` must use the unsafe version or use the safe versions atomically.
Pull Request resolved: pytorch/pytorch#96326
Approved by: https://github.com/zhaojuanmao, https://github.com/fegin
ydwu4 added a commit to ydwu4/pytorch that referenced this pull request Mar 13, 2023
This adds the option to use an unsafe `setattr` for `_use_sharded_views()` and `_use_unsharded_views()` gated by the environment variable `FSDP_USE_UNSAFE_SETATTR`, where a value of `1` means to use the unsafe `setattr`. The unsafe option is disabled by default.

The unsafe `setattr` may be able to save CPU overhead and may be used to intentionally bypass `setattr` checks. Both `_use_sharded_views()` and `_use_unsharded_views()` must use the unsafe version or use the safe versions atomically.
Pull Request resolved: pytorch#96326
Approved by: https://github.com/zhaojuanmao, https://github.com/fegin
@facebook-github-bot facebook-github-bot deleted the gh/awgu/344/head branch June 8, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: distributed (fsdp) release notes category topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants