Skip to content

Documentation Updates for cuML Python Developer Guide#6843

Merged
rapids-bot[bot] merged 14 commits intorapidsai:branch-25.08from
csadorf:docs/improve-dev-docs
Jun 5, 2025
Merged

Documentation Updates for cuML Python Developer Guide#6843
rapids-bot[bot] merged 14 commits intorapidsai:branch-25.08from
csadorf:docs/improve-dev-docs

Conversation

@csadorf
Copy link
Copy Markdown
Contributor

@csadorf csadorf commented Jun 3, 2025

This PR addresses several documentation improvements outlined in the issue, focusing on the Python Developer Guide and Estimator Guide.

Core Documentation Updates

  • Completed logging section with usage guidance, best practices and examples
  • Completed "Device and Host Memory" section with RMM integration details
  • Completed "Multi GPU" section with usage patterns and limitations
  • Expanded API documentation guidelines with docstring formatting standards and examples
  • Improved section order and overall flow

Estimator Guide Revisions

  • Updated guide to clarify API matching policy:

    • Match scikit-learn/umap-learn APIs where possible and reasonable
    • API deviations must be well-justified and documented
    • Unused parameters or arguments should generally not be matched
    • Exact API matching is not required; consumers should use cuml.accel if that is needed
  • Content Organization:

    • Deleted all sections referencing cuml-cpu functionality
    • Added explicit requirement for keyword-only arguments in constructors

Memory Management Terminology

  • Revised documentation to use "accessibility" terminology instead of physical location references
  • Updated memory management sections to acknowledge that memory can be host or device accessible
  • Removed references to cuml-cpu related components

Closes #6850 .

- Added guidelines on using the rapids-logger library for logging, including usage examples and best practices.
- Updated the documentation style guide for consistency with NumPy docstring standards.
- Expanded the multi-GPU support section with Dask integration details and best practices for effective usage.
- General formatting improvements.
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Jun 3, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@csadorf csadorf added doc Documentation non-breaking Non-breaking change labels Jun 4, 2025
@csadorf csadorf changed the title Improve python/DEVELOPER_GUIDE.md Improve Python Developer Documentation Jun 4, 2025
@csadorf csadorf force-pushed the docs/improve-dev-docs branch from 2ec87bb to a1428ef Compare June 4, 2025 20:19
@csadorf csadorf changed the title Improve Python Developer Documentation Documentation Updates for cuML Python Developer Guide Jun 4, 2025
Comment on lines +329 to +331
# These operations will run concurrently
kmeans1.fit(X1)
kmeans2.fit(X2)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm honestly not sure how true that is considering that the two Python functions are actually not running asynchronously. I've mostly corrected the previously defunct example and made sure that this code runs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This isn't true, kmeans1.fit will fully run before kmeans2.fit starts. It would be true if they were running in separate threads/processes though, but in that case there's no real reason to create your own handles anyway.

Copy link
Copy Markdown
Member

@betatim betatim Jun 5, 2025

Choose a reason for hiding this comment

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

I'm not sure about this specific case but when you talk about benchmarking you often get advice like https://docs.cupy.dev/en/stable/user_guide/performance.html#benchmarking or the very end of section 3 https://docs.pytorch.org/tutorials/recipes/recipes/benchmark.html#benchmarking-with-torch-utils-benchmark-timer

The cupy guide suggests to me a Python function (my_func in that example) can return before all the work is done. All that happens before it returns is that the kernel is launched. What exactly happens when the function returns the result of the kernel (but you don't look at it?) probably depends on the library (not sure if PyTorch and cupy do the same thing?).

But my take away from all this is that you have to be very precise with the example and what you are measuring. To avoid creating more confusion. So maybe we can link to such a guide that is very precise about when what happens? For example, in the k-means example here, what happens if I access kmeans1.cluster_centers_ without calling s1.sync()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed, the kernel functions could in fact run asynchronously until we block on the sync calls, but I just ran some tests and did some profiling and they are certainly not for this example. I made sure to additionally test with all sync calls removed (both within the user code within our code) and it made no difference.

This is certainly worth exploring, but not within the scope of this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given that the statement here isn't true (you can't concurrently run estimators like this), should we rip this section out then? Sorry, I should have been clearer in my review - I approved since it was strictly better than what we had before, but this section is inaccurate and should be amended IMO.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, we probably should and happy to do so in an immediate follow-up. This PR constitutes a general improvement so no need to bike-shed on this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think there is a lot redundancy and verbosity in this guide specifically w.r.t. to array and output type handling that should be improved, but I would prefer to punt on that and roll any necessary revisions into #6463 and #5022 .

@csadorf csadorf marked this pull request as ready for review June 4, 2025 21:34
@csadorf csadorf requested a review from a team as a code owner June 4, 2025 21:34
@csadorf csadorf requested review from cjnolet and jcrist June 4, 2025 21:34
Copy link
Copy Markdown
Member

@jcrist jcrist left a comment

Choose a reason for hiding this comment

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

One comment, otherwise LGTM.

Comment on lines +329 to +331
# These operations will run concurrently
kmeans1.fit(X1)
kmeans2.fit(X2)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This isn't true, kmeans1.fit will fully run before kmeans2.fit starts. It would be true if they were running in separate threads/processes though, but in that case there's no real reason to create your own handles anyway.

@csadorf
Copy link
Copy Markdown
Contributor Author

csadorf commented Jun 5, 2025

/merge

@rapids-bot rapids-bot Bot merged commit b603d65 into rapidsai:branch-25.08 Jun 5, 2025
70 checks passed
@csadorf csadorf deleted the docs/improve-dev-docs branch June 5, 2025 15:37
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.

Improve Python Developer Documentation

4 participants