WIP: Switch to use new Python bindings from cuda-python/cuda-bindings #111

leofang · 2024-10-29T03:32:04Z

Note: Currently to try this PR it requires building cuda-python/cuda-bindings from source:
https://github.com/NVIDIA/cuda-python/tree/main/cuda_bindings

Drop the old, CPython API-based nvJitLink bindings, in favor of the new one from CUDA Python
- Support dynamic linking, no more rebuilding bindings for each CUDA minor release
  - nvidia-nvjitlink-cu12 wheel on Linux is not yet supported (on Windows it works), see Add nvjitlink bindings NVIDIA/cuda-python#98
- Support Windows
- pynvjitlink becomes a pure Python package
All tests in pynvjitlink/tests/test_pynvjitlink.py passed except one (see comment below)
- I need someone to help take a look

copy-pr-bot · 2024-10-29T03:32:08Z

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 · 2024-10-29T03:32:58Z

pynvjitlink/tests/test_pynvjitlink.py

-    with pytest.raises(RuntimeError):
+    with pytest.raises(_nvjitlinklib.nvJitLinkError):
+        # FIXME: For some reason this API would leak the error log to stderr
        _nvjitlinklib.complete(handle)
    error_log = _nvjitlinklib.get_error_log(handle)
    _nvjitlinklib.destroy(handle)
    assert (
        "Undefined reference to '_Z5undefff' "
        "in 'undefined_extern.cubin'" in error_log
-    )
+    ), f"{error_log=}"


This is the failing test:

$ pytest test_pynvjitlink.py --maxfail=3 ============================================================= test session starts ============================================================= platform linux -- Python 3.12.5, pytest-8.3.3, pluggy-1.5.0 rootdir: /local/home/leof/dev/pynvjitlink configfile: pyproject.toml collected 20 items test_pynvjitlink.py ............F....... [100%] ================================================================== FAILURES =================================================================== _____________________________________________________________ test_get_error_log ______________________________________________________________ undefined_extern_cubin = ('undefined_extern.cubin', b'\x7fELF\x02\x01\x013\x07\x00\x00\x00\x00\x00\x00\x00\x01\x00\xbe\x00~\x00\x00\x00\x00\x00...0\x00\x00\x00\x00\x00\x03\x00\x00\x00\x07\x00\x00\x18\x80\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00') gpu_arch_flag = '-arch=sm_89' def test_get_error_log(undefined_extern_cubin, gpu_arch_flag): handle = _nvjitlinklib.create(gpu_arch_flag) filename, data = undefined_extern_cubin input_type = InputType.CUBIN.value _nvjitlinklib.add_data(handle, input_type, data, filename) with pytest.raises(_nvjitlinklib.nvJitLinkError): # FIXME: For some reason this API would leak the error log to stderr _nvjitlinklib.complete(handle) error_log = _nvjitlinklib.get_error_log(handle) _nvjitlinklib.destroy(handle) > assert ( "Undefined reference to '_Z5undefff' " "in 'undefined_extern.cubin'" in error_log ), f"{error_log=}" E AssertionError: error_log='ERROR 9: finish\n\x00' E assert "Undefined reference to '_Z5undefff' in 'undefined_extern.cubin'" in 'ERROR 9: finish\n\x00' test_pynvjitlink.py:81: AssertionError ------------------------------------------------------------ Captured stderr call ------------------------------------------------------------- error : Undefined reference to '_Z5undefff' in '��' =========================================================== short test summary info =========================================================== FAILED test_pynvjitlink.py::test_get_error_log - AssertionError: error_log='ERROR 9: finish\n\x00' ======================================================== 1 failed, 19 passed in 0.57s =========================================================

If we don't capture the exception raised by _nvjitlinklib.complete(handle), I do see the expected error log (no weird unicode mess) showing up in stderr, which is weird/unexpected.

This might be a regression in the version of nvJitLink used. Is that on your local system? What is the nvJitLink version?

leofang · 2024-10-29T03:42:51Z

CMakeLists.txt

 project(
  pynvjitlink
  VERSION ${SKBUILD_PROJECT_VERSION}
-  LANGUAGES CXX CUDA
+  LANGUAGES CXX
 )


Given that this PR turns pynvjitlink into a pure Python package (I think!), I don't really know what to put here, given that I've never used CMake to package a pure Python project... 😆

I don't think it makes sense to involve CMake for a pure Python package. Probably only a pyproject.toml would be necessary.

Yes, we would need to rearchitect the build systems and CI scripts to account for having only a single build of a pure Python package.

leofang · 2024-10-29T03:53:23Z

pynvjitlink/tests/test_patch.py

For some reason I had to set the env var NUMBA_CUDA_USE_NVIDIA_BINDING=1 to run the tests, otherwise I hit weird type errors when Numba tried to load the cubin etc using the Driver APIs (through ctypes).

If that's occurring, I think this PR is likely to be changing the API of pynvjitlink such that the type of a returned cubin is different. I haven't been able to guess exactly what might be happening from the code, so will have to try it locally (or if you have the type error handy, feel free to share it).

gmarkall · 2024-10-29T09:40:39Z

/ok to test

bdice · 2024-11-01T17:56:28Z

@gmarkall @leofang If you'd like a hand with the repackaging to make pynvjitlink a pure Python package, let me know. I can help with some pieces of this.

warsawnv

Just a couple more drive-by observations. Feel free to ignore.

warsawnv · 2024-11-13T21:41:58Z

pynvjitlink/_nvjitlinklib.py

+from cuda.bindings.nvjitlink import nvJitLinkError
+
+
+def nvjitlink_version():


This is kind of an odd looking module. Is it provided just for backward compatibility? Also, why is this one method not just called version()?

This is kind of an odd looking module. Is it provided just for backward compatibility?

Yup! The whole purpose of this module is to drop-in replace the old one (that was implemented using CPython C APIs). This method replaces the following one:

pynvjitlink/pynvjitlink/_nvjitlinklib.cpp

Line 58 in 666375a

static PyObject *nvjitlink_version(PyObject *self, PyObject *Py_UNUSED(args)) {

warsawnv · 2024-11-13T21:43:28Z

pynvjitlink/_nvjitlinklib.py

+
+
+def add_file(*args, **kwargs):
+    raise NotImplementedError("seems unused by pynvjitlink")


If unimplemented, should this shim be omitted? Is it better to get an AttributeError if someone tries to access this or a NotImplementedError when they do?

It's been a while. Looks like I did a faithful translation 😂

pynvjitlink/pynvjitlink/_nvjitlinklib.cpp

Lines 193 to 197 in 666375a

static PyObject *add_file(PyObject *self, PyObject *args) {

set_exception(PyExc_NotImplementedError, "Unimplemented", NVJITLINK_SUCCESS);

return nullptr;

}

gmarkall · 2024-11-29T15:01:31Z

If test binaries need to be built for Windows, I added a script to numba-cuda that builds them - it might be useful to copy across to this PR when Windows support is added / tested: https://github.com/NVIDIA/numba-cuda/blob/main/numba_cuda/numba/cuda/tests/test_binary_generation/build.bat

switch to use new bindings

41ef0ac

leofang commented Oct 29, 2024

View reviewed changes

leofang mentioned this pull request Oct 29, 2024

Support Windows #79

Closed

leofang commented Oct 29, 2024

View reviewed changes

update test_patch.py

2c362d0

leofang commented Oct 29, 2024

View reviewed changes

warsawnv reviewed Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Switch to use new Python bindings from cuda-python/cuda-bindings #111

WIP: Switch to use new Python bindings from cuda-python/cuda-bindings #111

leofang commented Oct 29, 2024 •

edited

Loading

copy-pr-bot bot commented Oct 29, 2024

leofang Oct 29, 2024

leofang Oct 29, 2024

gmarkall Oct 29, 2024

leofang Oct 29, 2024

gmarkall Oct 29, 2024

bdice Oct 29, 2024

leofang Oct 29, 2024

gmarkall Oct 29, 2024

gmarkall commented Oct 29, 2024

bdice commented Nov 1, 2024

warsawnv left a comment

warsawnv Nov 13, 2024

leofang Nov 14, 2024 •

edited

Loading

warsawnv Nov 13, 2024

leofang Nov 14, 2024

gmarkall commented Nov 29, 2024

		from cuda.bindings.nvjitlink import nvJitLinkError


		def nvjitlink_version():



		def add_file(args, *kwargs):
		raise NotImplementedError("seems unused by pynvjitlink")

	static PyObject add_file(PyObject self, PyObject *args) {
	set_exception(PyExc_NotImplementedError, "Unimplemented", NVJITLINK_SUCCESS);

	return nullptr;
	}

WIP: Switch to use new Python bindings from cuda-python/cuda-bindings #111

Are you sure you want to change the base?

WIP: Switch to use new Python bindings from cuda-python/cuda-bindings #111

Conversation

leofang commented Oct 29, 2024 • edited Loading

copy-pr-bot bot commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarkall commented Oct 29, 2024

bdice commented Nov 1, 2024

warsawnv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leofang Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarkall commented Nov 29, 2024

leofang commented Oct 29, 2024 •

edited

Loading

leofang Nov 14, 2024 •

edited

Loading