deepspeedai / deepspeedai/DeepSpeed

[BUG] need some asserts instead of warnings or silence

Open
#2,658 2 comments 0 reactions 1 assignee View on GitHub

@tjruwase is already working on this.

Since Jan 6, 2023.

bug training
Dominant language
Python
Stars
43.1k
Forks
5k
Avg merge
4d 15h
Merged PRs (30d)
112

Description

Describe the bug

Deepspeed has several places where things silently get dropped on the floor, and I propose those should be asserted. Here are some examples that I have run into in the last week:

  1. the skipping of backward in https://github.com/microsoft/DeepSpeed/pull/2642 - we fixed it for nn.Parameter but the warning is still there for some other cases. Users have no idea what it means and will get inferior training results. As things sort of continue working, nobody thinks twice about trying to understand what that warning even means. I think it should be an assert and should include a tell how a user can fix a problem. Skipping backward autograd function is not ok. See my commentary here: https://github.com/microsoft/DeepSpeed/pull/2642#issuecomment-1364100008

Here is the warning:
https://github.com/microsoft/DeepSpeed/blob/5188f0b501f3ba6d8ba93294ac21a9954fe3d565/deepspeed/runtime/zero/parameter_offload.py#L61-L64

  1. deepspeed.zero.GatheredParameters will silently ignore inputs if it's not a list / tuple / tensor, so if you pass param=model.parameters() - gathering will silently do nothing - because it's a generator which will silently get skipepd. It should assert if it can't handle the params, so that the user could act on it and fix their code. here one has to fix with list(model.parameters()) for it to work.

Incidentally this should take care of generators so that the example above can work as is, w/o needing list:

diff --git a/deepspeed/runtime/zero/partition_parameters.py b/deepspeed/runtime/zero/partition_parameters.py
index 0a7d1475..9303638b 100755
--- a/deepspeed/runtime/zero/partition_parameters.py
+++ b/deepspeed/runtime/zero/partition_parameters.py
@@ -1598,7 +1598,12 @@ class GatheredParameters:
         if not enabled:
             return

-        if not (isinstance(params, list) or isinstance(params, tuple)):
+        if isinstance(params, Iterable) and not isinstance(params, torch.Tensor):
+            # deal with generators like model.parameters()
+            # must convert to list to be able to iterate more than once if we get a generator
+            params = list(params)
+        else:
+            # single param
             params = [params]

         # enable if at least one is zero-param, otherwise a noop

but this is still not enough and there will be params that will silently get skipped. So probably need to check more tightly so that it's either a tensor or an iterable of tensors, or assert if it's neither. (I made the above into https://github.com/microsoft/DeepSpeed/pull/2664, but it's not a complete solution).

the problem with GatheredParameters is that it's often used to init weights and if you give those init functions a 0-sized partitioned tensor they will silently do nothing and the user is going to have a terrible training being unaware that their GatheredParameters call did absolutely nothing and thus the init weights functions silently failed. I have just run into this problem myself.

p.s. no idea how this bug was tagged with compression but this is not the right category.

@tjruwase, @jeffra

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.