Skip to content

[Typo] Replace rank to local_rank#3908

Merged
samnordmann merged 1 commit intomainfrom
fix_rank_to_local_rank
Feb 18, 2025
Merged

[Typo] Replace rank to local_rank#3908
samnordmann merged 1 commit intomainfrom
fix_rank_to_local_rank

Conversation

@samnordmann
Copy link
Collaborator

@samnordmann samnordmann commented Feb 17, 2025 •
edited
Loading

fixes a typo

@samnordmann samnordmann mentioned this pull request Feb 17, 2025
Closed
@github-actions
Copy link

github-actions bot commented Feb 17, 2025

Description

  • Corrected typo: replaced rank with local_rank

  • Updated variable names for consistency

Changes walkthrough 📝

Relevant files
Bug fix
executor.cpp
Replace `rank` with `local_rank` in `executor.cpp`             

csrc/host_ir/executor.cpp

  • Replaced my_device_index_ with my_local_device_index_
  • Updated getCurrentCUDAStream calls to use my_local_device_index_
    		• +3/-3     
    executor.h
    Rename `my_device_index_` to `my_local_device_index_`       

    csrc/host_ir/executor.h

    • Renamed my_device_index_ to my_local_device_index_
    		 +1/-1     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Variable Naming

    The variable my_local_device_index_ is introduced to replace my_device_index_. Ensure that all usages of my_device_index_ are correctly updated to my_local_device_index_ to avoid any unintended behavior.

    my_local_device_index_(communicator_ ? communicator_->local_rank() : 0) {
    Consistency Check

    Verify that the change from deviceId() to local_rank() is consistent with the intended functionality and does not introduce any logical errors.

    my_local_device_index_(communicator_ ? communicator_->local_rank() : 0) {
    Impact Analysis

    Evaluate the impact of this change on the performance and correctness of the application, especially in multi-GPU environments.

    my_local_device_index_(communicator_ ? communicator_->local_rank() : 0) {

    @samnordmann samnordmann mentioned this pull request Feb 17, 2025
    Merged
    @samnordmann
    Copy link
    Collaborator Author

    samnordmann commented Feb 18, 2025

    !test

    @samnordmann samnordmann merged commit 7bb2d61 into main Feb 18, 2025
    47 of 52 checks passed
    @samnordmann samnordmann deleted the fix_rank_to_local_rank branch February 18, 2025 17:30
    samnordmann added a commit that referenced this pull request Mar 12, 2025
    @samnordmann
    [CudaIpc 3/3]: p2p get-Zcopy (#3911)
    677b84c 
    On top of:
- #3910
- #3909
- #3908

Pending on issue:
- #3907
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @wujingyue wujingyue wujingyue approved these changes

    Assignees

    No one assigned

    Labels

    None yet

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @samnordmann @wujingyue