Skip to content

Documentation Update for NCCL#132

Merged
mrava87 merged 5 commits into
PyLops:mainfrom
tharittk:docs-nccl
Jun 3, 2025
Merged

Documentation Update for NCCL#132
mrava87 merged 5 commits into
PyLops:mainfrom
tharittk:docs-nccl

Conversation

@tharittk
Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread docs/source/installation.rst Outdated
Comment thread docs/source/installation.rst Outdated
Comment thread docs/source/installation.rst
Copy link
Copy Markdown
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 🚀

Comment thread Makefile Outdated
Comment thread Makefile
Comment thread Makefile Outdated
Comment thread Makefile Outdated
Comment thread README.md Outdated
Comment thread docs/source/gpu.rst Outdated
Comment thread docs/source/index.rst Outdated
Comment thread docs/source/installation.rst
Comment thread docs/source/installation.rst Outdated
Comment thread docs/source/installation.rst
Copy link
Copy Markdown
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
Copy Markdown
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