deepspeedai / deepspeedai/DeepSpeed
[BUG] need some asserts instead of warnings or silence
@tjruwase is already working on this.
Since Jan 6, 2023.
- 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:
- the skipping of backward in https://github.com/microsoft/DeepSpeed/pull/2642 - we fixed it for
nn.Parameterbut 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
deepspeed.zero.GatheredParameterswill silently ignore inputs if it's not a list / tuple / tensor, so if you passparam=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 withlist(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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.