[DRIVERS-2926] [PYTHON-4577] BSON Binary Vector Subtype Support #1813

caseyclements · 2024-08-24T01:22:41Z

Adds support for new subtype of Binary BSON subtype to be used for densely-packed 1d arrays (vectors).
Corresponding changes are in specifications repo PR #1658

bson/vector.py

bson/__init__.py

bson/vector.py

bson/binary.py

…otes.

…t.test_bson.TestBSON.test_encode_type_marker to pass

…ector, from_vector

bson/binary.py

doc/changelog.rst

ShaneHarvey · 2024-09-30T23:22:37Z

bson/binary.py

+ elif dtype == BinaryVectorDtype.FLOAT32:
+ n_bytes = len(self) - position
+ n_values = n_bytes // 4
+ assert n_bytes % 4 == 0


We should not use assert for validation data. If the argument type is incorrect we raise a TypeError, if the type is correct but the value is invalid we raise a ValueError.

ShaneHarvey · 2024-09-30T23:22:53Z

bson/binary.py

+ """
+ if dtype == BinaryVectorDtype.INT8: # pack ints in [-128, 127] as signed int8
+ format_str = "b"
+ assert not padding, f"padding does not apply to {dtype=}"


Same comment about assert.

ShaneHarvey · 2024-09-30T23:22:57Z

bson/binary.py

+ format_str = "B"
+ elif dtype == BinaryVectorDtype.FLOAT32: # pack floats as float32
+ format_str = "f"
+ assert not padding, f"padding does not apply to {dtype=}"


Same comment about assert.

ShaneHarvey · 2024-09-30T23:26:16Z

bson/binary.py

+
+ .. versionadded:: 4.9
+ """
+


We need to validate self.subtype == 9 here before attempting to decode.

@ShaneHarvey How do I do that?

Something like:

if self.subtype != VECTOR_SUBTYPE: raise ValueError(...)

Look at as_uuid it should be the same as that validation:

if self.subtype not in ALL_UUID_SUBTYPES: raise ValueError(f"cannot decode subtype {self.subtype} as a uuid")

ShaneHarvey · 2024-09-30T23:33:14Z

bson/binary.py

+ .. versionadded:: 4.9
+ """
+
+ INT8 = b"\x03"


An enum of bytes is a bit unusual. Can we change it to use ints? eg INT8 = 3

The naming convention that Geert used has meaning in both the first and last 4 bits, so I'd prefer it to stay as-is.

I don't see how the interpretation of the values here matters. Are you saying this is a bit flag?

The user doesn't see the value locally either. Given that the bson spec itself always uses integers I think we should use the integer form.

ShaneHarvey · 2024-09-30T23:36:11Z

bson/binary.py

+ .. versionadded:: 4.9
+ """
+
+ data: Sequence[float | int]


Add __slots__ = ("data", "dtype", "padding") to reduce the memory overhead of using this class.

Sure. How do I do type annotation for that?

None needed, e.g.

mongo-python-driver/bson/timestamp.py

Line 31 in 821811e

__slots__ = ("__time", "__inc")

We could pass slots=True instead: https://docs.python.org/3/library/dataclasses.html#dataclasses.dataclass

Strange. And you're cool with that?

We have to add them manually instead of using slots=True.

class BinaryVector: """**(BETA)** Vector of numbers along with metadata for binary interoperability. :param data (Sequence[float | int]): Sequence of numbers representing the mathematical vector. :param dtype (:class:`bson.Binary.BinaryVectorDtype`): The data type stored in binary :param padding (Optional[int] = 0): The number of bits in the final byte that are to be ignored when a vector element's size is less than a byte and the length of the vector is not a multiple of 8. Default is 0. .. versionadded:: 4.10 """ __slots__ = ("data", "dtype", "padding") def __init__(self, data, dtype, padding=0): self.data = data self.dtype = dtype self.padding = padding

Is this right? @blink1073

Oh I didn't realize __slots__ with dataclass was problematic.

It should be sorted now

ShaneHarvey · 2024-10-01T00:10:17Z

bson/binary.py

+ .. versionadded:: 4.9
+ """
+
+ INT8 = b"\x03"


I don't see how the interpretation of the values here matters. Are you saying this is a bit flag?

ShaneHarvey · 2024-10-01T00:14:54Z

bson/binary.py

+
+ data: Sequence[float | int]
+ dtype: BinaryVectorDtype
+ padding: Optional[int] = 0


Optional[int] -> int, unless padding=None is considered valid.

padding=None is expected when it is not a packed type

Is it? I see the code using padding=0 in that case.

ShaneHarvey · 2024-10-01T00:15:30Z

bson/binary.py

+ cls: Type[Binary],
+ vector: list[int, float],
+ dtype: BinaryVectorDtype,
+ padding: Optional[int] = 0,


Optional[int] -> int

ShaneHarvey · 2024-10-01T00:17:21Z

bson/binary.py

+ data = struct.pack(f"{len(vector)}{format_str}", *vector)
+ return cls(metadata + data, subtype=VECTOR_SUBTYPE)
+
+ def as_vector(self, uncompressed: Optional[bool] = False) -> BinaryVector:


Optional[bool] -> bool unless uncompressed=None is valid.

ShaneHarvey · 2024-10-01T00:20:58Z

bson/binary.py

+ else:
+ bits = []
+ for uint8 in unpacked_uint8s:
+ bits.extend([int(bit) for bit in f"{uint8:08b}"])


Why add an uncompressed option here at all? It looks like this option is irreversible because from_vector does not support the same option. Even if it did, is this option useful outside of test code?

Yeah, I think we should leave out uncompressed, it was a quality of life addition, but is not likely to be standardized.

Yes. It's irreversible. We don't yet provide an API to go from a full vector of zeros and ones to a packed bit vector.

Going in, you'd be using something like numpy.packbits

Let's omit it since it adds unneeded complexity at this point.

ShaneHarvey · 2024-10-01T01:21:26Z

Fixing the test suite and the Enum issue are my last remaining comments.

blink1073 · 2024-10-01T02:01:59Z

We're pressing ahead with the hex values in the Enum because they simplify the implementation of encode and decode.

blink1073 · 2024-10-01T02:02:45Z

Again, the user will only ever see BinaryVectorDtype.PACKED_BIT in their code.

…ryVectorDtype}

blink1073

LGTM!

blink1073 reviewed Aug 28, 2024

View reviewed changes

bson/vector.py Outdated Show resolved Hide resolved

bson/vector.py Outdated Show resolved Hide resolved

ShaneHarvey requested changes Aug 28, 2024

View reviewed changes

bson/__init__.py Outdated Show resolved Hide resolved

bson/vector.py Outdated Show resolved Hide resolved

blink1073 reviewed Sep 16, 2024

View reviewed changes

bson/binary.py Outdated Show resolved Hide resolved

blink1073 reviewed Sep 16, 2024

View reviewed changes

bson/binary.py Show resolved Hide resolved

blink1073 reviewed Sep 16, 2024

View reviewed changes

bson/binary.py Outdated Show resolved Hide resolved

caseyclements added 15 commits September 17, 2024 13:52

First commit on DRIVERS-2926-BSON-Binary-Vectors

245c869

Turns dtype into enum. Adds handling of padding, __eq__. Removal of n…

031cd8c

…otes.

Added docstring and comments

8d4e8a2

Changed order of BinaryVector and Binary in bson._ENCODERS to get tes…

2df0d6b

…t.test_bson.TestBSON.test_encode_type_marker to pass

Changed order of BinaryVector and Binary in bson._ENCODERS to get tes…

315a115

…t.test_bson.TestBSON.test_encode_type_marker to pass

json_util dumps/loads of BinaryVector

d74314d

Added bson_corpus tests. Needs more, and review of json_util

27f13c8

Removed BinaryVector as separate class. Instead, Binary includes as_v…

263f8c7

…ector, from_vector

Stop setting _USD_C to False

f8bcdef

mypy fixes

5435785

Removed stub vector.json for bson_corpus tests

5c4d152

More tests

f86d040

Added description of subtype 9 to bson.Binary docstring

adcb945

Addressed comments in docstrings.

7986cc5

Eased string comparison of exception in xfail in test_bson

26b8398

caseyclements force-pushed the DRIVERS-2926-BSON-Binary-Vectors branch from e32d1b8 to 26b8398 Compare September 17, 2024 17:52

caseyclements added 4 commits September 17, 2024 15:48

Updates to docstrings of BinaryVector and BinaryVectorDtype

28de28a

Simplified expected exeption case. Will be refactored with yaml anyway..

68235b8

Added draft of test runner

e2a1a3c

Added test cases: padding, and overflow

bf9758a

caseyclements marked this pull request as ready for review September 19, 2024 17:25

caseyclements added 4 commits September 19, 2024 13:36

Merge branch 'master' into DRIVERS-2926-BSON-Binary-Vectors

e1590aa

Cast Path to str

c4c7af7

Simplified as_vector API

de5a245

Added test case: list of floats with dtype int8 raises exception

43bcce4

blink1073 reviewed Sep 30, 2024

View reviewed changes

bson/binary.py Outdated Show resolved Hide resolved

Addition of Provision (BETA) specifiers change references to 4.10

0db9866

blink1073 reviewed Sep 30, 2024

View reviewed changes

doc/changelog.rst Outdated Show resolved Hide resolved

Add references to from_vector() and as_vector()

0532803

ShaneHarvey requested changes Sep 30, 2024

View reviewed changes

Add subtype number in changelog

3edeef6

ShaneHarvey reviewed Sep 30, 2024

View reviewed changes

Raise ValueErrors not AssertionErrors. Bumped from 4.9 to 4.10

d199597

caseyclements requested review from blink1073 and ShaneHarvey September 30, 2024 23:47

caseyclements added 3 commits September 30, 2024 19:50

Docstring for as_vector

abc7cd3

Add slots for BinaryVector

4550c20

Check subtype before decoding

99d44e1

ShaneHarvey requested changes Oct 1, 2024

View reviewed changes

caseyclements added 5 commits September 30, 2024 20:43

Try slots with default padding

001636d

Removed slots arg

637c474

Update dataclass

2d511f6

Remove unompressed kwarg from as_vector

17e1d33

Changed TypeError to ValueError

ce5f3e3

caseyclements and others added 5 commits September 30, 2024 21:25

Updates after removing uncompressed

edfe972

Fixed expected exceptions in invalid test cases

8aaa2f6

Merge branch 'master' into DRIVERS-2926-BSON-Binary-Vectors

dfb322c

padding in now Optional[int] = None

8946daf

padding does need to be an integer

9397129

Removed unneeded ugly TYPE_FROM_HEX = {key.value: key for key in Bina…

913403b

…ryVectorDtype}

blink1073 approved these changes Oct 1, 2024

View reviewed changes

blink1073 merged commit ae6cfd6 into mongodb:master Oct 1, 2024
26 of 27 checks passed

[DRIVERS-2926] [PYTHON-4577] BSON Binary Vector Subtype Support #1813

[DRIVERS-2926] [PYTHON-4577] BSON Binary Vector Subtype Support #1813

Conversation

caseyclements commented Aug 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey commented Oct 1, 2024

blink1073 commented Oct 1, 2024

blink1073 commented Oct 1, 2024

blink1073 left a comment

Choose a reason for hiding this comment

caseyclements commented Aug 24, 2024 •

edited

Loading

ShaneHarvey Oct 1, 2024 •

edited

Loading