fix: make device header self-contained#51
Open
voltjia wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make
src/device.hself-contained by including<cstdint>directly and usingstd::uint8_tas the underlying type ofMemorySpace.This removes reliance on a transitive declaration of
uint8_t. It does not add collective functionality or change runtime behavior, enum values, or supported platforms and backends.Changes
Header dependency
<cstdint>directly insrc/device.h.Fixed-width type
MemorySpaceasstd::uint8_t.Platform and Backend Affected
src/device.his shared by every device implementation and the common communication paths, so all supported configurations are affected at compile time. Runtime behavior is unchanged.Platform
Backend
Performance Impact
N/A. This change only makes a header dependency explicit and preserves the existing fixed-width enum representation.
Known Issues & Future Work
Test Results
Environment:
accelerator-dev/nvidia:latest(NVIDIA PyTorch 25.12)Results:
clang-format-16 --dry-run --Werror src/device.h: passed.ruff check .: passed.ruff format --check .: passed.src/device.h: passed.scripts/run_examples.pyforccl/all_reduce: passed with two NVIDIA GPUs.The applicable example was run as:
python scripts/run_examples.py \ --config /workspace/ccl-cluster.yaml \ --examples ccl/all_reduce \ --log-path /workspace/ccl-example-logs \ --timeout 600 \ -- -g 2 -w 1 -p 1 -n 1048576Generated
ccl_all_reduce.logresult:Test Involved Platform
Test Involved Backend
Checklist
Title, Branch, and Commits
<type>/xxx-yyyy-zzzz, with the type matching the PR title.masterwith no stray merge commits.fixup!,squash!, orwipcommits remain.Scope and Design
TODOitems were introduced.General Code Hygiene
C++ Specific (if C++ files changed)
clang-formatversion 16 has been run against the modified file; the diff is clean.Python Specific (if Python files changed)
Testing
Build, CI, and Tooling
clang-format.ymlandruff.ymlchecks pass locally.Documentation
Security and Safety