AUC metric optionally move concat to compute step instead of update through flag #1656
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D53027097 |
zainhuda
pushed a commit
to zainhuda/torchrec
that referenced
this pull request
Jan 24, 2024
…hrough flag (pytorch#1656) Summary: Previously unlanded D51399384 due to issues with some models. Introducing it again but through an experimental flag as this optimization can benefit many models. This should not conflict with any models running AUC metric. The feature flag is akin to fusing task computation in RecMetrics as that feature as well does not work across all families of models. The change in this diff is: We don't need to concatenate the tensor on every update step, since it is an expensive operation (creates a new tensor and allocates new memory every call, as tensors are contiguous) we can call tensor.concat on the compute step instead. Which happens every compute_interval_step batches. This optimization should boost performance of models using AUC with no regression in metric quality. Differential Revision: D53027097
zainhuda
force-pushed
the
export-D53027097
branch
from
b472efe to
1441796
Compare
|
This pull request was exported from Phabricator. Differential Revision: D53027097 |
|
Looking forward to the performance boost! |
Andrew12Liu
approved these changes
Jan 27, 2024
zainhuda
pushed a commit
to zainhuda/torchrec
that referenced
this pull request
Feb 1, 2024
… call (pytorch#1656) Summary: Previously unlanded D51399384 because there was a NCCL AllGather hang caused by the previous version of the optimization. This diff fixes the issue by moving the tensor concatenation before the collective is called. By doing this, model outputs are transformed to be identical to what was previous, and thus the flow (sync, collective call, compute) is identical to previous AUC implementation. But at the same time, we avoid concatting every update thus leveraging significant gains. **Why did the NCCL collective hang last time?** There existed an edge case where ranks could have mismatched number of tensors, the way torchmetrics sync and collective call is written it calls an allgather per tensor in a list of tensors. Since number of tensors in a list across ranks is not guaranteed to be the same, the issue can arise where an allgather is being called for a tensor that does not exist. Imagine, 3 tensors on rank 0 and 2 tensors on rank 1. For calling allgather on each tensor, on rank 0 calling on the 3rd tensor will cause a NCCL hang and subsequent timeout since there is no tensor on rank 1 that corresponds to it. A additional GPU unit test is added to cover this scenario, where on rank 0 - 2 tensors are passed in and on rank 1 - 1 tensor is passed in. With the concat before the collective, the test passes. Without the concat, the test will hang. Differential Revision: D53027097
zainhuda
force-pushed
the
export-D53027097
branch
from
1441796 to
43ae3fb
Compare
|
This pull request was exported from Phabricator. Differential Revision: D53027097 |
zainhuda
pushed a commit
to zainhuda/torchrec
that referenced
this pull request
Feb 1, 2024
… call (pytorch#1656) Summary: Previously unlanded D51399384 because there was a NCCL AllGather hang caused by the previous version of the optimization. This diff fixes the issue by moving the tensor concatenation before the collective is called. By doing this, model outputs are transformed to be identical to what was previous, and thus the flow (sync, collective call, compute) is identical to previous AUC implementation. But at the same time, we avoid concatting every update thus leveraging significant gains. **Why did the NCCL collective hang last time?** There existed an edge case where ranks could have mismatched number of tensors, the way torchmetrics sync and collective call is written it calls an allgather per tensor in a list of tensors. Since number of tensors in a list across ranks is not guaranteed to be the same, the issue can arise where an allgather is being called for a tensor that does not exist. Imagine, 3 tensors on rank 0 and 2 tensors on rank 1. For calling allgather on each tensor, on rank 0 calling on the 3rd tensor will cause a NCCL hang and subsequent timeout since there is no tensor on rank 1 that corresponds to it. A additional GPU unit test is added to cover this scenario, where on rank 0 - 2 tensors are passed in and on rank 1 - 1 tensor is passed in. With the concat before the collective, the test passes. Without the concat, the test will hang. Differential Revision: D53027097
zainhuda
force-pushed
the
export-D53027097
branch
from
43ae3fb to
3f5ec14
Compare
|
This pull request was exported from Phabricator. Differential Revision: D53027097 |
zainhuda
pushed a commit
to zainhuda/torchrec
that referenced
this pull request
Feb 1, 2024
… call (pytorch#1656) Summary: Previously unlanded D51399384 because there was a NCCL AllGather hang caused by the previous version of the optimization. This diff fixes the issue by moving the tensor concatenation before the collective is called. By doing this, model outputs are transformed to be identical to what was previous, and thus the flow (sync, collective call, compute) is identical to previous AUC implementation. But at the same time, we avoid concatting every update thus leveraging significant gains. **Why did the NCCL collective hang last time?** There existed an edge case where ranks could have mismatched number of tensors, the way torchmetrics sync and collective call is written it calls an allgather per tensor in a list of tensors. Since number of tensors in a list across ranks is not guaranteed to be the same, the issue can arise where an allgather is being called for a tensor that does not exist. Imagine, 3 tensors on rank 0 and 2 tensors on rank 1. For calling allgather on each tensor, on rank 0 calling on the 3rd tensor will cause a NCCL hang and subsequent timeout since there is no tensor on rank 1 that corresponds to it. A additional GPU unit test is added to cover this scenario, where on rank 0 - 2 tensors are passed in and on rank 1 - 1 tensor is passed in. With the concat before the collective, the test passes. Without the concat, the test will hang. Differential Revision: D53027097
zainhuda
force-pushed
the
export-D53027097
branch
from
3f5ec14 to
7a112fd
Compare
|
This pull request was exported from Phabricator. Differential Revision: D53027097 |
zainhuda
force-pushed
the
export-D53027097
branch
from
7a112fd to
49efc37
Compare
zainhuda
pushed a commit
to zainhuda/torchrec
that referenced
this pull request
Feb 1, 2024
… call (pytorch#1656) Summary: Previously unlanded D51399384 because there was a NCCL AllGather hang caused by the previous version of the optimization. This diff fixes the issue by moving the tensor concatenation before the collective is called. By doing this, model outputs are transformed to be identical to what was previous, and thus the flow (sync, collective call, compute) is identical to previous AUC implementation. But at the same time, we avoid concatting every update thus leveraging significant gains. **Why did the NCCL collective hang last time?** There existed an edge case where ranks could have mismatched number of tensors, the way torchmetrics sync and collective call is written it calls an allgather per tensor in a list of tensors. Since number of tensors in a list across ranks is not guaranteed to be the same, the issue can arise where an allgather is being called for a tensor that does not exist. Imagine, 3 tensors on rank 0 and 2 tensors on rank 1. For calling allgather on each tensor, on rank 0 calling on the 3rd tensor will cause a NCCL hang and subsequent timeout since there is no tensor on rank 1 that corresponds to it. A additional GPU unit test is added to cover this scenario, where on rank 0 - 2 tensors are passed in and on rank 1 - 1 tensor is passed in. With the concat before the collective, the test passes. Without the concat, the test will hang. Differential Revision: D53027097
|
This pull request was exported from Phabricator. Differential Revision: D53027097 |
zainhuda
pushed a commit
to zainhuda/torchrec
that referenced
this pull request
Feb 1, 2024
… call (pytorch#1656) Summary: Previously unlanded D51399384 because there was a NCCL AllGather hang caused by the previous version of the optimization. This diff fixes the issue by moving the tensor concatenation before the collective is called. By doing this, model outputs are transformed to be identical to what was previous, and thus the flow (sync, collective call, compute) is identical to previous AUC implementation. But at the same time, we avoid concatting every update thus leveraging significant gains. **Why did the NCCL collective hang last time?** There existed an edge case where ranks could have mismatched number of tensors, the way torchmetrics sync and collective call is written it calls an allgather per tensor in a list of tensors. Since number of tensors in a list across ranks is not guaranteed to be the same, the issue can arise where an allgather is being called for a tensor that does not exist. Imagine, 3 tensors on rank 0 and 2 tensors on rank 1. For calling allgather on each tensor, on rank 0 calling on the 3rd tensor will cause a NCCL hang and subsequent timeout since there is no tensor on rank 1 that corresponds to it. A additional GPU unit test is added to cover this scenario, where on rank 0 - 2 tensors are passed in and on rank 1 - 1 tensor is passed in. With the concat before the collective, the test passes. Without the concat, the test will hang. Differential Revision: D53027097
zainhuda
force-pushed
the
export-D53027097
branch
from
49efc37 to
81d9430
Compare
|
This pull request was exported from Phabricator. Differential Revision: D53027097 |
zainhuda
force-pushed
the
export-D53027097
branch
from
81d9430 to
850fc93
Compare
zainhuda
pushed a commit
to zainhuda/torchrec
that referenced
this pull request
Feb 23, 2024
… call (pytorch#1656) Summary: Previously unlanded D51399384 because there was a NCCL AllGather hang caused by the previous version of the optimization. This diff fixes the issue by moving the tensor concatenation before the collective is called. By doing this, model outputs are transformed to be identical to what was previous, and thus the flow (sync, collective call, compute) is identical to previous AUC implementation. But at the same time, we avoid concatting every update thus leveraging significant gains. **Why did the NCCL collective hang last time?** There existed an edge case where ranks could have mismatched number of tensors, the way torchmetrics sync and collective call is written it calls an allgather per tensor in a list of tensors. Since number of tensors in a list across ranks is not guaranteed to be the same, the issue can arise where an allgather is being called for a tensor that does not exist. Imagine, 3 tensors on rank 0 and 2 tensors on rank 1. For calling allgather on each tensor, on rank 0 calling on the 3rd tensor will cause a NCCL hang and subsequent timeout since there is no tensor on rank 1 that corresponds to it. In the case where other ranks have more tensors than rank 0, the collective would not hang but those additional tensors would not be collected on to rank 0 resulting in a wrong calculation. The unit test covers for this case as well. A additional GPU unit test is added to cover this scenario, where on rank 0 - 2 tensors are passed in and on rank 1 - 1 tensor is passed in (and vice versa). With the concat before the collective, the test passes. Without the concat, the test will hang. Reviewed By: dstaay-fb Differential Revision: D53027097
|
This pull request was exported from Phabricator. Differential Revision: D53027097 |
… call (pytorch#1656) Summary: Previously unlanded D51399384 because there was a NCCL AllGather hang caused by the previous version of the optimization. This diff fixes the issue by moving the tensor concatenation before the collective is called. By doing this, model outputs are transformed to be identical to what was previous, and thus the flow (sync, collective call, compute) is identical to previous AUC implementation. But at the same time, we avoid concatting every update thus leveraging significant gains. **Why did the NCCL collective hang last time?** There existed an edge case where ranks could have mismatched number of tensors, the way torchmetrics sync and collective call is written it calls an allgather per tensor in a list of tensors. Since number of tensors in a list across ranks is not guaranteed to be the same, the issue can arise where an allgather is being called for a tensor that does not exist. Imagine, 3 tensors on rank 0 and 2 tensors on rank 1. For calling allgather on each tensor, on rank 0 calling on the 3rd tensor will cause a NCCL hang and subsequent timeout since there is no tensor on rank 1 that corresponds to it. In the case where other ranks have more tensors than rank 0, the collective would not hang but those additional tensors would not be collected on to rank 0 resulting in a wrong calculation. The unit test covers for this case as well. A additional GPU unit test is added to cover this scenario, where on rank 0 - 2 tensors are passed in and on rank 1 - 1 tensor is passed in (and vice versa). With the concat before the collective, the test passes. Without the concat, the test will hang. Reviewed By: dstaay-fb Differential Revision: D53027097
zainhuda
force-pushed
the
export-D53027097
branch
from
850fc93 to
8dce235
Compare
|
This pull request was exported from Phabricator. Differential Revision: D53027097 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Previously unlanded D51399384 due to issues with some models. Introducing it again but through an experimental flag as this optimization can benefit many models. This should not conflict with any models running AUC metric. The feature flag is akin to fusing task computation in RecMetrics as that feature as well does not work across all families of models.
The change in this diff is:
We don't need to concatenate the tensor on every update step, since it is an expensive operation (creates a new tensor and allocates new memory every call, as tensors are contiguous) we can call tensor.concat on the compute step instead. Which happens every compute_interval_step batches. This optimization should boost performance of models using AUC with no regression in metric quality.
Differential Revision: D53027097