[fix][OSS] adding an assert for empty shards + corresponding unit test #406
Conversation
facebook-github-bot
added
the
CLA Signed
blefaudeux
requested review from
msbaines,
min-xu-ai,
anj-s and
Vittorio-Caggiano
and removed request for
min-xu-ai
blefaudeux
force-pushed
the
oss_assert_empty_shards
branch
from
d24c484 to
9d3ce35
Compare
| @@ -8,12 +8,12 @@ | |||
|
|
|||
| adascale_test_data = [ | |||
| # "input" value is a list of input tensors for micro-batch/rank 0 and micro-batch/rank 1. | |||
| {"input": [[1.0, 0], [0, 1.0]], "expected_gain": 2.0}, | |||
| {"input": [[1.0, 0], [0, 1.0]], "expected_gain": 4.0 / 3}, | |||
There was a problem hiding this comment.
@min-xu-ai @mikerabbat checking with you that this is ok. Since adascale is not changed by this PR, I assumed that the current state was correct
There was a problem hiding this comment.
why is 4.0/3 is the new value? maybe if you init the bias to 0 then the value here won't change? the original value of 2 is because we have two grads from two ranks that are completely independent. it must be the grad from the bias are point to the same direction now, hence 4 grads but 3 directions. So it is fine.
There was a problem hiding this comment.
I tried setting the bias to zero, but the returned expected gain is still 1.3333
There was a problem hiding this comment.
That's fine. I think the new value make sense. Keep it 4.0/3 would be good.
| @@ -37,7 +37,7 @@ def _test_basic_func(rank, world_size, tempfile_name, test_case, oss, model=None | |||
| _dist_init(rank, world_size, tempfile_name, backend="nccl") | |||
|
|
|||
| if model is None: | |||
| model = Linear(2, 2, bias=False) | |||
| model = Linear(2, 2) | |||
There was a problem hiding this comment.
you need the bias or otherwise there isn't enough params?
| def run_test_catch_empty_shardd(rank, world_size, tempfile_name): | ||
| dist_init(rank, world_size, tempfile_name, backend="gloo") | ||
| m = torch.nn.Linear(1, 1) | ||
| with pytest.raises(AssertionError): |
| @@ -8,12 +8,12 @@ | |||
|
|
|||
| adascale_test_data = [ | |||
| # "input" value is a list of input tensors for micro-batch/rank 0 and micro-batch/rank 1. | |||
| {"input": [[1.0, 0], [0, 1.0]], "expected_gain": 2.0}, | |||
| {"input": [[1.0, 0], [0, 1.0]], "expected_gain": 4.0 / 3}, | |||
There was a problem hiding this comment.
why is 4.0/3 is the new value? maybe if you init the bias to 0 then the value here won't change? the original value of 2 is because we have two grads from two ranks that are completely independent. it must be the grad from the bias are point to the same direction now, hence 4 grads but 3 directions. So it is fine.
|
unfortunately the unit tests are still failing. Perhaps you need to update a few more places since the golden values are used in several places. |
Before submitting
What does this PR do?
Fixes #405 .
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃