Repository navigation
Skip accelerator sync for unprofiled comm ops - #8795
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation matches the issue requirements and is covered across all supported profiling-selection modes.
0 open findings
What changed in this PR
Limits accelerator synchronization to communication operations selected for profiling, reducing unnecessary logging overhead.
Changes:
- Guard accelerator synchronization with
should_profile. - Test synchronization behavior across profiling modes.
| File | Description |
|---|---|
deepspeed/comm/comm.py |
Skips synchronization for unprofiled operations. |
tests/unit/comm/test_comms_logger.py |
Verifies synchronization call counts. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Can you fix the DCO error? |
timed_op synchronized the accelerator after every wrapped comm op whenever comms logging was enabled, even for ops that were not selected by prof_ops, prof_all or prof. Those ops never get a timer or a record, so the sync only added overhead. Only synchronize when should_profile is set. The MPI barrier is unchanged. Count synchronize() calls in test_collective_profiling for both profiled and unselected ops. Signed-off-by: guptaishaan <guptaishaan@users.noreply.github.com>
|
Thanks for catching that. The commit was missing its Signed-off-by line. I rebuilt the branch as a single commit on current master with the sign-off added, and will force-push it. The code change is identical to before, and master had no conflicting changes in |
Fixes #8756
timed_opcalledget_accelerator().synchronize()for every wrapped comm op whenever comms logging was enabled. Ops not selected byprof_ops,prof_allorprofget no timer and no record, so the sync was pure overhead.The sync is now guarded by
should_profile. The MPIcdb.barrier()is unchanged.Test:
test_collective_profilingnow countssynchronize()calls. It must be 0 for theunselectedcase and 1 forprof_all,prof, and bothprof_opscases. Without the fix the 8unselectedcases fail; with it all 52 tests intest_comms_logger.pypass. yapf is clean.Verified on a node with A40 GPUs, but the tests use a mocked backend and accelerator, so no GPU was exercised. Not verified: real NCCL/MPI runs, training step timing, or the rest of the unit test suite.
Thanks to @marcusacosta for the report.
This change was prepared by an automated AI agent (Claude) and verified as described above.