NVIDIA / cuda-python

CUDA Python Low-level Bindings
https://nvidia.github.io/cuda-python/
Other
981 stars 76 forks source link

Need to make class destruction more robust #141

Open leofang opened 1 month ago

leofang commented 1 month ago

Possible solutions include

  1. using weakref.finalize() (as you also suggested)
  2. cythonize everything, make all classes cdef'd, and switch from __del__ to __dealloc__
  3. using a lightweight shutdown detection hack (see, e.g., https://github.com/numba/numba/issues/9731#issuecomment-2383856159 and https://github.com/numba/llvmlite/commit/7347d77e5a5494a96fe188108c52ceb8fe39f02c)

_Originally posted by @leofang in https://github.com/NVIDIA/cuda-python/pull/87#discussion_r1792740313_

leofang commented 1 month ago

See also https://github.com/NVIDIA/cuda-python/pull/87#discussion_r1793660730. This doesn't seem trivial to me by a curtesy look at the Python atexit docs...

leofang commented 1 month ago

Moving this to beta 2 and bumping to P0

rwgk commented 1 week ago

I spent a few minutes playing with weakref.finalize() and also looked around.

This seems to work great, even with self.close as the weakref.finalize() callback:

import weakref

class ShopKeeper:
    def __init__(self, serno, exception_harness):
        self.serno = serno
        self.exception_harness = exception_harness
        weakref.finalize(self, self.close)

    def close(self):
        if self.exception_harness:
            try:
                print("close", self.serno, 1 / (self.serno - 2))
            except Exception:
                print("problem", self.serno)
        else:
                print("close", self.serno, 1 / (self.serno - 2))

for exception_harness in [False, True]:
    for serno in range(5):
        ShopKeeper(serno, exception_harness)
$ python3 --version
Python 3.10.2
$ python3 ShopKeeper.py
close 4 0.5
close 3 1.0
problem 2
close 1 -1.0
close 0 -0.5
close 4 0.5
close 3 1.0
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/weakref.py", line 667, in _exitfunc
    f()
  File "/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/weakref.py", line 591, in __call__
    return info.func(*info.args, **(info.kwargs or {}))
  File "/Users/rgrossekunst/Downloads/ShopKeeper.py", line 16, in close
    print("close", self.serno, 1 / (self.serno - 2))
ZeroDivisionError: division by zero
close 1 -1.0
close 0 -0.5

Is it better than __del__?

Maybe not a lot? — See https://peps.python.org/pep-0442/ (Python 3.4 was released in 2014)

But I also found https://groups.google.com/g/dev-python/c/iFlQm0j5lpU/m/viOe4hQeCAAJ, with postings from 2019/2020.

Quoting for easy reference:

The main thing going for weakref.finalize is that it is called at the beginning of the interpreter shutdown while __del__ may be called much later in the process.

My own conclusion: Replacing __del__ with weakref.finalize() seems to be super easy** and will make the code (slightly) more robust.

** I'm counting only 5 __del__ in cuda_core

leofang commented 1 week ago

Great, @rwgk mind I re-assign this task to you? 😛

rwgk commented 1 week ago

Done :-)

I think this is great for me to learn the cuda-python developer workflow.