Repository navigation
docs(cuda.core): document PinnedMemoryResource.allocate parameters - #2713
Conversation
The override's one-line docstring is what autodoc renders, so the generated page for the public class documented neither the required keyword-only stream argument nor the return value, and never mentioned the RuntimeError raised when the device does not support the requested host memory pool. Give the override the full numpydoc body. The .pyi stub is regenerated by the stubgen-pyx-cuda-core pre-commit hook. Closes NVIDIA#2712. Signed-off-by: Vyron Vasileiadis <hi@fedonman.com>
|
/ok to test 3626c66 |
Andy-Jost
left a comment
There was a problem hiding this comment.
Thank you for this PR, and sorry for the eight-week wait. The expanded docstring is a clear improvement, and the Parameters and Raises sections match the implementation. One sentence needs a change before merge.
cuda_core/cuda/core/_memory/_pinned_memory_resource.pyx, line 132: the Returns text says the buffer is accessible "from the device that the stream belongs to". The implementation does not establish that.PinnedMemoryResourcecreates its pool withCUmemPoolPropsonly and never callscuMemPoolSetAccess, and the CUDA Driver API documentation forcuMemPoolCreatesays that pools created withCU_MEM_LOCATION_TYPE_HOSTorCU_MEM_LOCATION_TYPE_HOST_NUMAare accessible from the host CPU by default. Please drop the clause after "accessible from the host" (the attached suggestion does that), and regenerate the.pyistub so the two stay in sync.
Whether PinnedMemoryResource.is_device_accessible is right without a cuMemPoolSetAccess call is a separate question. I will track it in its own issue. It does not block this PR.
I am adding the labels and milestone and starting CI now. After the edit, CI runs once more, and I merge when it is green.
| The allocated buffer object, which is accessible from the host and | ||
| from the device that the stream belongs to. |
There was a problem hiding this comment.
| The allocated buffer object, which is accessible from the host and | |
| from the device that the stream belongs to. | |
| The allocated buffer object, which is accessible from the host. |
There was a problem hiding this comment.
Thanks for the review! Done.
|
…(cuDSS 1.0 library names) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
I merged |
|
/ok to test 0b5bfa3 |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
|
| Layer / File(s) | Summary |
|---|---|
Document the allocate contract cuda_core/cuda/core/_memory/_pinned_memory_resource.pyx, cuda_core/cuda/core/_memory/_pinned_memory_resource.pyi |
The docstrings describe the required stream argument, returned Buffer, unsupported-pool RuntimeError, and alternative resource. The method declaration and implementation are unchanged. |
Assessment against linked issues
| Objective | Addressed | Explanation |
|---|---|---|
Document the required stream argument, return value, and unsupported-pool RuntimeError for PinnedMemoryResource.allocate [#2712] |
✅ |
Possibly related issues
- NVIDIA/cuda-python#2795: The docstring describes the unsupported-pool error path that this issue reports can incorrectly reject supported
HOST_NUMApools. The PR does not change that behavior. - NVIDIA/cuda-python#3205: This issue concerns the device-accessibility contract for pinned buffers, which is related to the documented
Bufferreturn.
Possibly related PRs
- NVIDIA/cuda-python#3034: The release-note gate may apply to the
.pyxand.pyichanges; the supplied context does not confirm the outcome. - NVIDIA/cuda-python#3209: This PR and the current PR both edit the pinned memory resource stub header, so their merge order may affect whether regeneration is needed.
Suggested reviewers: uday1o1
Priority:
Change: Other · Severity of issue fixed: Low
Merge Risk: 
Minimal · previous review fc1d6
-
The documentation-only change is ready to merge.
Comment @coderabbitai help to get the list of available commands.
…llocate The Returns text said the buffer is accessible from the device that the stream belongs to. PinnedMemoryResource creates its pool from CUmemPoolProps alone and never calls cuMemPoolSetAccess, and the driver documents host-located pools as accessible from the host CPU by default, so only the host access is established. Signed-off-by: Vyron Vasileiadis <hi@fedonman.com>
|
/ok to test fc1d657 |
|
/ok to test eb4f272 |
Description
closes #2712
PinnedMemoryResource.allocateoverrides the inherited method with a one-line docstring, and that one line is all autodoc has to render, so the generated page for the public class documented neither the required keyword-onlystreamargument nor the return value, and never mentioned theRuntimeErrorraised when the device does not support the requested host memory pool. This gives the override the full numpydoc body, including aRaisessection for that error. The.pyistub is regenerated by thestubgen-pyx-cuda-corepre-commit hook rather than hand-edited.Docstring-only change, so there is no new test. What the class now renders, and the existing suite:
No release note: the one-line docstring landed in #2487, after the
cuda-core-v1.1.1tag, so nothing user-visible changed since the last release.Checklist