Skip to content

Mempool memory resource - IPC - #446

Closed
keenan-simpson wants to merge 29 commits into
NVIDIA:mainfrom
keenan-simpson:IPC
Closed

Mempool memory resource - IPC#446
keenan-simpson wants to merge 29 commits into
NVIDIA:mainfrom
keenan-simpson:IPC

Conversation

@keenan-simpson

@keenan-simpsonkeenan-simpson commented Feb 12, 2025

Copy link
Copy Markdown
Contributor

This is ready for at least a first look

Add Mempool class as the first public MemoryResource implementation. It supports IPC.

Add IPC buffer which is an internal buffer implementation used to represent a buffer descriptor which is used to export and import buffers across process boundaries. It implements reduce which is expected by common multiprocessing libraries. It's use is seen in the tests added to this review.

@copy-pr-bot

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.

@keenan-simpson

Copy link
Copy Markdown
ContributorAuthor

/ok to test

@leofangleofang added triage Needs the team's attention P0 High priority - Must do! feature New feature or request cuda.core Everything related to the cuda.core module labels Feb 14, 2025
@keenan-simpson

Copy link
Copy Markdown
ContributorAuthor

/ok to test

@github-actions

Copy link
Copy Markdown

@keenan-simpson

Copy link
Copy Markdown
ContributorAuthor

/ok to test

@keenan-simpsonkeenan-simpson changed the title WIP: IPCMempool memory resource - IPCFeb 24, 2025
@keenan-simpson

Copy link
Copy Markdown
ContributorAuthor

/ok to test

1 similar comment
@keenan-simpson

Copy link
Copy Markdown
ContributorAuthor

/ok to test

Comment threadcuda_core/cuda/core/experimental/_memory.py Outdated
@keenan-simpson

Copy link
Copy Markdown
ContributorAuthor

/ok to test

@leofang
leofang self-requested a review February 25, 2025 05:50
@keenan-simpson

Copy link
Copy Markdown
ContributorAuthor

/ok to test

@keenan-simpson

Copy link
Copy Markdown
ContributorAuthor

/ok to test

@keenan-simpson

Copy link
Copy Markdown
ContributorAuthor

/ok to test

Comment on lines +22 to +35
# Check if pywin32 is available on Windows
_PYWIN32_AVAILABLE = False
if platform.system() == "Windows":
try:
import win32security

_PYWIN32_AVAILABLE = True
except ImportError:
import warnings

warnings.warn(
"pywin32 module not found. For better IPC support on Windows, " "install it with: pip install pywin32",
stacklevel=2,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How hard would it be to recreate the functionality we need from here to avoid this optional dependency?

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.

I think we'll need to discuss our next move first and then clean this up based on the decision. Keenan struggled for quite a long time and it turns out the IPC support for mempools is not enabled on Windows at all. There is a number of doc and sample bugs we'll create later (the memory team created one).

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've created 2 bugs to address the documentation issues.

yeah the review is in limbo right now. I'm not sure if we should push the change for IPC mempools and have it only support linux for now, or if we should shelve it and add a cuMem and VMM API IPC memory_resource in the meantime.

All of the Windows specific changes should be treated as draft. That optional dependency is from when I was debugging windows errors and operating under the assumption that I was improperly creating the windows security descriptor for the mempool handle. In hindsight it was a (temporary) lack of suport in driver layer as Leo said.

@keenan-simpson
keenan-simpson marked this pull request as ready for review March 5, 2025 18:13
@keenan-simpson

Copy link
Copy Markdown
ContributorAuthor

/ok to test

@keenan-simpson

keenan-simpson commented Mar 6, 2025

Copy link
Copy Markdown
ContributorAuthor

I am tasked with adding the IPC mempool support for windows at the driver level (5151668), so I think we should roll with this implementation and have it pick up windows once that change is integrated. Users who need IPC on windows for older CTKs can implement their own MR or use the cuMemCreate & VMM API bindings. WDYT

@leofang
leofang marked this pull request as draft May 2, 2025 04:30
@leofangleofang linked an issue Jun 26, 2025 that may be closed by this pull request
@leofangleofang added this to the cuda.core beta 7 milestone Aug 26, 2025
@leofangleofang removed the triage Needs the team's attention label Sep 11, 2025
@leofangleofang added blocked This task is currently blocked by other tasks and removed P0 High priority - Must do! labels Sep 17, 2025
@leofang

leofang commented Sep 17, 2025

Copy link
Copy Markdown
Member

IPC support for Linux landed (#930) 🎉 We'll revisit (and close) this PR when there's a viable path emerging for Windows counterpart (and preferably not through VMM #968) .

github-actionsBot pushed a commit that referenced this pull request Dec 4, 2025
Removed preview folders for the following PRs:
- PR #1305
- PR #446
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blockedThis task is currently blocked by other taskscuda.coreEverything related to the cuda.core modulefeatureNew feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure the mempool wrapped behind DeviceMemoryResource can be customized

5 participants

@keenan-simpson@leofang@kkraus14@jrhemstad@Andy-Jost