Skip to content

[fix] VM resource options - #3343

Open
martindurant wants to merge 3 commits into
NVIDIA:mainfrom
martindurant:mem-opt
Open

martindurant wants to merge 3 commits into
NVIDIA:mainfrom
martindurant:mem-opt

Conversation

@martindurant

Copy link
Copy Markdown

Description

Solves parts of #2910

In VirtualMemoryResourceOptions, •

  • Reject allocation_type="managed" with ValueError.
  • Default Windows handles to None, preserving Linux’s POSIX descriptor default.

These are points 3 and 6 of the linked issue.

Checklist

  • I have read the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@copy-pr-bot

copy-pr-bot Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

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.

@martindurant martindurant self-assigned this Nov 3, 2026
@github-actions github-actions Bot added the cuda.core Everything related to the cuda.core module label Nov 3, 2026
@martindurant martindurant added bug Something isn't working and removed cuda.core Everything related to the cuda.core module labels Nov 3, 2026
@martindurant martindurant added this to the cuda.core 1.3.0 milestone Nov 3, 2026
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Explain Change →

Summary

Summary by CodeRabbit

  • Bug Fixes
    • Virtual memory allocations now reject the unsupported MANAGED allocation type with a ValueError; use the managed memory resource for managed allocations.
    • The default handle type is platform-specific: None on Windows and POSIX_FD on other platforms. Host allocations require None.
  • Documentation
    • Clarified supported allocation types, platform-specific handle defaults, accepted handle type values, and the requirement to use None for host allocations.

Virtual memory resource options now use platform-specific handle defaults and accept None. VirtualMemoryResource supports pinned allocations and validates allocation types during option initialization and configuration checks. Documentation, release notes, and tests reflect these changes.

Changes: Virtual memory resource options

Layer / File(s) Summary
Platform-specific handle defaults
cuda_core/cuda/core/_memory/_virtual_memory_resource.pyi, cuda_core/cuda/core/_memory/_virtual_memory_resource.pyx, cuda_core/tests/test_memory.py, cuda_core/docs/source/release/1.3.0-notes.rst
handle_type accepts strings and None. Its default is None on Windows and POSIX_FD elsewhere. Host-location checks and tests reflect the handle requirements.
Pinned-only allocation validation
cuda_core/cuda/core/_memory/_virtual_memory_resource.pyx, cuda_core/cuda/core/_memory/_virtual_memory_resource.pyi, cuda_core/cuda/core/typing.py, cuda_core/tests/test_memory.py, cuda_core/docs/source/release/1.3.0-notes.rst
The allocation map contains only PINNED. Option initialization and configuration checks validate allocation types. Documentation identifies MANAGED as unsupported, and tests check managed allocation rejection.

Possibly related issues

  • NVIDIA/cuda-python issue 2910: The change rejects allocation_type="managed" and sets the Windows handle default to None, while retaining the POSIX descriptor default elsewhere.

Suggested reviewers: andy-jost

Change: Bug fix

Merge Risk: Low · up to 55c15

Windows users could follow the documentation into an unsupported VMM configuration; correct the platform guidance before merging.

- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

@Andy-Jost Andy-Jost added the P1 Medium priority - Should do label Nov 3, 2026

@Andy-Jost Andy-Jost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code changes look good. I agree with the coderabbit items. Will approve once those are complete.

@github-actions github-actions Bot added the cuda.core Everything related to the cuda.core module label Nov 4, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

Minor · 🎯 Functional Correctness · _virtual_memory_resource.pyx:109

cuda_core/cuda/core/_memory/_virtual_memory_resource.pyx:109
Minor · Functional Correctness · Quick win

important: Restore the Linux qualifier for posix_fd import/export guidance.

cuda.core transports POSIX descriptors for VMM IPC only on Linux. The current wording can direct Windows users to an unsupported configuration. Apply the same correction in the .pyi documentation.

-        :attr:`VirtualMemoryResource.is_ipc_enabled`). Use ``"posix_fd"`` on
-        if you plan to import/export the allocation. Use `None` if you
+        :attr:`VirtualMemoryResource.is_ipc_enabled`). Use ``"posix_fd"`` on
+        Linux if you plan to import/export the allocation. Use `None` if you
Review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between a519baa and 55c156e.

Files selected for processing: 3
  • cuda_core/cuda/core/_memory/_virtual_memory_resource.pyi
  • cuda_core/cuda/core/_memory/_virtual_memory_resource.pyx
  • cuda_core/tests/test_memory.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

@Andy-Jost

Copy link
Copy Markdown
Contributor

/ok to test 55c156e

@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cuda.core Everything related to the cuda.core module P1 Medium priority - Should do

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants