Skip to content

Commit 9bb44a8

Browse files
jinyouzhiCopilot
andcommitted
Bind positional prof/log_name args and cover with regression tests
timed_op previously read prof/log_name only from kwargs, so callers that passed them positionally (e.g. dist.all_to_all(out, inp, None, False, True, 'name')) had those values silently ignored. Bind args and kwargs against func's signature with inspect.signature(...). bind_partial() instead, so positional values are picked up the same as keyword ones. This makes the earlier keyword-only ('*') marker on broadcast_object_list/all_to_all unnecessary, so it is reverted. The signature binding stays inside the 'if comms_logger.enabled' guard to keep the disabled fast path at its previous cost. Add regression tests for reading prof/log_name from positional args and for calc_bw_log supporting broadcast_object_list/all_to_all. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jin, Youzhi <youzhi.jin@intel.com>
1 parent 94ed85d commit 9bb44a8

2 files changed

Lines changed: 44 additions & 4 deletions

File tree

deepspeed/comm/comm.py

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
import torch
2525
from torch.distributed import GradBucket # noqa: F401
26+
import inspect
2627
import os
2728
from typing import Any, Optional, TYPE_CHECKING
2829

@@ -110,8 +111,12 @@ def log_wrapper(*args, **kwargs):
110111
should_profile = False
111112
# Add enabled flag so that overhead to each comm op is two if conditions at most
112113
if comms_logger.enabled:
113-
selected_log_name = kwargs.get('log_name', default_log_name)
114-
should_profile = (('prof' in kwargs and kwargs['prof']) or comms_logger.prof_all
114+
# prof/log_name may be passed positionally, so bind args/kwargs to
115+
# func's signature rather than only looking at kwargs
116+
bound_args = inspect.signature(func).bind_partial(*args, **kwargs)
117+
bound_args.apply_defaults()
118+
selected_log_name = bound_args.arguments.get('log_name', default_log_name)
119+
should_profile = (bound_args.arguments.get('prof', False) or comms_logger.prof_all
115120
or selected_log_name in comms_logger.prof_ops)
116121
if should_profile:
117122
# Need func args for their defaults
@@ -238,7 +243,6 @@ def broadcast_object_list(object_list,
238243
src,
239244
group=None,
240245
device=None,
241-
*,
242246
prof=False,
243247
log_name='broadcast_object_list',
244248
debug=get_caller_func()):
@@ -379,7 +383,6 @@ def all_to_all(output_tensor_list,
379383
input_tensor_list,
380384
group=None,
381385
async_op=False,
382-
*,
383386
prof=False,
384387
log_name='all_to_all',
385388
debug=get_caller_func()):

tests/unit/comm/test_comms_logger.py

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,3 +94,40 @@ def barrier(log_name='barrier'):
9494

9595
assert barrier() == 'done'
9696
assert 'barrier' in comm.comms_logger.comms_dict
97+
98+
99+
def test_timed_op_reads_prof_and_log_name_from_positional_args(monkeypatch):
100+
# prof/log_name are ordinary parameters, so callers may pass them positionally
101+
# (e.g. dist.all_to_all(out, inp, None, False, True, "my_all_to_all")). timed_op
102+
# used to look them up only in kwargs, silently ignoring positional values.
103+
from deepspeed.comm import comm
104+
105+
monkeypatch.setattr(comm, 'comms_logger', CommsLogger())
106+
monkeypatch.setattr(
107+
comm, 'cdb', SimpleNamespace(using_mpi=False, is_initialized=lambda: True,
108+
get_world_size=lambda group=None: 1))
109+
monkeypatch.setattr(comm, 'get_accelerator', lambda: SimpleNamespace(synchronize=lambda: None))
110+
111+
@comm.timed_op
112+
def barrier(prof=False, log_name='barrier'):
113+
return 'done'
114+
115+
comm.comms_logger.enabled = True
116+
117+
assert barrier(True, 'custom_barrier') == 'done'
118+
assert 'custom_barrier' in comm.comms_logger.comms_dict
119+
120+
121+
def test_calc_bw_log_supports_object_and_list_collectives(monkeypatch):
122+
# broadcast_object_list and all_to_all gained profiling support but were
123+
# missing from calc_bw_log, so profiling them hit the "wrong comm_op
124+
# specified" fallback and exited the process instead of logging bandwidth.
125+
import deepspeed.comm as dist
126+
from deepspeed.utils.comms_logging import calc_bw_log
127+
128+
monkeypatch.setattr(dist, 'get_world_size', lambda group=None: 2)
129+
130+
for comm_op in ('broadcast_object_list', 'all_to_all'):
131+
tput, busbw = calc_bw_log(comm_op, 1024, 1.0)
132+
assert tput > 0
133+
assert busbw > 0

0 commit comments

Comments
 (0)