Drop the documented grad_hooks ZeRO option, which does not exist - #8242
Drop the documented grad_hooks ZeRO option, which does not exist#8242vineethsaivs wants to merge 1 commit into
Conversation
config-json.md documents grad_hooks as a ZeRO option with a default of True, but
DeepSpeedZeroConfig has no such field and DeepSpeedConfigModel sets
extra="forbid", so following the docs is a hard failure:
DeepSpeedZeroConfig(**{"stage": 1, "grad_hooks": False})
pydantic_core._pydantic_core.ValidationError: 1 validation error
grad_hooks
Extra inputs are not permitted
The field is not a recent casualty. It is absent from deepspeed/runtime/zero/
config.py and from the constants module in every release back to v0.3.0, so
there is nothing to restore, and DeepSpeedEngine.zero_grad_hooks() reads
zero_config.grad_hooks, which can only raise AttributeError. Nothing in the
package or the tests calls it.
Remove the doc entry and the dead accessor, and add a test that every ZeRO key
config-json.md documents is one DeepSpeedZeroConfig accepts, so the next one is
caught. That test passes deliberately invalid values and only treats
extra_forbidden as a failure, which keeps aliases such as
stage3_prefetch_bucket_size and deprecated fields such as cpu_offload passing.
Signed-off-by: Vineeth Sai <[email protected]>
ebarkhordar
left a comment
ebarkhordar
left a comment
There was a problem hiding this comment.
The central claim holds, and the history makes it stronger than the body puts it. grad_hooks was not removed at some point, it was never wired up. cfa63f5da ("ZeRO stage 1 refresh", #1042) added the doc entry and zero_grad_hooks() in the same commit, and added the field to neither the ZeRO config nor the constants module:
$ git show cfa63f5da:deepspeed/runtime/zero/config.py | grep -i grad_hook # no output
$ git show cfa63f5da:deepspeed/runtime/zero/constants.py | grep -i grad_hook # no output
$ git show cfa63f5da -- deepspeed/runtime/engine.py docs/_pages/config-json.md | grep grad_hooks
+ def zero_grad_hooks(self):
+ return self._config.zero_config.grad_hooks
+<i>**grad_hooks**</i>: [boolean]
Naming that commit in the body would answer the "did we break this, and should we restore it instead" question up front.
The dead-accessor claim holds too. An ast sweep over every .py in the master tree finds zero_grad_hooks exactly once, its own definition at deepspeed/runtime/engine.py:1290, and .grad_hooks exactly once, on the line inside it. Removing it cannot break a caller, since any call raises AttributeError: 'DeepSpeedZeroConfig' object has no attribute 'grad_hooks' today.
I ran your new guard, because CI has not. On this head sha cpu-torch-latest, Formatting, python, nv-pre-compile-ops and DCO / required are all sitting at action_required, so modal-torch-latest is the only leg that has executed. In a clean python:3.11-slim container with torch 2.13.0+cpu and pydantic 2.13.4:
$ python -m pytest tests/unit/runtime/zero/test_zero_config.py -q
7 passed
It also bites, which is the part worth knowing: re-adding the grad_hooks doc block and rerunning gives AssertionError: documented but not accepted by DeepSpeedZeroConfig: ['grad_hooks']. Your parser finds 22 keys against its floor of 15, and the six stage3_* entries among them are accepted through their pydantic aliases rather than as declared fields, so filtering on extra_forbidden is the right call there.
For scope, not a request: the drift runs both ways. 28 fields DeepSpeedZeroConfig declares have no entry in that doc section, zenflow, leaf_module, sub_group_size and mics_shard_size among them. This PR guards the direction that actually breaks users, which seems like the right half to take on here.
2 participants
Problem
config-json.mddocumentsgrad_hooksas a ZeRO option with a default ofTrue:DeepSpeedZeroConfighas no such field, andDeepSpeedConfigModelsetsextra="forbid", so anyone who follows the docs gets a hard failure out ofdeepspeed.initialize:It is not a recent casualty.
grad_hooksis absent fromdeepspeed/runtime/zero/config.pyand from the ZeRO constants module in every release back to v0.3.0, so there is nothing to restore.DeepSpeedEngine.zero_grad_hooks()still readsself._config.zero_config.grad_hooks, which can only raiseAttributeError, and nothing in the package or the tests calls it.Fix
Remove the doc entry and the dead accessor. If the intent is that the option should exist, that is a feature rather than a fix and I would rather leave it to you than invent a semantic for it.
Test
tests/unit/runtime/zero/test_zero_config.pygains a check that every ZeRO keyconfig-json.mddocuments is oneDeepSpeedZeroConfigaccepts, so the next drift is caught rather than found by a user.It passes a deliberately invalid value for each key and treats only
extra_forbiddenas a failure, so aliases such asstage3_prefetch_bucket_sizeand deprecated fields such ascpu_offloadstill pass. It also asserts it found more than fifteen keys and thatstageis among them, so a change to the doc format makes the test fail loudly rather than silently check nothing.python -m pytest tests/unit/runtime/zero/test_zero_config.pygives 1 failed / 6 passed againstmasterand 7 passed with this change, on CPU.yapf --style .style.yapfandflake8 --config .flake8are clean on both changed Python files, and clean on the unmodified tree as a control.How it was found
Cross-checking every
<i>**key**</i>inconfig-json.mdagainst agrepfor that literal indeepspeed/. Of 126 documented keys only two came back unreferenced:Compression, which is a section name, and this one.