Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose option to enable fabric memory handle support to Python #1787

Draft
wants to merge 3 commits into
base: branch-25.02
Choose a base branch
from

Conversation

pentschev
Copy link
Member

Description

Follow-up to #1743, exposing a way to enable fabric memory handle to Python.

Checklist

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

Copy link

copy-pr-bot bot commented Jan 20, 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.

@github-actions github-actions bot added the Python Related to RMM Python API label Jan 20, 2025
@pentschev
Copy link
Member Author

/ok to test

1 similar comment
@bdice
Copy link
Contributor

bdice commented Jan 20, 2025

/ok to test

Comment on lines 197 to 200
descriptor = <allocation_handle_type>(
<int>descriptor |
<int>c_allocation_handle_type_posix_file_descriptor
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely not the prettiest way to write this bitwise or, I couldn't find anything much better with Cython though. In pure C++ I would at least have used std::underlying_type_t but that's not exposed to Cython. If anybody has a better idea, please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have a TODO to use an enum class instead of enum, now that we're using Cython 3. That might help here, I'm not sure.

# TODO: when we adopt Cython 3.0 use enum class

Here's an example of an enum class in Cython from cudf:

https://github.com/rapidsai/cudf/blob/9cd525361c198713a8eaa41fa4d91ad1fe176942/python/pylibcudf/pylibcudf/libcudf/unary.pxd#L13

Copy link
Member Author

Choose a reason for hiding this comment

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

We can make the switch but it doesn't seem to help Cython, unfortunately:

      Error compiling Cython file:
      ------------------------------------------------------------
      ...
              # constructor below will raise an error from C++.
              cdef allocation_handle_type descriptor = allocation_handle_type.none
              if enable_ipc:
                  descriptor = descriptor | allocation_handle_type.posix_file_descriptor
              if enable_fabric:
                  descriptor = descriptor | allocation_handle_type.fabric
                                          ^
      ------------------------------------------------------------

      ~/src/rmm/python/rmm/rmm/pylibrmm/memory_resource.pyx:196:36: Invalid operand types for '|' (allocation_handle_type; allocation_handle_type)

I've anyway applied the change in 85a95de, but left the explicit cast in to make sure it compiles.

Copy link
Contributor

@vyasr vyasr Jan 24, 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 what we're hoping to happen here.

  • Switching to an enum class is a good thing to do in general for modern C++ usage, but if anything it makes the situation here worse because scoped enumerations are (by design) strongly typed and will not convert automatically to an integer even in C++ (here's a quick demo).
  • One difference in binding C-style enums is that unlike in C they will not will not automatically convert to integers. You will have to cast yourself, as you found. Here's an example:
In [3]: %%cython
    ...: cdef enum A:
    ...:     x = 1
    ...:     y = 2
    ...:
    ...: cdef A a = A.x
    ...: cdef A b = A.y
    ...: cdef A c = a | b
    ...: print(c)
    ...:
    ...:

Error compiling Cython file:
------------------------------------------------------------
...
    x = 1
    y = 2

cdef A a = A.x
cdef A b = A.y
cdef A c = a | b
             ^
------------------------------------------------------------

/home/coder/.cache/ipython/cython/_cython_magic_698ab74b4727f04ebc23520f0c784e1a85498944.pyx:7:13: Cannot assign type 'int' to 'A'
  • With a cpdef enum you get both a Python and a C enum (that's what the cpdef does). Because of how Python enums work, you will see this operator work in pure Python code because it uses the Python enum wrapper, but it will not work in typed Cython code. Compare
In [4]: %%cython --cplus
   ...: cpdef enum class A:
   ...:     x = 1
   ...:     y = 2
   ...:
   ...: a = A.x
   ...: b = A.y
   ...: c = a | b
   ...: print(c)
   ...:
   ...:
3

with

In [5]: %%cython --cplus
   ...: cpdef enum class A:
   ...:     x = 1
   ...:     y = 2
   ...:
   ...: cdef A a = A.x
   ...: cdef A b = A.y
   ...: cdef A c = a | b
   ...: print(c)
   ...:
   ...:

Error compiling Cython file:
------------------------------------------------------------
...
    x = 1
    y = 2

cdef A a = A.x
cdef A b = A.y
cdef A c = a | b
             ^
------------------------------------------------------------

/home/coder/.cache/ipython/cython/_cython_magic_b143dd57cf4b954689b33b6386f8a1098deb2571.pyx:7:13: Invalid operand types for '|' (A; A)

The "pure Python" usage above would work equally well if you had an enum instead of an enum class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Python Related to RMM Python API
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants