Skip to content

Documentation Update for NCCL #132

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

Merged
merged 5 commits into from
Jun 3, 2025
Merged

Documentation Update for NCCL #132

merged 5 commits into from
Jun 3, 2025

Conversation

tharittk
Copy link
Collaborator

@tharittk tharittk commented May 23, 2025

Ongoing update - in parallel to NCCL implementation PR (#130)

Tasks

  • Update README mentioning the possibility to use NCCL instead of MPI for distributed cupy arrays, updating the install, example and tests sections with NCCL-related commands
  • Update index.rst similar to README to reflect new NCCL engine
  • Update gpu.rst documenting the new env variable (NCCL_PYLOPS_MPI), adding NCCL to the example, and perhaps consider adding a table like in https://pylops.readthedocs.io/en/stable/gpu.html to document what features are supported in NCCL and what are not, eg the missing support for complex numbers (this can also serve as a live roadmap for you work, as we progress we should see more and more features being supported by both MPI and NCCL)

Copy link
Contributor

@mrava87 mrava87 left a comment

Choose a reason for hiding this comment

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

Just left a few comments on what you have done so far, this needs much more to be ready but hopefully with the checklist that I put in the description of the PR you will have a easier life to navigate the documentation and add bits related to NCCL

Copy link
Contributor

@mrava87 mrava87 left a comment

Choose a reason for hiding this comment

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

Very good work @tharittk, I left some additional mostly stylistic comments... once you have address them, I think for now this is already a very good improvement to the documentation to include NCCL related stuff and we can continue revising it as we progress with the code development 🚀

Copy link
Contributor

@mrava87 mrava87 left a comment

Choose a reason for hiding this comment

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

@tharittk this looks great to me!

@rohanbabbar04 you want to have a look or shall I go ahead and merge?

Copy link
Collaborator

@rohanbabbar04 rohanbabbar04 left a comment

Choose a reason for hiding this comment

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

Yup, looks good, I am going to approve this.
@tharittk, once the merge conflicts are fixed, it is good to go. I have fixed the linting with flake8 in this PR #134, so no need to change the setup.cfg.

@tharittk tharittk marked this pull request as ready for review June 3, 2025 13:52
@mrava87 mrava87 merged commit 97f8f5a into PyLops:main Jun 3, 2025
61 checks passed
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