You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "echuraev (via GitHub)" <gi...@apache.org> on 2024/07/23 13:01:46 UTC
[PR] Fix crash when call del operator for handle [tvm]
echuraev opened a new pull request, #17190:
URL: https://github.com/apache/tvm/pull/17190
In case of cython when we create a set function for property then the following code will be generated:
```
static int __pyx_setprop_4test_9TestClass_handle(PyObject *o, PyObject *v, CYTHON_UNUSED void *x) {
if (v) {
return __pyx_pw_4test_9TestClass_6handle_3__set__(o, v);
}
else {
PyErr_SetString(PyExc_NotImplementedError, "__del__");
return -1;
}
}
```
And when we call operator `del` for this handler, then the memory will be released and operator `__set__` will be called for NULL object. In this case an exception that operator `__del__` is not implemented will be generated. To avoid this problem we need to declare `__del__` function for each property where we define operator `__set__`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Cython][FFI] Fix crash when call del operator for handle [tvm]
Posted by "echuraev (via GitHub)" <gi...@apache.org>.
echuraev commented on PR #17190:
URL: https://github.com/apache/tvm/pull/17190#issuecomment-2247636436
@tqchen I modified `__del__` functions only to set `NULL` to `handle`. Looking at the code in [disco/session.py](https://github.com/apache/tvm/blob/main/python/tvm/runtime/disco/session.py#L93-L96), it looks like we shouldn't release memory after calling `del` operator, we just need to set `NULL` to `handle` of `dref` object and memory should be released by the last instance of this `handle`.
As a test for these changes, we can use [two tests](https://github.com/apache/tvm/blob/main/tests/python/disco/test_session.py#L122-L221) from `disco/test_session.py`. Without this fix, these tests are crashed when we use cython.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Cython][FFI] Fix crash when call del operator for handle [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #17190:
URL: https://github.com/apache/tvm/pull/17190#issuecomment-2248127617
I think I agree with @tqchen here. The `TVMObjectFree` function internally checks whether the handle is a nullptr, so setting `dref.handle = None` would not cause a segfault. Since no C++ object would have its reference count decremented, setting `dref.handle = None` would have the same desired result of removing the ownership of `dref.handle` from `dref`.
If there are other use cases of `del my_obj.handle`, then we probably should still add it to the cython definition, but it could be just a wrapper for `my_obj.handle = None`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Cython][FFI] Fix crash when call del operator for handle [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on code in PR #17190:
URL: https://github.com/apache/tvm/pull/17190#discussion_r1688288273
##########
python/tvm/_ffi/_cython/object.pxi:
##########
@@ -106,6 +106,10 @@ cdef class ObjectBase:
def __set__(self, value):
self._set_handle(value)
+ def __del__(self):
+ if self.chandle != NULL:
Review Comment:
cross check impl of `__dealloc__ ` and see if there will be issue of double free
##########
python/tvm/_ffi/_cython/ndarray.pxi:
##########
@@ -72,6 +72,10 @@ cdef class NDArrayBase:
def __set__(self, value):
self._set_handle(value)
+ def __del__(self):
+ if self.chandle != NULL:
+ CHECK_CALL(TVMFuncFree(self.chandle))
Review Comment:
cross check impl of `__dealloc__ ` function, this should not be `TVMFuncFree `
##########
python/tvm/_ffi/_cython/packed_func.pxi:
##########
@@ -308,15 +308,23 @@ cdef class PackedFuncBase:
def __set__(self, value):
self.c_is_global = value
+ def __del__(self):
+ self.c_is_global = 0
+
property handle:
def __get__(self):
if self.chandle == NULL:
return None
else:
return ctypes.cast(<unsigned long long>self.chandle, ctypes.c_void_p)
+
def __set__(self, value):
self._set_handle(value)
+ def __del__(self):
+ if self.chandle != NULL:
Review Comment:
cross check impl of dealloc function
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Cython][FFI] Fix crash when call del operator for handle [tvm]
Posted by "echuraev (via GitHub)" <gi...@apache.org>.
echuraev commented on PR #17190:
URL: https://github.com/apache/tvm/pull/17190#issuecomment-2246260177
@tqchen thank you for your review! I'll work on your comments tomorrow. About the test case. Originally, problem appeared in disco. When we run model on multiple GPU and TVM was compiled with cython, then we have such crash:
```
Traceback (most recent call last):
File "/opt/dlami/nvme/echuraev/Workspace/OctoML/ollm-lora/mlc-serve/mlc_serve/engine/staging_engine_worker.py", line 421, in run_generation_loop_worker
model_module = model_module_loader(**model_module_loader_kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/dlami/nvme/echuraev/Workspace/OctoML/ollm-lora/mlc-serve/mlc_serve/model/paged_cache_model.py", line 136, in __init__
model, cache_manager = init_tvm_model(model_artifact_config, engine_config)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/dlami/nvme/echuraev/Workspace/OctoML/ollm-lora/mlc-serve/mlc_serve/model/tvm_model.py", line 922, in init_tvm_model
model = Model(model_artifact_config, dev)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/dlami/nvme/echuraev/Workspace/OctoML/ollm-lora/mlc-serve/mlc_serve/model/tvm_model.py", line 112, in __init__
self.mod, self.params, self.disco_session = get_tvm_model(config, dev)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/dlami/nvme/echuraev/Workspace/OctoML/ollm-lora/mlc-serve/mlc_serve/model/tvm_model_utils.py", line 275, in get_tvm_model
return load_disco_module(config.model_artifact_path, lib_path, config.num_shards)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/dlami/nvme/echuraev/Workspace/OctoML/ollm-lora/mlc-serve/mlc_serve/model/tvm_model_utils.py", line 93, in load_disco_module
module, params = load_disco_weights_through_shard_loader(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/dlami/nvme/echuraev/Workspace/OctoML/ollm-lora/mlc-serve/mlc_serve/model/tvm_model_utils.py", line 39, in load_disco_weights_through_shard_loader
module = sess.load_vm_module(lib_path)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/dlami/nvme/echuraev/Workspace/OctoML/ollm-lora/deps/tvm/python/tvm/runtime/disco/session.py", line 309, in load_vm_module
return DModule(func(path, device, alloc_type), self)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/dlami/nvme/echuraev/Workspace/OctoML/ollm-lora/deps/tvm/python/tvm/runtime/disco/session.py", line 94, in __init__
del dref.handle
^^^^^^^^^^^
NotImplementedError: __del__
```
The crash happen [here](https://github.com/apache/tvm/blob/main/python/tvm/runtime/disco/session.py#L95) when we try to delete `dref.handle`.
I simplified the problem and we can create a simple `test.pyx` without any TVM specific:
```python
cdef class TestClass:
property handle:
def __get__(self):
print("Getter")
return None
def __set__(self, value):
print("Setter")
return
```
The property in the code above has the same methods as exists in [packed_func.pxi](https://github.com/apache/tvm/blob/main/python/tvm/_ffi/_cython/packed_func.pxi#L311-L318).
Next, create a `setup.py` file:
```python
from setuptools import setup
from Cython.Build import cythonize
setup(
ext_modules=cythonize("test.pyx"),
)
```
And build it by the following command:
```bash
python setup.py build_ext --inplace
```
After that, we can reproduce original issue:
```python
$ python3
>>> import test
>>> t = test.TestClass()
>>> del t.handle
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
NotImplementedError: __del__
>>>
```
If we add an implementation of `__del__` method to `test.pyx` than we won't have this exception:
- Add this method to `TestClass`:
```
def __del__(self):
print("Dtor")
return
```
- Recompile the code and run the reproducer once again:
```python
$ python3
>>> import test
>>> t = test.TestClass()
>>> del t.handle
Dtor
>>>
```
And after adding implementation of `__del__` methods to the ffi files, the crash in disco also was fixed. If it is necessary, I can try to create a simple unit test tomorrow, but not sure how to integrate cython tests to the TVM infrastructure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Cython][FFI] Fix crash when call del operator for handle [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #17190:
URL: https://github.com/apache/tvm/pull/17190#issuecomment-2248014344
@echuraev thank you! I think in this particular case we should change https://github.com/apache/tvm/blob/main/python/tvm/runtime/disco/session.py#L95 to
```python
dref.handle = None
```
as in https://github.com/apache/tvm/blob/main/python/tvm/runtime/disco/session.py#L83
So we don't have to add del function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Cython][FFI] Fix crash when call del operator for handle [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen merged PR #17190:
URL: https://github.com/apache/tvm/pull/17190
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Cython][FFI] Fix crash when call del operator for handle [tvm]
Posted by "echuraev (via GitHub)" <gi...@apache.org>.
echuraev commented on PR #17190:
URL: https://github.com/apache/tvm/pull/17190#issuecomment-2249515604
@tvm-bot rerun
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Cython][FFI] Fix crash when call del operator for handle [tvm]
Posted by "echuraev (via GitHub)" <gi...@apache.org>.
echuraev commented on PR #17190:
URL: https://github.com/apache/tvm/pull/17190#issuecomment-2245194701
cc: @tqchen, @vinx13, @Lunderberg, @masahi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Cython][FFI] Fix crash when call del operator for handle [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #17190:
URL: https://github.com/apache/tvm/pull/17190#issuecomment-2245586965
can you show a test case that would trigger the error?
One thing we need to be very careful about is whethere there would be double free issue, since the free function was called in dealloc as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Cython][FFI] Fix crash when call del operator for handle [tvm]
Posted by "echuraev (via GitHub)" <gi...@apache.org>.
echuraev commented on PR #17190:
URL: https://github.com/apache/tvm/pull/17190#issuecomment-2248633526
@tqchen, @Lunderberg thank you for your review. Removed implementation of `__del__` methods and set `dref.handle` to `None` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org