Skip to content

Conversation

@MC952-arch
Copy link
Collaborator

@MC952-arch MC952-arch commented Nov 17, 2025

PR Category

UIL

PR Types

Bug Fixes

PR Description

This PR updates the Python ctypes wrapper to accurately reflect the latest flagcx.h device-handle and stream-related definitions.

Key Updates

  • Aligned flagcxDeviceHandle fields with the C header, adding and restructuring function-pointer typedefs for: Device queries (getDevice, getDeviceCount, getVendor); Host/device pointer mapping; Stream creation/destruction and event operations; IPC memory handle creation/opening/closing/freeing.
  • Standardized ctypes typedefs (flagcxMemcpyType_t, flagcxMemType_t, flagcxEventType_t, etc.) and ensured consistent pointer usage (e.g., flagcxStream_t, flagcxUniqueId_t, flagcxIpcMemHandle_t).
  • Updated adaptor_stream_copy implementation to correctly construct and pass stream pointers, eliminating mismatches between cudaStream_t and c_void_p.
  • Cleaned up and deduplicated the module exports (all) to remove repeated symbols and export a coherent public API.

@MC952-arch
Copy link
Collaborator Author

@gemini-code-assist please review this pull request in details

@MC952-arch MC952-arch changed the title [UIL] Flagcx wrapper [UIL] Update Python ctypes Bindings to Match Revised flagcxDeviceHandle and IPC/Stream APIs Nov 17, 2025
@MC952-arch MC952-arch changed the title [UIL] Update Python ctypes Bindings to Match Revised flagcxDeviceHandle and IPC/Stream APIs [UIL] Update python ctypes bindings to match revised deviceHandle and Ipc/Stream APIs Nov 17, 2025
@aoyulong aoyulong requested a review from Copilot November 17, 2025 09:53
Copilot finished reviewing on behalf of aoyulong November 17, 2025 09:56
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the Python ctypes bindings to align with the revised C header API (flagcx.h) for device handles, streams, and IPC memory operations.

Key Changes:

  • Refactored flagcxStream_t from a custom structure to an opaque pointer type (c_void_p) to match the C API definition
  • Added IPC memory handle operations and hostGetDevicePointer to the flagcxDeviceHandle structure
  • Updated stream handling in adaptor_stream_copy to work with the new pointer-based stream type

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@mikethegoblin mikethegoblin 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

@ceci3 ceci3 left a comment

Choose a reason for hiding this comment

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

LGTM

@MC952-arch
Copy link
Collaborator Author

@gemini-code-assist please review this pull request in details

@MC952-arch MC952-arch requested a review from Copilot November 19, 2025 03:27
Copilot finished reviewing on behalf of MC952-arch November 19, 2025 03:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

EVENT_SYNCHRONIZE_FUNCTYPE = ctypes.CFUNCTYPE(flagcxResult_t, flagcxEvent_t)
EVENT_QUERY_FUNCTYPE = ctypes.CFUNCTYPE(flagcxResult_t, flagcxEvent_t)

IPC_MEM_HANDLE_CREATE_FUNCTYPE = ctypes.CFUNCTYPE(flagcxResult_t, ctypes.POINTER(flagcxIpcMemHandle_t), ctypes.POINTER(ctypes.c_size_t))
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The IPC_MEM_HANDLE_CREATE_FUNCTYPE signature appears inconsistent with the C header. According to flagcx.h line 158-159, ipcMemHandleCreate takes flagcxIpcMemHandle_t *handle (pointer to pointer) and size_t *size, but flagcxIpcMemHandle_t is already defined as ctypes.c_void_p (line 22), which is a single pointer. The first parameter should be ctypes.POINTER(ctypes.c_void_p) to match the double-pointer indirection.

Copilot uses AI. Check for mistakes.
Comment on lines +71 to +72
IPC_MEM_HANDLE_GET_FUNCTYPE = ctypes.CFUNCTYPE(flagcxResult_t, flagcxIpcMemHandle_t, ctypes.c_void_p)
IPC_MEM_HANDLE_OPEN_FUNCTYPE = ctypes.CFUNCTYPE(flagcxResult_t, flagcxIpcMemHandle_t, ctypes.POINTER(ctypes.c_void_p))
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The parameter types for IPC_MEM_HANDLE_GET_FUNCTYPE and IPC_MEM_HANDLE_OPEN_FUNCTYPE are swapped. According to flagcx.h lines 160-162, ipcMemHandleGet takes (flagcxIpcMemHandle_t handle, void *devPtr) while ipcMemHandleOpen takes (flagcxIpcMemHandle_t handle, void **devPtr). The current definitions have the pointer indirection reversed - line 71 should have ctypes.POINTER(ctypes.c_void_p) as the second parameter, and line 72 should have ctypes.c_void_p.

Suggested change
IPC_MEM_HANDLE_GET_FUNCTYPE = ctypes.CFUNCTYPE(flagcxResult_t, flagcxIpcMemHandle_t, ctypes.c_void_p)
IPC_MEM_HANDLE_OPEN_FUNCTYPE = ctypes.CFUNCTYPE(flagcxResult_t, flagcxIpcMemHandle_t, ctypes.POINTER(ctypes.c_void_p))
IPC_MEM_HANDLE_GET_FUNCTYPE = ctypes.CFUNCTYPE(flagcxResult_t, flagcxIpcMemHandle_t, ctypes.POINTER(ctypes.c_void_p))
IPC_MEM_HANDLE_OPEN_FUNCTYPE = ctypes.CFUNCTYPE(flagcxResult_t, flagcxIpcMemHandle_t, ctypes.c_void_p)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants