Skip to content

Commit 6daac90

Browse files
Kymi808tohtanasfc-gh-truwase
authored andcommitted
activation_checkpointing: default num_layers to None so configure() assert fires (#8041)
## Summary Calling `deepspeed.checkpointing.configure(contiguous_checkpointing=True, partition_activations=True)` *without* setting `num_checkpoints` (or otherwise providing a layer count) is supposed to fail fast with the assert at `deepspeed/runtime/activation_checkpointing/checkpointing.py:1108`: ```python if CONTIGUOUS_CHECKPOINTING: assert num_layers is not None, "Must specify the number of layers with contiguous memory checkpointing" ``` That assert never fires because `_configure_defaults()` (called inside `configure()` at line 1079) initialized: ```python num_layers = False ``` `False is not None` is `True`, so the assert silently passes. The user instead hits a much later cryptic `IndexError` from `range(num_layers)` (lines 399 / 406) or a 0-element allocation from `numel * num_layers` (lines 457 / 461). The module-level default at line 46 is already `num_layers = None`, and every other path that sets it (`set_num_layers`, `config.number_checkpoints`) assigns an integer — only `_configure_defaults` used `False`, which looks like a copy-paste from the surrounding `PARTITION_ACTIVATIONS = False` etc. block. ## Fix ```diff - num_layers = False + num_layers = None ``` One-character change. No callers compare `num_layers` to `False` (downstream uses are `range(num_layers)` and `numel * num_layers`, both requiring an int), so the only path this changes is the broken one: users now get the documented "Must specify the number of layers" assert instead of a downstream `IndexError`. ## Test Adds `test_configure_with_contiguous_checkpointing_requires_num_checkpoints` to the existing `tests/unit/runtime/activation_checkpointing/test_activation_checkpointing.py` (consolidating per `AGENTS.md`, not a new file). It calls `configure(contiguous_checkpointing=True, partition_activations=True)` and asserts the expected `AssertionError` matches `"number of layers"`. On `main` the assert silently passes and the test fails; with this change it passes. ## CI/lint - `pre-commit run --files <changed files>`: all hooks pass (yapf, flake8, `check-torchdist`, `check-license`, codespell, `check-torchcuda`). - DCO `Signed-off-by` on the commit. - No competing PR (`gh pr list --search "num_layers checkpointing OR _configure_defaults OR contiguous_checkpointing assert"`). --------- Signed-off-by: Kymi808 <zeng.kyle13@gmail.com> Co-authored-by: Masahiro Tanaka <81312776+tohtana@users.noreply.github.com> Co-authored-by: Olatunji Ruwase <tunji.ruwase@snowflake.com> Signed-off-by: Guokai Ma <guokai.ma@intel.com>
1 parent 4421665 commit 6daac90

4 files changed

Lines changed: 173 additions & 3 deletions

File tree

deepspeed/module_inject/auto_tp.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -571,7 +571,7 @@ def _replace_module(self, r_module, prev_name='', prev_class_name=''):
571571
# When using partition_config (custom patterns/presets), use pattern-based routing
572572
# instead of linear_policies. This keeps all pattern logic centralized here.
573573
if self.partition_config is not None:
574-
full_name = prev_name + '.' + name if prev_name else name
574+
full_name = class_name + '.' + name if class_name else name
575575
if isinstance(child, nn.Embedding):
576576
# Check if embedding matches any pattern
577577
param_name = full_name + ".weight"
@@ -588,7 +588,7 @@ def _replace_module(self, r_module, prev_name='', prev_class_name=''):
588588
setattr(r_module, name, new_child)
589589
else:
590590
self.update_mp_params(child)
591-
self._replace_module(child, full_name, class_name)
591+
self._replace_module(child, name, class_name)
592592
# Traditional path: use linear_policies for type-based routing
593593
elif child.__class__ in self.linear_policies:
594594
setattr(r_module, name, self.linear_policies[child.__class__](child, prev_name + '.' + name,

deepspeed/runtime/activation_checkpointing/checkpointing.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1019,7 +1019,7 @@ def _configure_defaults():
10191019

10201020
PARTITION_ACTIVATIONS = False
10211021
CONTIGUOUS_CHECKPOINTING = False
1022-
num_layers = False
1022+
num_layers = None
10231023
CPU_CHECKPOINT = False
10241024
SYNCHRONIZE = False
10251025
PROFILE_TIME = False
Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
# SPDX-License-Identifier: Apache-2.0
2+
# DeepSpeed Team
3+
"""Test that partition_config receives correct full hierarchical module paths.
4+
5+
The bug: AutoTP._replace_module built ``full_name`` from ``prev_name`` (the
6+
immediate parent only) instead of ``class_name`` (the accumulated hierarchical
7+
path). Patterns like ``model.layers.0.self_attn.q_proj`` never matched
8+
because the name was just ``0.self_attn.q_proj``.
9+
"""
10+
11+
import pytest
12+
import torch.nn as nn
13+
14+
from deepspeed.module_inject.auto_tp import AutoTP, AutoTPConfig, PartitionType, TPLayerSpec
15+
16+
17+
class SubAttn(nn.Module):
18+
19+
def __init__(self):
20+
super().__init__()
21+
self.q_proj = nn.Linear(32, 32, bias=False)
22+
self.k_proj = nn.Linear(32, 32, bias=False)
23+
self.v_proj = nn.Linear(32, 32, bias=False)
24+
self.o_proj = nn.Linear(32, 32, bias=False)
25+
26+
27+
class DecoderLayer(nn.Module):
28+
29+
def __init__(self):
30+
super().__init__()
31+
self.self_attn = SubAttn()
32+
self.mlp = nn.Sequential(nn.Linear(32, 64), nn.GELU(), nn.Linear(64, 32))
33+
34+
35+
class DummyModel(nn.Module):
36+
37+
def __init__(self, num_layers=2):
38+
super().__init__()
39+
self.embed = nn.Embedding(100, 32)
40+
self.layers = nn.ModuleList([DecoderLayer() for _ in range(num_layers)])
41+
self.head = nn.Linear(32, 100, bias=False)
42+
43+
44+
def _build_config():
45+
"""Partition config that matches q_proj and o_proj via regex."""
46+
return AutoTPConfig(layer_specs=[
47+
TPLayerSpec(patterns=[r".*\.self_attn\.q_proj"], partition_type=PartitionType.COLUMN),
48+
TPLayerSpec(patterns=[r".*\.self_attn\.o_proj"], partition_type=PartitionType.ROW),
49+
])
50+
51+
52+
def _capture_matched_names(model, config):
53+
"""Run _replace_module and capture full_name values that match a spec."""
54+
matched_names = []
55+
original = AutoTP._replace_with_config
56+
57+
def capture(self, child, full_name):
58+
# Only capture if a spec actually matches
59+
param_name = full_name + ".weight"
60+
model_type = self._get_model_type() if hasattr(self, '_get_model_type') else None
61+
spec = config.find_matching_spec(param_name, model_type)
62+
if spec is not None:
63+
matched_names.append(full_name)
64+
return None
65+
66+
AutoTP._replace_with_config = capture
67+
try:
68+
autotp = AutoTP(
69+
module=model,
70+
all_reduce_linears=[],
71+
prefix="model",
72+
state_dict=None,
73+
linear_layer_setting=None,
74+
orig_layer_impl=None,
75+
partition_config=config,
76+
)
77+
autotp._replace_module(model)
78+
finally:
79+
AutoTP._replace_with_config = original
80+
return matched_names
81+
82+
83+
def test_partition_config_receives_full_path():
84+
"""Verify that pattern matching sees the full hierarchical path."""
85+
model = DummyModel(num_layers=2)
86+
config = _build_config()
87+
matched_names = _capture_matched_names(model, config)
88+
89+
for layer_idx in range(2):
90+
assert f"layers.{layer_idx}.self_attn.q_proj" in matched_names, \
91+
f"Expected 'layers.{layer_idx}.self_attn.q_proj', got: {matched_names}"
92+
assert f"layers.{layer_idx}.self_attn.o_proj" in matched_names, \
93+
f"Expected 'layers.{layer_idx}.self_attn.o_proj', got: {matched_names}"
94+
95+
96+
def test_no_truncated_paths():
97+
"""Ensure paths are never truncated to just the immediate parent prefix."""
98+
model = DummyModel(num_layers=3)
99+
config = _build_config()
100+
matched_names = _capture_matched_names(model, config)
101+
102+
for name in matched_names:
103+
assert name.startswith("layers."), \
104+
f"Path should start with 'layers.', got: {name}"
105+
assert ".self_attn." in name, \
106+
f"Path should contain '.self_attn.', got: {name}"
107+
# With the bug, paths would be '0.self_attn.q_proj' (only layer index as prefix)
108+
assert name.count(".") >= 3, \
109+
f"Path should have at least 3 dots (layers.N.self_attn.X_proj), got: {name}"
110+
111+
112+
def test_nested_depth_correct():
113+
"""Verify correct count and paths at 3 layers deep."""
114+
model = DummyModel(num_layers=3)
115+
config = _build_config()
116+
matched_names = _capture_matched_names(model, config)
117+
118+
expected_count = 3 * 2 # 3 layers × (q_proj + o_proj)
119+
assert len(matched_names) == expected_count, \
120+
f"Expected {expected_count} matches, got {len(matched_names)}: {matched_names}"
121+
122+
for layer_idx in range(3):
123+
assert f"layers.{layer_idx}.self_attn.q_proj" in matched_names
124+
assert f"layers.{layer_idx}.self_attn.o_proj" in matched_names
125+
126+
127+
if __name__ == "__main__":
128+
pytest.main([__file__, "-v"])

tests/unit/runtime/activation_checkpointing/test_activation_checkpointing.py

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,3 +309,45 @@ def __init__(self):
309309
assert model._is_checkpointable([layers[0]]) == True # ParallelTransformerLayerPipe
310310
assert model._is_checkpointable([layers[1]]) == True # GMLPBlock
311311
assert model._is_checkpointable([layers[2]]) == False # Linear layer
312+
313+
314+
def test_configure_with_contiguous_checkpointing_requires_num_checkpoints():
315+
# Regression: ``_configure_defaults`` previously initialized ``num_layers``
316+
# to ``False`` while the assert below uses ``is not None``; ``False is not
317+
# None`` is True, so the missing-config assert silently passed and a
318+
# cryptic ``IndexError`` surfaced later from ``range(num_layers)``. With
319+
# the default switched to ``None`` (matching the module-level default),
320+
# the helpful assert message fires at the configure() call site.
321+
#
322+
# ``configure()`` mutates module globals before raising, so snapshot and
323+
# restore them around the call to avoid order-dependent failures in other
324+
# activation-checkpointing tests sharing the same pytest worker.
325+
cp = deepspeed.checkpointing
326+
saved = (
327+
cp.PARTITION_ACTIVATIONS,
328+
cp.CONTIGUOUS_CHECKPOINTING,
329+
cp.num_layers,
330+
cp.CPU_CHECKPOINT,
331+
cp.SYNCHRONIZE,
332+
cp.PROFILE_TIME,
333+
cp.mpu,
334+
cp.deepspeed_checkpointing_enabled,
335+
)
336+
try:
337+
with pytest.raises(AssertionError, match="number of layers"):
338+
deepspeed.checkpointing.configure(
339+
mpu_=None,
340+
partition_activations=True,
341+
contiguous_checkpointing=True,
342+
)
343+
finally:
344+
(
345+
cp.PARTITION_ACTIVATIONS,
346+
cp.CONTIGUOUS_CHECKPOINTING,
347+
cp.num_layers,
348+
cp.CPU_CHECKPOINT,
349+
cp.SYNCHRONIZE,
350+
cp.PROFILE_TIME,
351+
cp.mpu,
352+
cp.deepspeed_checkpointing_enabled,
353+
) = saved

0 commit comments

Comments
 (0)