Revert "Simplify handle indexing (#105006)" #105984

awgu · 2023-07-25T23:32:47Z

Stack from ghstack (oldest at bottom):

This reverts commit 429d45f.

Unfortunately, #105006 broke backward prefetching (where backward prefetching working correctly was not captured in our unit tests).

I need more time to dig into this (tomorrow), but I think the issue is related to:
429d45f#diff-9a6937168d232432c34c2c4605b96f3147afa2786e287f74b6074b20aa5980e6R143-R146

Follow-ups:

Investigate this thoroughly
Add unit tests to capture backward prefetch functionality

Differential Revision: D47816273

This reverts commit 429d45f. [ghstack-poisoned]

This reverts commit 429d45f. ghstack-source-id: fdc129260f85a4f14a3a325d97625ef5a6c25f0b Pull Request resolved: #105984

pytorch-bot · 2023-07-25T23:40:05Z

🔗 Helpful Links

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

📄 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:

6-7 hour queue times caused by GPU instances not being available

✅ 1 Unrelated Failure

As of commit 33e2d2c:

UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:

linux-focal-rocm5.6-py3.8 / test (default, 2, 3, linux.rocm.gpu, unstable) (gh)

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

fegin

Stamped to unblock

This reverts commit 429d45f. Unfortunately, #105006 broke backward prefetching (where backward prefetching working correctly was not captured in our unit tests). I need more time to dig into this (tomorrow), but I think the issue is related to: 429d45f#diff-9a6937168d232432c34c2c4605b96f3147afa2786e287f74b6074b20aa5980e6R143-R146 Follow-ups: 1. Investigate this thoroughly 2. Add unit tests to capture backward prefetch functionality [ghstack-poisoned]

This reverts commit 429d45f. ghstack-source-id: 14ee53b0ac632ccc56ebb5b544a204dba2d9f44c Pull Request resolved: pytorch#105984

awgu · 2023-07-26T12:09:31Z

Failure is unrelated:
linux-focal-rocm5.6-py3.8 / test (default, 2, 3, linux.rocm.gpu, unstable

awgu · 2023-07-26T12:09:39Z

@pytorchbot merge -f "unrelated failure"

pytorchmergebot · 2023-07-26T12:12:09Z

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

awgu · 2023-07-26T22:40:02Z

@awgu has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

awgu · 2023-07-26T22:41:18Z

@awgu has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

This reverts commit a9a3c45. fixes [ghstack-poisoned]

This reverts commit a9a3c45. fixes ghstack-source-id: a025b0c5d5aeb7606266767ffaa3b6ef1e7f96a9 Pull Request resolved: #106357

This reverts commit a9a3c45. fixes ghstack-source-id: 8350887b112673b94b6a13f61106ad2650a7376a Pull Request resolved: #106357 fix

This reverts commit a9a3c45. fixes ghstack-source-id: 0342a7c447a14d8a46f177962d2cb1ded41d0682 Pull Request resolved: #106357 fix fix

issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 [ghstack-poisoned]

issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 ghstack-source-id: ec6826e16df19b11b7ff3c48c28c17d1a045c211 Pull Request resolved: #107058

…test" issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 [ghstack-poisoned]

issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 ghstack-source-id: f6119e94d29a8feaf6248cbe3c62a0a1df0e2387 Pull Request resolved: #107058

…test" issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 [ghstack-poisoned]

issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 ghstack-source-id: 2ee4bc10963a4bff0f39625dadee716ddaab2df0 Pull Request resolved: #107058

…test" issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 [ghstack-poisoned]

issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 ghstack-source-id: aae553db4518bb223c79a8c86c3a0767cb8e51b3 Pull Request resolved: #107058

…test" issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 [ghstack-poisoned]

issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 ghstack-source-id: 69aa41a50b24c21498c4ade9dfe47baa0b63b834 Pull Request resolved: #107058

…test" issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect params from prefetch handle to follow order DECODER_PARAM_FQNS, ENCODER_PARAM_FQNS * backward_prefetch = None: expect None prefetch handle [ghstack-poisoned]

issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 ghstack-source-id: f0c358c27f89f004bba1953125ee42d7d5dad29d Pull Request resolved: #107058

issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 ghstack-source-id: 9926fd24f22b14e32261979b1e4a7f7cbd56e914 Pull Request resolved: #107058

…ectly with unit test" issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect params from prefetch handle to follow order DECODER_PARAM_FQNS, ENCODER_PARAM_FQNS * backward_prefetch = None: expect None prefetch handle [ghstack-poisoned]

…test" issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect params from prefetch handle to follow order DECODER_PARAM_FQNS, ENCODER_PARAM_FQNS * backward_prefetch = None: expect None prefetch handle [ghstack-poisoned]

…ectly with unit test" issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and check which handles are prefetched for backward_prefetch = BackwardPrefetch.BACKWARD_PRE * state._exec_order_data.handles_post_forward_order equals forward order: encoder 0...5 -> decoder 0...5 -> root * pre-backward hook order: root -> decoder 5...0 -> encoder 5...0 * prefetch order: decoder 5...0 -> encoder 5...0 -> None * when current_handle=encoder 0, _get_handle_to_prefetch returns None for backward_prefetch = BackwardPrefetch.BACKWARD_POST * state._exec_order_data.handles_post_forward_order equals forward order: encoder 0...5 -> decoder 0...5 -> root * post-backward hook (AccumulateGrad) order: decoder 5, 4...0 -> encoder 5...0 -> root * prefetch order: decoder 4...0 -> encoder 5...0 -> None -> None * 1st None: when current_handle=encoder 0, _get_handle_to_prefetch returns None * 2nd None: when current_handle=root, we get decoder 5 inside _get_handle_to_prefetch but is not needed. so returns None [ghstack-poisoned]

…test" issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and check which handles are prefetched for backward_prefetch = BackwardPrefetch.BACKWARD_PRE * state._exec_order_data.handles_post_forward_order equals forward order: encoder 0...5 -> decoder 0...5 -> root * pre-backward hook order: root -> decoder 5...0 -> encoder 5...0 * prefetch order: decoder 5...0 -> encoder 5...0 -> None * when current_handle=encoder 0, _get_handle_to_prefetch returns None for backward_prefetch = BackwardPrefetch.BACKWARD_POST * state._exec_order_data.handles_post_forward_order equals forward order: encoder 0...5 -> decoder 0...5 -> root * post-backward hook (AccumulateGrad) order: decoder 5, 4...0 -> encoder 5...0 -> root * prefetch order: decoder 4...0 -> encoder 5...0 -> None -> None * 1st None: when current_handle=encoder 0, _get_handle_to_prefetch returns None * 2nd None: when current_handle=root, we get decoder 5 inside _get_handle_to_prefetch but is not needed. so returns None [ghstack-poisoned]

issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 ghstack-source-id: 527d658362d51520fa30177521237cfd796f5ad0 Pull Request resolved: #107058

issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and get total function call and total non-None handle * BackwardPrefetch.BACKWARD_POST and BackwardPrefetch.BACKWARD_PRE: expect total non-None handle > 0 * backward_prefetch = None: expect total function call = 0 ghstack-source-id: 34b907180ac5cd649aba940fa8bbdee2e5b5e57e Pull Request resolved: #107058

…ectly with unit test" issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and check which handles are prefetched for backward_prefetch = BackwardPrefetch.BACKWARD_PRE * state._exec_order_data.handles_post_forward_order equals forward order: encoder 0...5 -> decoder 0...5 -> root * pre-backward hook order: root -> decoder 5...0 -> encoder 5...0 * prefetch order: decoder 5...0 -> encoder 5...0 -> None * when current_handle=encoder 0, _get_handle_to_prefetch returns None for backward_prefetch = BackwardPrefetch.BACKWARD_POST * state._exec_order_data.handles_post_forward_order equals forward order: encoder 0...5 -> decoder 0...5 -> root * post-backward hook (AccumulateGrad) order: decoder 5, 4...0 -> encoder 5...0 -> root * prefetch order: decoder 4...0 -> encoder 5...0 -> None -> None * 1st None: when current_handle=encoder 0, _get_handle_to_prefetch returns None * 2nd None: when current_handle=root, we get decoder 5 inside _get_handle_to_prefetch but is not needed. so returns None [ghstack-poisoned]

…test" issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and check which handles are prefetched for backward_prefetch = BackwardPrefetch.BACKWARD_PRE * state._exec_order_data.handles_post_forward_order equals forward order: encoder 0...5 -> decoder 0...5 -> root * pre-backward hook order: root -> decoder 5...0 -> encoder 5...0 * prefetch order: decoder 5...0 -> encoder 5...0 -> None * when current_handle=encoder 0, _get_handle_to_prefetch returns None for backward_prefetch = BackwardPrefetch.BACKWARD_POST * state._exec_order_data.handles_post_forward_order equals forward order: encoder 0...5 -> decoder 0...5 -> root * post-backward hook (AccumulateGrad) order: decoder 5, 4...0 -> encoder 5...0 -> root * prefetch order: decoder 4...0 -> encoder 5...0 -> None -> None * 1st None: when current_handle=encoder 0, _get_handle_to_prefetch returns None * 2nd None: when current_handle=root, we get decoder 5 inside _get_handle_to_prefetch but is not needed. so returns None [ghstack-poisoned]

issue resolved: #105984 context: * CI did not catch the commit that breaks backward_prefetch #105006 * we had an action item to add unit test to prevent similar cases: #105984 what's included in this unit test * monkey patch torch.distributed.fsdp._runtime_utils._get_handle_to_prefetch and check which handles are prefetched for backward_prefetch = BackwardPrefetch.BACKWARD_PRE * state._exec_order_data.handles_post_forward_order equals forward order: encoder 0...5 -> decoder 0...5 -> root * pre-backward hook order: root -> decoder 5...0 -> encoder 5...0 * prefetch order: decoder 5...0 -> encoder 5...0 -> None * when current_handle=encoder 0, _get_handle_to_prefetch returns None for backward_prefetch = BackwardPrefetch.BACKWARD_POST * state._exec_order_data.handles_post_forward_order equals forward order: encoder 0...5 -> decoder 0...5 -> root * post-backward hook (AccumulateGrad) order: decoder 5, 4...0 -> encoder 5...0 -> root * prefetch order: decoder 4...0 -> encoder 5...0 -> None -> None * 1st None: when current_handle=encoder 0, _get_handle_to_prefetch returns None * 2nd None: when current_handle=root, we get decoder 5 inside _get_handle_to_prefetch but is not needed. so returns None Pull Request resolved: #107058 Approved by: https://github.com/awgu

Revert "Simplify handle indexing (#105006)"

66957b5

This reverts commit 429d45f. [ghstack-poisoned]

awgu requested review from mrshenli, zhaojuanmao, rohan-varma, H-Huang, kwen2501, wanchaol, fegin, fduwjj, kiukchung and d4l3k as code owners July 25, 2023 23:32

awgu added a commit that referenced this pull request Jul 25, 2023

Revert "Simplify handle indexing (#105006)"

55bd5ac

This reverts commit 429d45f. ghstack-source-id: fdc129260f85a4f14a3a325d97625ef5a6c25f0b Pull Request resolved: #105984

awgu requested a review from voznesenskym July 25, 2023 23:33

awgu mentioned this pull request Jul 25, 2023

[FSDP] Add record_function for explicit prefetching #105985

Closed

pytorch-bot bot added release notes: distributed (fsdp) release notes category labels Jul 25, 2023

awgu added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 25, 2023

fegin approved these changes Jul 26, 2023

View reviewed changes

awgu added the topic: not user facing topic category label Jul 26, 2023

awgu added a commit to awgu/pytorch that referenced this pull request Jul 26, 2023

Revert "Simplify handle indexing (pytorch#105006)"

f3b8cce

This reverts commit 429d45f. ghstack-source-id: 14ee53b0ac632ccc56ebb5b544a204dba2d9f44c Pull Request resolved: pytorch#105984

pytorchmergebot added merging Merged and removed merging labels Jul 26, 2023

pytorchmergebot closed this in a9a3c45 Jul 26, 2023

facebook-github-bot deleted the gh/awgu/429/head branch July 29, 2023 14:16

voznesenskym added a commit that referenced this pull request Aug 1, 2023

Revert "Revert "Simplify handle indexing (#105006)" (#105984)"

0ee4822

This reverts commit a9a3c45. fixes [ghstack-poisoned]

voznesenskym added a commit that referenced this pull request Aug 1, 2023

Revert "Revert "Simplify handle indexing (#105006)" (#105984)"

92f0a0a

This reverts commit a9a3c45. fixes ghstack-source-id: a025b0c5d5aeb7606266767ffaa3b6ef1e7f96a9 Pull Request resolved: #106357

voznesenskym added a commit that referenced this pull request Aug 3, 2023

Revert "Revert "Simplify handle indexing (#105006)" (#105984)"

087d6bd

This reverts commit a9a3c45. fixes ghstack-source-id: 8350887b112673b94b6a13f61106ad2650a7376a Pull Request resolved: #106357 fix

voznesenskym added a commit that referenced this pull request Aug 3, 2023

Revert "Revert "Simplify handle indexing (#105006)" (#105984)"

6ae189e

This reverts commit a9a3c45. fixes ghstack-source-id: 0342a7c447a14d8a46f177962d2cb1ded41d0682 Pull Request resolved: #106357 fix fix

weifengpy mentioned this pull request Aug 11, 2023

[FSDP] verify backward_prefetch works correctly with unit test #107058

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "Simplify handle indexing (#105006)" #105984

Revert "Simplify handle indexing (#105006)" #105984

awgu commented Jul 25, 2023 •

edited

pytorch-bot bot commented Jul 25, 2023 •

edited

fegin left a comment

awgu commented Jul 26, 2023

awgu commented Jul 26, 2023

pytorchmergebot commented Jul 26, 2023

awgu commented Jul 26, 2023

awgu commented Jul 26, 2023

Revert "Simplify handle indexing (#105006)" #105984

Revert "Simplify handle indexing (#105006)" #105984

Conversation

awgu commented Jul 25, 2023 • edited

pytorch-bot bot commented Jul 25, 2023 • edited

🔗 Helpful Links

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

❗ 1 Active SEVs

✅ 1 Unrelated Failure

fegin left a comment

Choose a reason for hiding this comment

awgu commented Jul 26, 2023

awgu commented Jul 26, 2023

pytorchmergebot commented Jul 26, 2023

Merge started

awgu commented Jul 26, 2023

awgu commented Jul 26, 2023

awgu commented Jul 25, 2023 •

edited

pytorch-bot bot commented Jul 25, 2023 •

edited