Skip to content

Various handle-related changes and improvements #463

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

Merged
merged 16 commits into from
Feb 25, 2025
Merged

Conversation

leofang
Copy link
Member

@leofang leofang commented Feb 22, 2025

Close #370. Close #433.

  • Ensure Program/Linker have consistent .backend and .handle
  • Add get_cuda_native_handle() to fetch the underlying CUDA C object from the corresponding cuda.bindings Python-layer object
    • Recall that for all generated bindings there are 3 layers: Python, Cython, and internal. This refers to the Python layer. The Cython layer provides the C type information (i.e. think of it as a CUDA header)
    • Steps:
      1. Take the Python wrapper object from <some_cuda_core_object>.handle
      2. Pass it to get_cuda_native_handle(), and the underlying C object is returned (no refcount or lifetime management)
    • Since cuda.core is not yet cythonized, this is currently the best solution for Python/Cython/C++ interoperability. Once we lower to Cython we can also apply a similar treatment to all cuda.core objects.
    • For reviewers of this part: Please focus on these two files (the rest of Cython-related files are direct copy/paste from our existing cuda.bindings infrastructure):
      • tests/cython/test_get_cuda_native_handle.pyx
      • cuda/core/experimental/include/utility.hpp
  • some minor doc build fixes

@leofang leofang added enhancement Any code-related improvements P0 High priority - Must do! cuda.core Everything related to the cuda.core module labels Feb 22, 2025
@leofang leofang added this to the cuda.core beta 3 milestone Feb 22, 2025
@leofang leofang self-assigned this Feb 22, 2025
Copy link
Contributor

copy-pr-bot bot commented Feb 22, 2025

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.

@leofang leofang changed the title WIP: Ensure Program/Linker have consistent .backend and .handle Various handle-related changes and improvements Feb 24, 2025
@leofang
Copy link
Member Author

leofang commented Feb 24, 2025

/ok to test

@leofang leofang requested review from vzhurba01 and rwgk and removed request for vzhurba01 February 24, 2025 01:01
@leofang leofang marked this pull request as ready for review February 24, 2025 01:03

#include <type_traits>

// In cuda.bindings 12.8, the private member name was renamed from "_ptr" to "_pvt_ptr".
Copy link
Member Author

Choose a reason for hiding this comment

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

@vzhurba01 do you recall what prompted this change when updating to 12.8?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We're using a private member? Do we own the code with the private member?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we own everything, they are autogen'd, for example for CUstream:

cdef class CUstream:
"""
CUDA stream
Methods
-------
getPtr()
Get memory address of class instance
"""
cdef cydriver.CUstream _pvt_val
cdef cydriver.CUstream* _pvt_ptr

hence I feel SFINAE over all possibilities is OK. I think if we accidentally change the codegen in the future, the added Cython test should be able to catch it.

This comment has been minimized.

@leofang leofang added the breaking Breaking changes are introduced label Feb 24, 2025
@leofang
Copy link
Member Author

leofang commented Feb 24, 2025

/ok to test

1 similar comment
@leofang
Copy link
Member Author

leofang commented Feb 24, 2025

/ok to test

@leofang
Copy link
Member Author

leofang commented Feb 24, 2025

/ok to test

@leofang
Copy link
Member Author

leofang commented Feb 24, 2025

/ok to test

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

I only have a few minor comments. I didn't see anything troubling, except the raise vs pass question maybe.


#include <type_traits>

// In cuda.bindings 12.8, the private member name was renamed from "_ptr" to "_pvt_ptr".
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're using a private member? Do we own the code with the private member?

@rwgk
Copy link
Collaborator

rwgk commented Feb 24, 2025

I only have a few minor comments. I didn't see anything troubling, except the raise vs pass question maybe.

Oh wait, did the except ImportError: raise disappear in the meantime? Please disregard.

rwgk
rwgk previously approved these changes Feb 24, 2025
item = py_func(item)
setattr(sys.modules[__name__], name, item)
except ImportError:
raise
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here it is, I accidentally lost my original comment.

Did you mean to write pass here?

Also, it's safer to make the try-except scope as narrow as possible, e.g. (untested):

    try:
        # For each callable in `mod` with name `test_*`,
        # wrap the callable in a plain Python function
        # and set the result as an attribute of this module.
        mod = importlib.import_module(mod)
    except ImportError:
        pass
    else:
        for name in dir(mod):
            item = getattr(mod, name)
            if callable(item) and name.startswith("test_"):
                item = py_func(item)
                setattr(sys.modules[__name__], name, item)

If there is any tooling that might import this file for analysis: Moving the for loop into a function and using if __name__ == "__main__": could guard against surprises.

Copy link
Member Author

Choose a reason for hiding this comment

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

This file is copied/pasted from cuda-bindings. I think Vlad (or @shwina? 🙂) was the original author:
https://shwina.github.io/cython-testing/

Did you mean to write pass here?

I suspect the answer was yes (based on Ashwin's blog post) but perhaps it caused some issues, so it was changed to raise? I can see that this is a cheap way to ensure the listed modules must exist and get tested. But then the whole try-except can be removed (or refactored). Perhaps @vzhurba01 can confirm the intention?

In any case, since this is a copy/paste let's change both files in a separate PR.

@leofang
Copy link
Member Author

leofang commented Feb 24, 2025

/ok to test

#!/bin/bash

SCRIPTPATH=$(dirname $(realpath "$0"))
CPLUS_INCLUDE_PATH=$SCRIPTPATH/../../cuda/core/experimental/include:$CUDA_HOME/include:$CPLUS_INCLUDE_PATH cythonize -3 -i $(dirname "$0")/test_*.pyx
Copy link
Member Author

@leofang leofang Feb 24, 2025

Choose a reason for hiding this comment

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

Note: This approach left a bug uncaught (fixed in commit 37a6850): Ideally, we want to have a cmdline functionality like PyBind11 that returns the header path, something like python -m cuda.core --include-path. Here, we use the header from the local clone of this repo (hence using $SCRIPTPATH) instead of the installed cuda.core package, meaning we do not check if the header is actually packaged (and it was not before the fix). However, I think the said functionality needs to be done in a separate PR...

Copy link
Member Author

Choose a reason for hiding this comment

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

rwgk
rwgk previously approved these changes Feb 24, 2025
@leofang
Copy link
Member Author

leofang commented Feb 24, 2025

/ok to test

@rwgk
Copy link
Collaborator

rwgk commented Feb 25, 2025

/ok to test

@leofang leofang merged commit 976246a into NVIDIA:main Feb 25, 2025
73 checks passed
@leofang leofang deleted the handle branch February 25, 2025 13:11
Copy link

Doc Preview CI
Preview removed because the pull request was closed or merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking changes are introduced cuda.core Everything related to the cuda.core module enhancement Any code-related improvements P0 High priority - Must do!
Projects
None yet
3 participants