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

Merge mscclpp-lang to mscclpp project #442

Merged
merged 37 commits into from
Jan 22, 2025
Merged

Merge mscclpp-lang to mscclpp project #442

merged 37 commits into from
Jan 22, 2025

Conversation

Binyang2014
Copy link
Contributor

@Binyang2014 Binyang2014 commented Jan 7, 2025

First step to merge msccl-tools into mscclpp repo. In this step will move all msccl related code, pass the current tests and do some necessary refactor.

Add mscclpp.language module
Add _InstructionOptimizer and DagOptimizer class to optimize the dag
Add DagLower to lower dag to intermediate representation
Add documents for mscclpp.language
Remove msccl related code

@Binyang2014 Binyang2014 changed the title Merge msccl-tools Merge mscclpp-lang to mscclpp project Jan 8, 2025
@Binyang2014 Binyang2014 marked this pull request as ready for review January 8, 2025 02:20
@Binyang2014 Binyang2014 requested a review from Copilot January 8, 2025 02:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 10 out of 25 changed files in this pull request and generated 2 comments.

Files not reviewed (15)
  • python/mscclpp/language/init.py: Evaluated as low risk
  • python/mscclpp/language/dag/init.py: Evaluated as low risk
  • .azure-pipelines/nccl-api-test.yaml: Evaluated as low risk
  • python/mscclpp/language/dag/lower.py: Evaluated as low risk
  • python/mscclpp/language/types.py: Evaluated as low risk
  • python/mscclpp/language/dag/instruction_dag.py: Evaluated as low risk
  • python/mscclpp/language/utils.py: Evaluated as low risk
  • python/examples/allreduce_allpairs_get.py: Evaluated as low risk
  • python/examples/allreduce_allpairs_packet.py: Evaluated as low risk
  • python/examples/allreduce_allpairs.py: Evaluated as low risk
  • python/mscclpp/language/collectives.py: Evaluated as low risk
  • python/mscclpp/language/rank.py: Evaluated as low risk
  • python/mscclpp/language/buffer.py: Evaluated as low risk
  • python/examples/allreduce_nvls.py: Evaluated as low risk
  • .github/workflows/mscclpp-lang.yml: Evaluated as low risk
Comments suppressed due to low confidence (3)

python/examples/send_recv_packet.py:28

  • The parameters temp_buffer and temp_buffer_index are not used in the send_recv function. This might be an oversight or an incomplete implementation.
c.put_packet(

python/examples/send_recv_packet.py:11

  • [nitpick] The function name send_recv is ambiguous as it is the same as in the other example. Consider renaming it to send_recv_packet to avoid confusion.
def send_recv(instances):

python/examples/send_recv_packet.py:0

  • Ensure that the new behavior introduced by put_packet and copy_packet is covered by tests.
def send_recv(instances):

python/mscclpp/language/chunk.py Outdated Show resolved Hide resolved
python/mscclpp/language/chunk.py Outdated Show resolved Hide resolved
@Binyang2014 Binyang2014 requested review from yzygitzh and removed request for yzygitzh January 16, 2025 04:58
docs/index.rst Outdated Show resolved Hide resolved
python/examples/allgather_barrier.py Show resolved Hide resolved
docs/design/mscclpp-dsl.md Outdated Show resolved Hide resolved
.azure-pipelines/nccl-api-test.yaml Outdated Show resolved Hide resolved
python/test/generate_mscclpp_lang_test_result.py Outdated Show resolved Hide resolved
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.

two general comments:

  • did we validate that nccl-tests and rccl-tests are passing with this PR?
  • in future, please add more descriptions for the commit messages

docs/design/mscclpp-dsl.md Outdated Show resolved Hide resolved
docs/design/mscclpp-dsl.md Outdated Show resolved Hide resolved
docs/design/mscclpp-dsl.md Outdated Show resolved Hide resolved
docs/design/mscclpp-dsl.md Outdated Show resolved Hide resolved
docs/design/mscclpp-dsl.md Show resolved Hide resolved
docs/design/mscclpp-dsl.md Outdated Show resolved Hide resolved
docs/design/mscclpp-dsl.md Outdated Show resolved Hide resolved
docs/design/mscclpp-dsl.md Outdated Show resolved Hide resolved
docs/design/mscclpp-dsl.md Outdated Show resolved Hide resolved
python/examples/allgather_barrier.py Show resolved Hide resolved
@Binyang2014
Copy link
Contributor Author

two general comments:

  • did we validate that nccl-tests and rccl-tests are passing with this PR?
  • in future, please add more descriptions for the commit messages
  1. Yes, I triggered CI with nccl-test and passed with this PR.
  2. Let me try to add more descriptions. This PR is special since merge many code from msccl-tools, mixed many changes.

Copy link
Contributor

@chhwang chhwang left a comment

Choose a reason for hiding this comment

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

LGTM

@Binyang2014
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

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.

Thanks for addressing the comments. Looks good to me.

@Binyang2014 Binyang2014 merged commit af0bb86 into main Jan 22, 2025
23 of 25 checks passed
@Binyang2014 Binyang2014 deleted the binyli/mscclpp-lang branch January 22, 2025 17:47
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