Use the val argument in mask_nan_or_inf_with_val_inplace - #8386
Use the val argument in mask_nan_or_inf_with_val_inplace#8386vineethsaivs wants to merge 1 commit into
Conversation
The helper takes `val` and then hardcodes -1.0 in the tensor it fills with, so any caller asking for a different sentinel silently gets -1 instead. `val` was added with the helper in deepspeedai#7184, which folded three copies of the inf/nan masking into one function; the parameter is the knob that refactor introduced and it was never wired to the body. Every in-tree caller uses the default, so the fix is a no-op for them: -1. is the same float the body hardcoded. Signed-off-by: Vineeth Sai <vineethsai4444@gmail.com>
ebarkhordar
left a comment
There was a problem hiding this comment.
get_grad_norm_direct in zero/stage3.py never got folded into the helper, so the three copies in the description are missing one. It still masks inline, with the same hardcoded literal:
# deepspeed/runtime/zero/stage3.py:2311-2316
norm_is_inf = total_norm.isinf()
norm_is_nan = total_norm.isnan()
inf_or_nan = norm_is_nan.logical_or(norm_is_inf)
err = torch.tensor(-1.0, device=self.device, dtype=torch.float)
total_norm = torch.where(inf_or_nan, err, total_norm)So that path cannot honor val either. Swapping those six lines for mask_nan_or_inf_with_val_inplace(total_norm, device=self.device) returns the same value at the same dtype: total_norm comes fresh off total_norm_cuda ** (1. / norm_type), so the in-place write has no other reader, and I ran torch.where against masked_fill_ for both get_norm_dtype() cases, fp64 and fp32, and got -1.0 at the input dtype from each.
Your three call sites are right. I enumerated them with ast rather than grep and got exactly runtime/utils.py:963, zero/stage3.py:1885 and zero/stage_1_and_2.py:2114, all taking the default.
Not blocking, and it may well be cleaner as its own PR.
|
You are right, that is a fourth site and it cannot honour The "no other reader" argument needs the other branch too. # norm_type == inf
total_norm = total_norm_cuda[0] # a view into total_norm_cuda
# else
total_norm = total_norm_cuda**(1. / norm_type) # freshSo Dtypes agree on both paths. The inf branch comes off Taking your suggestion: this PR stays the one-line |
|
Separate, I'd say. This one is a one line change with a self-contained argument. The stage3 swap turns an allocating You are right that my justification only covered the else branch. |
|
Agreed on both counts. Keeping this one at the single line, and I will open the stage3 |
What
mask_nan_or_inf_with_val_inplace(input, device=None, val=-1.)acceptsvaland then builds the fill tensor from a hardcoded-1.0, so the argument is dead:Why it is there
valarrived with the helper in #7184, which folded three copies of the inf/nan masking (runtime/utils.py,zero/stage3.py,zero/stage_1_and_2.py) into one function. The parameter is the knob that refactor introduced, and the body kept the literal from the code it replaced.Fix
One line: build
errfromval.All three in-tree callers use the default, and
-1.is the same float the body hardcoded, so the gradient-norm paths are bit-identical.Test
New
test_mask_nan_or_inf_with_val_inplace_honors_valintests/unit/runtime/test_runtime_utils.py, covering nan / +inf / -inf for severalvals plus the unchanged default.The
TestClipGradNorm*/TestCheckOverflowcases in the same file areDistributedTestand were deselected on this CPU-only box; they do not touchval.