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

DOC: Document that GIL must be grabbed (and Py_IsInitialized()) #140

Merged
merged 4 commits into from
Mar 22, 2024

Conversation

seberg
Copy link
Contributor

@seberg seberg commented Mar 22, 2024

I honestly don't remember if that Py_IsInitialized wasn't just to tape over C++ related deficiencies, but I suspect there isn't much to be avoided there...

(I.e. if we take care of the GIL for the other library, we also have to do this check for them which would normally be their job.)

Closes gh-103

I honestly don't remember if that ``Py_IsInitialized`` wasn't just
to tape over C++ related deficiencies, but I suspect there isn't
much to be avoided there...

(I.e. if we take care of the GIL for the other library, we also
have to do this check for them which would normally be their job.)

Closes dmlcgh-103
docs/source/python_spec.rst Outdated Show resolved Hide resolved
docs/source/python_spec.rst Outdated Show resolved Hide resolved
docs/source/python_spec.rst Outdated Show resolved Hide resolved
@pitrou
Copy link
Contributor

pitrou commented Mar 22, 2024

I honestly don't remember if that Py_IsInitialized wasn't just to tape over C++ related deficiencies

Our experience in PyArrow (not in DLPack context) is that it's required for robust integration with C++:
https://github.com/apache/arrow/blob/51817917e1436c8799ed382d160798060cd76652/python/pyarrow/src/arrow/python/common.h#L194-L199

@seberg
Copy link
Contributor Author

seberg commented Mar 22, 2024

Yeah, makes sense: If you knew it came from Python, it would be your job to store it in a way like that OwnedRef which deals with it. But the whole issue here is that we want to allow you "forget" that this came from Python...

@@ -134,6 +134,37 @@ C API which is called either when the refcount on the capsule named
Note: the capsule names ``"dltensor"`` and ``"used_dltensor"`` must be
statically allocated.

The ``DLManagedTensor`` deleter must ensure that sharing beyond Python
boundaries is possible, this means that the GIL must be acquired explicitly.
Copy link
Member

@tqchen tqchen Mar 22, 2024

Choose a reason for hiding this comment

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

aquired explicitly if the original memory comes from python runtime and deleter involves interacting with python runtime. If the original memory allocation comes from other languages(e.g. c++ and exposed to python) and de-allocation does not involve python runtime, then it is not necessary to grab the GIL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair, added "[...] if it uses Python objects or API"

@tqchen tqchen merged commit 6764572 into dmlc:main Mar 22, 2024
3 checks passed
@seberg seberg deleted the gil-sharing branch March 22, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interaction of DLManagedTensor::deleter and the GIL
4 participants