Skip to content

Cython bindings for libbson #1

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

Merged
merged 4 commits into from
Feb 4, 2021
Merged

Conversation

prashantmital
Copy link
Contributor

@prashantmital prashantmital commented Jan 29, 2021

PR that adds rudimentary bindings for libbson and also an example of how to use them from a C++ cython project.

In order to setup the project you will need to download libbson builds from https://evergreen.mongodb.com/waterfall/mongo-c-driver-r1.17:

  • Start by identifying a debug-compile task build for the platform you are looking to use. E.g. for macOS you can use this build
  • Download the attached tar.gz file and unzip it.
  • Note the abspath to the unzipped tree - e.g /path/to/cdriver

A simpler way to do this on macOS is to use homebrew:

$ brew install mongo-c-driver

To run the included test:

  • Navigate to the mongo-arrow/bindings/python
  • Run CFLAGS=-I/path/to/cdriver/install-dir/include/libbson-1.0 python setup.py build_ext --inplace --force
  • If you used Homebrew to install libbson use the following command: CFLAGS=-I/usr/local/include/libbson-1.0 python setup.py build_ext --inplace --force
  • Run LDFLAGS=-L/path/to/cdriver/install-dir/lib python -m unittest discover test
  • If you used Homebrew to install libbson use the following command: python -m unittest discover test

@prashantmital
Copy link
Contributor Author

prashantmital commented Jan 29, 2021

Note that unlike cffi and ctypes, Cython does not allow setting the path at which to find the shared library object to be imported at runtime. Consequently, I have included the extra_link_args hack in this PR. However, this solution is not portable as it sets the import path at wheel build time.

In order to actually create distributable wheels, we will need to incorporate the delocate utility on macOS and auditwheels repair functionality into the build process to 'fix' the path from which to load libbson at runtime.

Copy link
Collaborator

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

Neat! A few quick comments.

@@ -0,0 +1,35 @@
from setuptools import find_packages, setup, Extension
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add our standard LICENSE file and add the license header to all files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LICENSE has been added at the root of the project. Should I add another under the Python bindings?

Added standard license header to all files.

Copy link
Collaborator

@ShaneHarvey ShaneHarvey Feb 4, 2021

Choose a reason for hiding this comment

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

Yes the LICENSE needs to be included with the distribution, eg:https://github.com/mongodb/mongo-python-driver/blob/master/MANIFEST.in#L2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -31,8 +31,5 @@ MANIFEST
*.c
*.cpp

# PyCharm
*.idea/

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good to keep this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

if _parse_version(libbson_version) < _parse_version(_MIN_LIBBSON_VERSION):
raise RuntimeError(
"Expected libbson version {} or greater, found {}}".format(
_MIN_LIBBSON_VERSION, libbson_version))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think using the library at import time like this can cause some problems down the line. Imagine when we want to integrate this library with pymongo. Then pymongo will look like this:

try:
    import pymongoarrow
except ImportError:
    pass  # not installed 

With the current design we need to catch RuntimeError as well which is a bit odd:

try:
    import pymongoarrow
except (ImportError, RuntimeError):
    pass  # not installed 

I suggest we defer calling into the library until it's first used. For example:

__version__ = bson_get_version().decode('utf-8')

Would become:

def libbson_version():
    return bson_get_version().decode('utf-8')

Feel free to open a follow up ticket if you like to merge this as is and discuss this issue further in Jira.

Edit: a simple alternative option might be to change this line to raise an ImportError instead of a RuntimeError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point except there is no one point of entry for loading libbson in this case so even if the version check is done lazily, it would have to be explicitly added to every possible method in the libbson bindings that could be directly called by external code.

I have changed it to raise an ImportError instead.

@prashantmital prashantmital merged commit 0bb2434 into main Feb 4, 2021
@prashantmital prashantmital deleted the feat/libbson-cython-bindings branch April 19, 2021 20:49
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.

2 participants