-
Notifications
You must be signed in to change notification settings - Fork 16
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
Conversation
Note that unlike 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. |
There was a problem hiding this 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.
bindings/python/setup.py
Outdated
@@ -0,0 +1,35 @@ | |||
from setuptools import find_packages, setup, Extension |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
bindings/python/.gitignore
Outdated
@@ -31,8 +31,5 @@ MANIFEST | |||
*.c | |||
*.cpp | |||
|
|||
# PyCharm | |||
*.idea/ | |||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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:
debug-compile
task build for the platform you are looking to use. E.g. for macOS you can use this buildtar.gz
file and unzip it./path/to/cdriver
A simpler way to do this on macOS is to use homebrew:
To run the included test:
mongo-arrow/bindings/python
CFLAGS=-I/path/to/cdriver/install-dir/include/libbson-1.0 python setup.py build_ext --inplace --force
CFLAGS=-I/usr/local/include/libbson-1.0 python setup.py build_ext --inplace --force
LDFLAGS=-L/path/to/cdriver/install-dir/lib python -m unittest discover test
python -m unittest discover test