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

Wrap pybind11 API and and Fix #1106 #1124

Merged
merged 1 commit into from
Dec 19, 2022
Merged

Wrap pybind11 API and and Fix #1106 #1124

merged 1 commit into from
Dec 19, 2022

Conversation

mewim
Copy link
Collaborator

@mewim mewim commented Dec 19, 2022

This PR adds the following:

  • A Python wrapper for the pybind11 Python API. This creates a layer around the APIs exposed by pybind11, which gives us the flexibility to override and extend Python APIs with native Python script instead of doing everything in C++. (Note that if an override is not created, the C++ endpoints are still exposed and can be called by the user, so we do not have to wrap everything in Python if not necessary).
  • Fix Closing ipython session without closing conn causes BufferManagerException #1106. The fix is as follows:
    • In query_result.py, the __del__ method is added, which will close the query result automatically.
    • In query_result.py, a reference to connection is added. This ensures that when the garbage collection is invoked for the connection, the corresponding query results have to be deleted first.
    • Similarly, a reference to database is added to the connection. Overall, the above measurements ensures the garbage collection order of query result -> connection -> database to prevent the memory leak caused by database being closed first.
    • To ensure Closing ipython session without closing conn causes BufferManagerException #1106 is properly fixed, an automated test case is added (test_query_result_close.py). Note that the issue is not reproducible in pytest. Therefore, this script invokes a new Python process. It is verified that without the above measurements, this test script does fail.
  • Additionally, this PR overrides the APIs in query_result.py to use sneak_case in accordance with the Python standard.

tools/python_api/CMakeLists.txt Outdated Show resolved Hide resolved
tools/python_api/src_py/connection.py Outdated Show resolved Hide resolved
tools/python_api/src_py/query_result.py Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@@ -3,17 +3,21 @@ project(_kuzu)

set(CMAKE_CXX_STANDARD 20)

file(GLOB SOURCE_PY
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather not use GLOB, but list all necessary files here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have changed the C++ target to use the explicit path instead of using GLOB. But I think we can keep the GLOB for Python files since CMake is not actually running any build script against the Python files. It is just copying the files to the build directory.

@mewim mewim force-pushed the py-api-wrapper branch 2 times, most recently from 35efc30 to 4da3069 Compare December 19, 2022 16:02
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.

Closing ipython session without closing conn causes BufferManagerException
3 participants