Shengf/fix all2all #206
Shengf/fix all2all #206
Conversation
shengfukevin
requested review from
briancoutinho,
kingchc,
louisfeng,
shengbao-zheng and
sunghlin
as code owners
facebook-github-bot
added
the
CLA Signed
|
@shengfukevin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Fix support to replay all2all. Test Plan: constructed 4 rank case to invoke torch.distributed.all_to_all() and torch.distributed.all_to_all_single(), then dump trace and replay. Differential Revision: D75101007 Pulled By: shengfukevin
facebook-github-bot
force-pushed
the
shengf/fix_all2all
branch
from
d284c4c to
457f45c
Compare
|
@shengfukevin has updated the pull request. You must reimport the pull request before landing. |
|
This pull request was exported from Phabricator. Differential Revision: D75101007 |
shengfukevin
left a comment
shengfukevin
left a comment
There was a problem hiding this comment.
LG2M! Have one inline question, please check.
| opTensor = [] | ||
| if allocate: | ||
| alloc_func = ( | ||
| i_alloc_func = ( |
There was a problem hiding this comment.
@sanshang-nv, what is the reason for the change in this code block
There was a problem hiding this comment.
Which point?
i_should be prefix ofinput_.i_scale_factoris the same usage as other prepare function, which is not used before this PR.- the input and output of
all_to_allis a list of tensor.
There was a problem hiding this comment.
What I ask is why do you change to logic to create data for input/output tensors?
The original code is to create tensor with initVal when check_data is true, otherwise fill the tensor with all one.
With your change, you create data differently for input and output tensors. In some cases, you use scaleFactor. What is the reason behind this change?
There was a problem hiding this comment.
Previous logic:
if dcheck is true, use alloc_ones with initVal to create both input and output tensor. Problem is, output tensor should always use random for check.
if dcheck is false, use alloc_random, but still use initVal. It's wrong, should use scale_factor. Otherwise, why pass parameter scaleFactor in.
Fixed logic in PR:
if dcheck is true, use alloc_ones with initVal.
if dcheck is false, use alloc_random with scaleFactor.
output tensor shoudl always be allocated with alloc_random.
take
param/et_replay/comm/comms_utils.py
Line 966 in 656b66c
There was a problem hiding this comment.
Thanks. I think what value to initialize output tensor should not matter. right? Since it will be overwritten.
There was a problem hiding this comment.
I need a stamp from Meta side to get it approved.
|
@shengfukevin merged this pull request in 1fea15e. |
Summary
Fix support to replay all2all.
Test Plan
constructed 4 rank case to invoke torch.distributed.all_to_all() and torch.distributed.all_to_all_single(), then dump trace and replay.