Skip to content

Conversation

susmnty
Copy link

@susmnty susmnty commented Sep 4, 2025

have done the documentation in the basic of understanding and the workflow, feel free to make changes.

Copy link

copy-pr-bot bot commented Sep 4, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@csadorf csadorf added doc Documentation non-breaking Non-breaking change labels Sep 4, 2025

- **Memory leaks**: forgetting to free memory after use.
- **Dangling pointers**: using freed memory by mistake.
- **Incompatibility**: does not integrate well with=========================================
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax error

Raw Allocation vs. RMM
----------------------
**❌ Raw Allocation Example:**
.. code-block:: cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

please review this in the github preview; this doesn't render correctly

Comment on lines 113 to 36
Migration Strategy
------------------
When contributing to cuML:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this apply to new contributions? Or a migration effort? This seems unclear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Overall this seems a bit overly verbose. I would prefer this to be a bit more concise and tailored to the specific needs of the cuML project. We should avoid duplicating the RMM docs and instead refer to those where appropriate.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I’ve simplified the documentation to avoid duplication with RMM, kept it cuML-specific, and added a reference link to the RMM docs. Please let me know if further adjustments are needed.

@susmnty susmnty force-pushed the docs-memory-allocation branch from ddddc64 to 949ecab Compare September 5, 2025 03:51
@csadorf
Copy link
Contributor

csadorf commented Sep 5, 2025

Thanks for addressing my comments.

Quick note: please do not merge the upstream branch unless this branch is significantly out of date. It unnecessarily increases our Ci worker load.

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Please make sure to integrate this guide with wiki/cpp/DEVELOPER_GUIDE.md .

@susmnty susmnty requested a review from a team as a code owner September 5, 2025 19:11
@susmnty susmnty requested a review from divyegala September 5, 2025 19:11
@susmnty
Copy link
Author

susmnty commented Sep 5, 2025

Thanks for the feedback! I’ve integrated the memory management guide into wiki/cpp/DEVELOPER_GUIDE.md as requested. Also noted about not merging upstream unless the branch is significantly out of date - I’ll keep that in mind going forward. This should now be ready for another review.

@susmnty
Copy link
Author

susmnty commented Sep 5, 2025

quite sorry again for the unnecessary clicking of the update branch, noted I wont press it again the branch is actually out of the date.

@csadorf
Copy link
Contributor

csadorf commented Sep 5, 2025

Thanks for the feedback! I’ve integrated the memory management guide into wiki/cpp/DEVELOPER_GUIDE.md as requested. Also noted about not merging upstream unless the branch is significantly out of date - I’ll keep that in mind going forward. This should now be ready for another review.

Please actually integrate it rather than linking to it. Thanks!

@susmnty
Copy link
Author

susmnty commented Sep 6, 2025

Thanks for the feedback! I’ve now integrated the memory management content directly into DEVELOPER_GUIDE.md instead of linking out. Please let me know if there are any further adjustments needed.

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

I have some suggestions for improvement. More generally I am unsure about the duplication of information.


When contributing to cuML:

- Replace raw ``cudaMalloc/cudaFree`` with RMM containers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Replace raw ``cudaMalloc/cudaFree`` with RMM containers.
- Use RMM containers instead of raw ``cudaMalloc``/``cudaFree``.

When contributing to cuML:

- Replace raw ``cudaMalloc/cudaFree`` with RMM containers.
- Prefer ``device_uvector`` for typed data.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Prefer ``device_uvector`` for typed data.
- Prefer ``rmm::device_uvector<T>`` for typed data.


- Replace raw ``cudaMalloc/cudaFree`` with RMM containers.
- Prefer ``device_uvector`` for typed data.
- Refer to the official `RMM documentation <https://github.com/rapidsai/rmm>`_ for advanced features. No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Refer to the official `RMM documentation <https://github.com/rapidsai/rmm>`_ for advanced features.
- Refer to the `RMM documentation <https://docs.rapids.ai/api/rmm/>`_ or `RMM repository <https://docs.rapids.ai/api/rmm/>`_ for more information.

thrust::for_each(execution_policy->on(stream), ... );
}
```
## Memory Management in cuML
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have duplicate information? It seems like we have two developer guides. What is the difference between them? Maybe a question for @csadorf @dantegd.

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering the information density, we should just integrate everything into this document. We should also be careful in duplicating information from the RMM docs and instead refer to them where possible.

thrust::for_each(execution_policy->on(stream), ... );
}
```
## Memory Management in cuML
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering the information density, we should just integrate everything into this document. We should also be careful in duplicating information from the RMM docs and instead refer to them where possible.

@csadorf
Copy link
Contributor

csadorf commented Sep 30, 2025

I'm retargeting this for 25.12.

@csadorf csadorf changed the base branch from branch-25.10 to branch-25.12 September 30, 2025 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Documentation non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants