Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renaming channels #436

Open
wants to merge 42 commits into
base: main
Choose a base branch
from
Open

Renaming channels #436

wants to merge 42 commits into from

Conversation

chhwang
Copy link
Contributor

@chhwang chhwang commented Jan 3, 2025

No description provided.

@chhwang
Copy link
Contributor Author

chhwang commented Jan 5, 2025

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@chhwang chhwang changed the base branch from main to chhwang/malloc January 6, 2025 00:09
Base automatically changed from chhwang/malloc to main January 8, 2025 02:40
Copy link

@mahdiehghazim mahdiehghazim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approved this PR. just have a few general comments/clarifying questions:

  • Based on the paper, the implementation of each channel is specific to the interconnect. For example, the PortChannel uses the DMA-copy data transfer mode over NVLink, xGMI, PCIe, and Infiniband." Does it mean we have different implementations for PortChannel to support each interconnect (NVLink, xGMI, PCIe, and Infiniband)? same question for MemoryChannel

  • Since this PR is changing many files, we need to make sure all mscclpp tests as well as nccl-tests/rccl-tests are passing before the merge

  • Once the paper is out, we can add a link to it in the repo

Copy link
Contributor

@Binyang2014 Binyang2014 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, maybe we can merge mscclpp-lang related PR first. Then hold-on other PR requests and merge this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants