ronantakizawa/codereview-bench huggingface dataset:
Repository: activeloopai/Hub
File: deeplake/core/vectorstore/vector_search/utils.py (Python)

Code before:
from deeplake.util.warnings import always_warn

import numpy as np

import random
import string
from typing import Optional, List, Dict, Callable

EXEC_OPTION_TO_RUNTIME: Dict[str, Optional[Dict]] = {
    "compute_engine": None,
    "python": None,
    "tensor_db": {"db_engine": True},
}


def parse_tensor_return(tensor):
    return tensor.data(aslist=True)["value"]


def parse_exec_option(dataset, exec_option):
    if exec_option == "auto":
        if "hub://" in dataset.path:
            if "vector_db/" in dataset.base_storage.path:
                return "tensor_db"
            else:
                return "compute_engine"
        else:
            return "python"
    else:
        return exec_option


def check_indra_installation(exec_option, indra_installed):
    if exec_option == "compute_engine" and not indra_installed:
        raise raise_indra_installation_error(
            indra_import_error=False
        )  # pragma: no cover


def get_runtime_from_exec_option(exec_option):
    return EXEC_OPTION_TO_RUNTIME[exec_option]


def check_length_of_each_tensor(tensors):
    first_item = next(iter(tensors))
    tensor_length = len(tensors[first_item])

    for tensor_name in tensors:
        if len(tensors[f"{tensor_name}"]) != tensor_length:
            tensor_lengths = create_tensor_to_length_str(tensors)


Review comment (suggestion):
Checking for wether dataset is in tensordb definitely works. Path is unlikely to change at the moment so should be good for now, given that `dataset.path` is the s3 one.

Code after:
from deeplake.util.warnings import always_warn

from deeplake.core.dataset import DeepLakeCloudDataset

import numpy as np

import random
import string
from typing import Optional, List, Dict, Callable

EXEC_OPTION_TO_RUNTIME: Dict[str, Optional[Dict]] = {
    "compute_engine": None,
    "python": None,
    "tensor_db": {"db_engine": True},
}


def parse_tensor_return(tensor):
    return tensor.data(aslist=True)["value"]


def parse_exec_option(dataset, exec_option, indra_installed):
    """Select the best available exec_option for the given dataset and environment"""
    if exec_option == "auto":
        if isinstance(dataset, DeepLakeCloudDataset):
            if "vector_db/" in dataset.base_storage.path:
                return "tensor_db"
            elif indra_installed:
                return "compute_engine"
            else:
                return "python"
        else:
            return "python"
    else:
        return exec_option


def parse_return_tensors(dataset, return_tensors, embedding_tensor, return_view):
    """Select the best selection of data and tensors to be returned"""
    if return_view:
        return_tensors = "*"

    if not return_tensors or return_tensors == "*":
        return_tensors = [
            tensor
            for tensor in dataset.tensors
            if (tensor != embedding_tensor or return_tensors == "*")
        ]
    return return_tensors
Repository: activeloopai/Hub
File: deeplake/core/vectorstore/test_deeplake_vectorstore.py (Python)

Code before:
def embedding_fn2(text, embedding_dim=EMBEDDING_DIM):
    return []  # pragma: no cover


def embedding_fn3(text, embedding_dim=EMBEDDING_DIM):
    """Returns embedding in List[np.ndarray] format"""
    return [np.zeros(embedding_dim) for i in range(len(text))]


def embedding_fn4(text, embedding_dim=EMBEDDING_DIM):
    return np.zeros((1, EMBEDDING_DIM))  # pragma: no cover


def embedding_fn5(text, embedding_dim=EMBEDDING_DIM):
    """Returns embedding in List[np.ndarray] format"""
    return [np.zeros(i) for i in range(len(text))]


def test_id_backward_compatibility(local_path):
    num_of_items = 10
    embedding_dim = 100

    ids = [f"{i}" for i in range(num_of_items)]
    embedding = [np.zeros(embedding_dim) for i in range(num_of_items)]
    text = ["aadfv" for i in range(num_of_items)]
    metadata = [{"key": i} for i in range(num_of_items)]

    ds = deeplake.empty(local_path, overwrite=True)
    ds.create_tensor("id", htype="text")
    ds.create_tensor("embedding", htype="embedding")
    ds.create_tensor("text", htype="text")
    ds.create_tensor("metadata", htype="json")

    ds.extend(
        {
            "id": ids,
            "embedding": embedding,
            "text": text,
            "metadata": metadata,
        }
    )

    vectorstore = VectorStore(path=local_path)
    vectorstore.add(
        text=text,
        embedding=embedding,
        metadata=metadata,
    )

    assert len(vectorstore) == 20


Review comment (suggestion):
this test is for backwards compatibility. Here we are checking the old `ids` tensor's support. We can't change this to `id`

Code after:
def embedding_fn2(text, embedding_dim=EMBEDDING_DIM):
    return []  # pragma: no cover


def embedding_fn3(text, embedding_dim=EMBEDDING_DIM):
    """Returns embedding in List[np.ndarray] format"""
    return [np.zeros(embedding_dim) for i in range(len(text))]


def embedding_fn4(text, embedding_dim=EMBEDDING_DIM):
    return np.zeros((1, EMBEDDING_DIM))  # pragma: no cover


def embedding_fn5(text, embedding_dim=EMBEDDING_DIM):
    """Returns embedding in List[np.ndarray] format"""
    return [np.zeros(i) for i in range(len(text))]


def test_id_backward_compatibility(local_path):
    num_of_items = 10
    embedding_dim = 100

    ids = [f"{i}" for i in range(num_of_items)]
    embedding = [np.zeros(embedding_dim) for i in range(num_of_items)]
    text = ["aadfv" for i in range(num_of_items)]
    metadata = [{"key": i} for i in range(num_of_items)]

    ds = deeplake.empty(local_path, overwrite=True)
    ds.create_tensor("ids", htype="text")
    ds.create_tensor("embedding", htype="embedding")
    ds.create_tensor("text", htype="text")
    ds.create_tensor("metadata", htype="json")

    ds.extend(
        {
            "ids": ids,
            "embedding": embedding,
            "text": text,
            "metadata": metadata,
        }
    )

    vectorstore = VectorStore(path=local_path)
    vectorstore.add(
        text=text,
        embedding=embedding,
        metadata=metadata,
    )

    assert len(vectorstore) == 20
Repository: activeloopai/Hub
File: deeplake/core/dataset/deeplake_query_tensor.py (Python)

Code before:


class DeepLakeQueryTensor(tensor.Tensor):
    def __init__(
        self,
        deeplake_tensor,
        indra_tensor,
        index: Optional[Index] = None,
        is_iteration: bool = False,
    ):
        self.deeplake_tensor = deeplake_tensor
        self.indra_tensor = indra_tensor
        self.is_iteration = is_iteration

        self.key = (
            deeplake_tensor.key
            if hasattr(deeplake_tensor, "key")
            else indra_tensor.name
        )

        self.first_dim = None

        self._index = index or Index(self.indra_tensor.index)

    def __getattr__(self, key):
        if key == "chunk_engine":
            raise AttributeError(f"'{self.__class__}' object has no attribute '{key}'")
        try:
            return getattr(self.deeplake_tensor, key)
        except AttributeError:
            try:
                return getattr(self.indra_tensor, key)
            except AttributeError:
                raise AttributeError(
                    f"'{self.__class__}' object has no attribute '{key}'"
                )

    def __getitem__(
        self,
        item,
        is_iteration: bool = False,
    ):
        if not isinstance(item, (int, slice, list, tuple, type(Ellipsis), Index)):
            raise InvalidKeyTypeError(item)

        if isinstance(item, tuple) or item is Ellipsis:
            item = replace_ellipsis_with_slices(item, self.ndim)

        indra_tensor = self.indra_tensor[item]

        return DeepLakeQueryTensor(

Review comment (suggestion):
Will be better to have a `chunk_engine` method that raises a `NotImplementedError`

Code after:


class DeepLakeQueryTensor(tensor.Tensor):
    def __init__(
        self,
        deeplake_tensor,
        indra_tensor,
        index: Optional[Index] = None,
        is_iteration: bool = False,
    ):
        self.deeplake_tensor = deeplake_tensor
        self.indra_tensor = indra_tensor
        self.is_iteration = is_iteration

        self.key = (
            deeplake_tensor.key
            if hasattr(deeplake_tensor, "key")
            else indra_tensor.name
        )

        self.first_dim = None

        self._index = index or Index(self.indra_tensor.index)

    def __getattr__(self, key):
        try:
            return getattr(self.deeplake_tensor, key)
        except AttributeError:
            try:
                return getattr(self.indra_tensor, key)
            except AttributeError:
                raise AttributeError(
                    f"'{self.__class__}' object has no attribute '{key}'"
                )

    def __getitem__(
        self,
        item,
        is_iteration: bool = False,
    ):
        if not isinstance(item, (int, slice, list, tuple, type(Ellipsis), Index)):
            raise InvalidKeyTypeError(item)

        if isinstance(item, tuple) or item is Ellipsis:
            item = replace_ellipsis_with_slices(item, self.ndim)

        indra_tensor = self.indra_tensor[item]

        return DeepLakeQueryTensor(
            self.deeplake_tensor,
            indra_tensor,
Repository: activeloopai/Hub
File: deeplake/core/vectorstore/vector_search/indra/search_algorithm.py (Python)

Code before:
    except ImportError:
        INDRA_INSTALLED = False
        pass

    if tql_string:
        tql_query = tql_string
    else:
        tql_query = query.parse_query(
            distance_metric,
            k,
            query_embedding,
            embedding_tensor,
            tql_filter,
            return_tensors,
        )

    if runtime:
        view, data = deeplake_dataset.query(
            tql_query, runtime=runtime, return_data=True
        )
        return_data = data
    elif deep_memory:
        if not INDRA_INSTALLED:
            raise raise_indra_installation_error(indra_import_error=None)

        from indra import api

        indra_dataset = api.dataset(deeplake_dataset.path, token=token, org_id=org_id)
        api.tql.prepare_deepmemory_metrics(indra_dataset)

        indra_view = indra_dataset.query(tql_query)
        indexes = indra_view.indexes
        view = deeplake_dataset[indexes]

        return_data = {}

        for tensor in view.tensors:
            return_data[tensor] = utils.parse_tensor_return(view[tensor])

    else:
        if not INDRA_INSTALLED:
            raise raise_indra_installation_error(
                indra_import_error=None
            )  # pragma: no cover
        return_data = {}

        view = deeplake_dataset.query(
            tql_query,
            runtime=runtime,
        )


Review comment (suggestion):
use instead following logic

```
    from deeplake.enterprise.convert_to_libdeeplake import import_indra_api

   api =  import_indra_api()
```

Code after:
    except ImportError:
        INDRA_INSTALLED = False
        pass

    if tql_string:
        tql_query = tql_string
    else:
        tql_query = query.parse_query(
            distance_metric,
            k,
            query_embedding,
            embedding_tensor,
            tql_filter,
            return_tensors,
        )

    if runtime:
        view, data = deeplake_dataset.query(
            tql_query, runtime=runtime, return_data=True
        )
        return_data = data
    elif deep_memory:
        if not INDRA_INSTALLED:
            raise raise_indra_installation_error(indra_import_error=None)

        from deeplake.enterprise.convert_to_libdeeplake import import_indra_api

        api = import_indra_api()

        indra_dataset = api.dataset(deeplake_dataset.path, token=token, org_id=org_id)
        api.tql.prepare_deepmemory_metrics(indra_dataset)

        indra_view = indra_dataset.query(tql_query)
        indexes = indra_view.indexes
        view = deeplake_dataset[indexes]

        return_data = {}

        for tensor in view.tensors:
            return_data[tensor] = utils.parse_tensor_return(view[tensor])

    else:
        if not INDRA_INSTALLED:
            raise raise_indra_installation_error(
                indra_import_error=None
            )  # pragma: no cover
        return_data = {}

        view = deeplake_dataset.query(
            tql_query,
            runtime=runtime,
Repository: activeloopai/Hub
File: deeplake/api/tests/test_pickle.py (Python)

Code before:
            ds.label.append(i)

    pickled_ds = pickle.dumps(ds)
    unpickled_ds = pickle.loads(pickled_ds)
    assert len(unpickled_ds.image) == len(ds.image)
    assert len(unpickled_ds.label) == len(ds.label)
    assert unpickled_ds.tensors.keys() == ds.tensors.keys()
    assert unpickled_ds.index.values[0].value == ds.index.values[0].value
    assert unpickled_ds.meta.version == ds.meta.version

    for i in range(10):
        np.testing.assert_array_equal(
            ds.image[i].numpy(),
            (i * np.ones(((i + 1) * 20, (i + 1) * 20, 3), dtype=np.uint8)),
        )
        np.testing.assert_array_equal(
            ds.image[i].numpy(), unpickled_ds.image[i].numpy()
        )
    for i in range(5):
        np.testing.assert_array_equal(ds.label[i].numpy(), i)
        np.testing.assert_array_equal(
            ds.label[i].numpy(), unpickled_ds.label[i].numpy()
        )


def test_no_chunks_present(local_ds):
    with local_ds as ds:
        ds.create_tensor("abc")
        ds.abc.append([1, 2, 3, 4, 5])

    size = len(pickle.dumps(ds))

    ds.abc.append([1, 2, 3, 4, 5])

    assert len(pickle.dumps(ds)) == size


Review comment (bug):
```suggestion
def test_pickling_dataset_should_not_store_cached_chunks(local_ds):
```

Renaming this test to `test_pickling_dataset_should_not_store_cached_chunks`, will make test name more clear so that if it fails, developer knows from the traceback what happened

Code after:
            ds.label.append(i)

    pickled_ds = pickle.dumps(ds)
    unpickled_ds = pickle.loads(pickled_ds)
    assert len(unpickled_ds.image) == len(ds.image)
    assert len(unpickled_ds.label) == len(ds.label)
    assert unpickled_ds.tensors.keys() == ds.tensors.keys()
    assert unpickled_ds.index.values[0].value == ds.index.values[0].value
    assert unpickled_ds.meta.version == ds.meta.version

    for i in range(10):
        np.testing.assert_array_equal(
            ds.image[i].numpy(),
            (i * np.ones(((i + 1) * 20, (i + 1) * 20, 3), dtype=np.uint8)),
        )
        np.testing.assert_array_equal(
            ds.image[i].numpy(), unpickled_ds.image[i].numpy()
        )
    for i in range(5):
        np.testing.assert_array_equal(ds.label[i].numpy(), i)
        np.testing.assert_array_equal(
            ds.label[i].numpy(), unpickled_ds.label[i].numpy()
        )


def test_pickled_dataset_does_not_store_cached_chunks(local_ds):
    with local_ds as ds:
        ds.create_tensor("abc")
        ds.abc.append([1, 2, 3, 4, 5])

    size = len(pickle.dumps(ds))

    ds.abc.append([1, 2, 3, 4, 5])

    assert (
        len(pickle.dumps(ds)) == size
    ), "Adding element should not change size of pickle"
Repository: activeloopai/Hub
File: deeplake/api/tests/test_pickle.py (Python)

Code before:

    for i in range(10):
        np.testing.assert_array_equal(
            ds.image[i].numpy(),
            (i * np.ones(((i + 1) * 20, (i + 1) * 20, 3), dtype=np.uint8)),
        )
        np.testing.assert_array_equal(
            ds.image[i].numpy(), unpickled_ds.image[i].numpy()
        )
    for i in range(5):
        np.testing.assert_array_equal(ds.label[i].numpy(), i)
        np.testing.assert_array_equal(
            ds.label[i].numpy(), unpickled_ds.label[i].numpy()
        )


def test_no_chunks_present(local_ds):
    with local_ds as ds:
        ds.create_tensor("abc")
        ds.abc.append([1, 2, 3, 4, 5])

    size = len(pickle.dumps(ds))

    ds.abc.append([1, 2, 3, 4, 5])

    assert len(pickle.dumps(ds)) == size


Review comment (bug):
```suggestion
    assert len(pickle.dumps(ds)) == size, 'After adding element to the pickled dataset, its size has changed while it shouldn't, it could be storing the cached chunks'
```

This will also help developers understand what went wrong more easily

Code after:

    for i in range(10):
        np.testing.assert_array_equal(
            ds.image[i].numpy(),
            (i * np.ones(((i + 1) * 20, (i + 1) * 20, 3), dtype=np.uint8)),
        )
        np.testing.assert_array_equal(
            ds.image[i].numpy(), unpickled_ds.image[i].numpy()
        )
    for i in range(5):
        np.testing.assert_array_equal(ds.label[i].numpy(), i)
        np.testing.assert_array_equal(
            ds.label[i].numpy(), unpickled_ds.label[i].numpy()
        )


def test_pickled_dataset_does_not_store_cached_chunks(local_ds):
    with local_ds as ds:
        ds.create_tensor("abc")
        ds.abc.append([1, 2, 3, 4, 5])

    size = len(pickle.dumps(ds))

    ds.abc.append([1, 2, 3, 4, 5])

    assert (
        len(pickle.dumps(ds)) == size
    ), "Adding element should not change size of pickle"
Repository: activeloopai/Hub
File: deeplake/core/tests/test_query.py (Python)

Code before:
    assert ds.images.meta.htype == "image"


@pytest.mark.slow
@requires_libdeeplake
def test_multi_source_query(hub_cloud_dev_token):
    with pytest.raises(RuntimeError):
        ds = deeplake.query(
            'SELECT * FROM "hub://activeloop/mnist-train" UNION (SELECT * FROM "hub://activeloop/coco-train")',
            token=hub_cloud_dev_token,
        )

    ds = deeplake.query(
        'SELECT * FROM "hub://activeloop/mnist-train" UNION (SELECT images, categories[0] as labels FROM "hub://activeloop/coco-train")',
        token=hub_cloud_dev_token,
    )
    assert len(ds) == 178287
    assert len(ds.tensors) == 2
    assert ds.images.meta.htype == "image"
    assert ds.labels.meta.htype == "class_label"

    ds = deeplake.query(
        'SELECT * FROM (SELECT * FROM "hub://activeloop/mnist-train" UNION (SELECT images, labels FROM "hub://activeloop/cifar100-train")) WHERE labels == 0',
        token=hub_cloud_dev_token,
    )

    assert len(ds) == 5923
    assert len(ds.tensors) == 2
    d = ds.labels.numpy()
    assert np.all(d == 0)


Review comment (suggestion):
I get why the result of this test was changed. Can you please add the following test too?
```
ds = deeplake.query(
    'SELECT * FROM "hub://activeloop/mnist-train" WHERE labels == 0 UNION (SELECT images, labels FROM "hub://activeloop/cifar100-train" WHERE labels == 0)',
        token=hub_cloud_dev_token,
    )
assert len(ds) == 6423
```

Code after:
    assert ds.images.meta.htype == "image"


@pytest.mark.slow
@requires_libdeeplake
def test_multi_source_query(hub_cloud_dev_token):
    with pytest.raises(RuntimeError):
        ds = deeplake.query(
            'SELECT * FROM "hub://activeloop/mnist-train" UNION (SELECT * FROM "hub://activeloop/coco-train")',
            token=hub_cloud_dev_token,
        )

    ds = deeplake.query(
        'SELECT * FROM "hub://activeloop/mnist-train" UNION (SELECT images, categories[0] as labels FROM "hub://activeloop/coco-train")',
        token=hub_cloud_dev_token,
    )
    assert len(ds) == 178287
    assert len(ds.tensors) == 2
    assert ds.images.meta.htype == "image"
    assert ds.labels.meta.htype == "class_label"

    ds = deeplake.query(
        'SELECT * FROM (SELECT * FROM "hub://activeloop/mnist-train" UNION (SELECT images, labels FROM "hub://activeloop/cifar100-train")) WHERE labels == 0',
        token=hub_cloud_dev_token,
    )

    assert len(ds) == 5923
    assert len(ds.tensors) == 2
    d = ds.labels.numpy()
    assert np.all(d == 0)

    ds = deeplake.query(
        'SELECT * FROM "hub://activeloop/mnist-train" WHERE labels == 0 UNION (SELECT images, labels FROM "hub://activeloop/cifar100-train" WHERE labels == 0)',
        token=hub_cloud_dev_token,
    )
    assert len(ds) == 6423
Repository: actix/actix-web
File: src/ws/frame.rs (Rust)

Code before:
use body::Binary;
use error::PayloadError;
use payload::PayloadHelper;

use ws::ProtocolError;
use ws::mask::apply_mask;
use ws::proto::{CloseCode, CloseReason, OpCode};

/// A struct representing a `WebSocket` frame.
#[derive(Debug)]
pub struct Frame {
    finished: bool,
    opcode: OpCode,
    payload: Binary,
}

impl Frame {
    /// Destruct frame
    pub fn unpack(self) -> (bool, OpCode, Binary) {
        (self.finished, self.opcode, self.payload)
    }

    /// Create a new Close control frame.
    #[inline]
    pub fn close(reason: Option<CloseReason>, genmask: bool) -> Binary {
	    let payload = match reason {
		    None => Vec::new(),
		    Some(reason) => {
			    let mut code_bytes = [0; 2];
			    NetworkEndian::write_u16(&mut code_bytes, reason.code.into());

			    let mut payload = Vec::from(&code_bytes[..]);
			    if let Some(description) = reason.description{
				    payload.extend(description.as_bytes());
			    }
			    payload
		    }
	    };

	    Frame::message(payload, OpCode::Close, true, genmask)
    }

    #[cfg_attr(feature = "cargo-clippy", allow(type_complexity))]
    fn read_copy_md<S>(
        pl: &mut PayloadHelper<S>, server: bool, max_size: usize
    ) -> Poll<Option<(usize, bool, OpCode, usize, Option<u32>)>, ProtocolError>
    where
        S: Stream<Item = Bytes, Error = PayloadError>,
    {
        let mut idx = 2;
        let buf = match pl.copy(2)? {

Review comment (suggestion):
Some indentations seem odd, mixing space and tab I guess? It would be better to use the same style as the rest of the codebase.

Code after:
use body::Binary;
use error::PayloadError;
use payload::PayloadHelper;

use ws::ProtocolError;
use ws::mask::apply_mask;
use ws::proto::{CloseCode, CloseReason, OpCode};

/// A struct representing a `WebSocket` frame.
#[derive(Debug)]
pub struct Frame {
    finished: bool,
    opcode: OpCode,
    payload: Binary,
}

impl Frame {
    /// Destruct frame
    pub fn unpack(self) -> (bool, OpCode, Binary) {
        (self.finished, self.opcode, self.payload)
    }

    /// Create a new Close control frame.
    #[inline]
    pub fn close(reason: Option<CloseReason>, genmask: bool) -> Binary {
        let payload = match reason {
            None => Vec::new(),
            Some(reason) => {
                let mut code_bytes = [0; 2];
                NetworkEndian::write_u16(&mut code_bytes, reason.code.into());

                let mut payload = Vec::from(&code_bytes[..]);
                if let Some(description) = reason.description{
                    payload.extend(description.as_bytes());
                }
                payload
            }
        };

        Frame::message(payload, OpCode::Close, true, genmask)
    }

    #[cfg_attr(feature = "cargo-clippy", allow(type_complexity))]
    fn read_copy_md<S>(
        pl: &mut PayloadHelper<S>, server: bool, max_size: usize
    ) -> Poll<Option<(usize, bool, OpCode, usize, Option<u32>)>, ProtocolError>
    where
        S: Stream<Item = Bytes, Error = PayloadError>,
    {
        let mut idx = 2;
        let buf = match pl.copy(2)? {