ronantakizawa/codereview-bench huggingface dataset:
Repository: huggingface/datasets
File: src/datasets/packaged_modules/__init__.py (Python)

Code before:
import inspect
import re
from hashlib import sha256
from typing import List

from .csv import csv
from .json import json
from .pandas import pandas
from .text import text


def hash_python_lines(lines: List[str]) -> str:
    filtered_lines = []
    for line in lines:
        line = line.replace("\n", "")  # remove line breaks, white space and comments
        line = line.replace(" ", "")
        line = line.replace("\t", "")
        line = re.sub(r"#.*", "", line)
        if line:
            filtered_lines.append(line)
    full_str = "\n".join(filtered_lines)

    # Make a hash from all this code
    full_bytes = full_str.encode("utf-8")
    return sha256(full_bytes).hexdigest()


# get importable module names and hash for caching
_PACKAGED_DATASETS_MODULES = {
    "csv": (csv.__name__, hash_python_lines(inspect.getsource(csv).splitlines())),
    "json": (json.__name__, hash_python_lines(inspect.getsource(json).splitlines())),
    "pandas": (pandas.__name__, hash_python_lines(inspect.getsource(pandas).splitlines())),
    "text": (text.__name__, hash_python_lines(inspect.getsource(text).splitlines())),
}


Review comment (performance):
I would just remove this replacements.
They could create some errors (return the same hash while the code is different). And since it would change the hashes, it means that all the cache directories that have been created until now would now be ignored. 

```suggestion
```

Code after:
import inspect
import re
from hashlib import sha256
from typing import List

from .csv import csv
from .json import json
from .pandas import pandas
from .parquet import parquet
from .text import text


def hash_python_lines(lines: List[str]) -> str:
    filtered_lines = []
    for line in lines:
        line = re.sub(r"#.*", "", line)  # remove comments
        if line:
            filtered_lines.append(line)
    full_str = "\n".join(filtered_lines)

    # Make a hash from all this code
    full_bytes = full_str.encode("utf-8")
    return sha256(full_bytes).hexdigest()


# get importable module names and hash for caching
_PACKAGED_DATASETS_MODULES = {
    "csv": (csv.__name__, hash_python_lines(inspect.getsource(csv).splitlines())),
    "json": (json.__name__, hash_python_lines(inspect.getsource(json).splitlines())),
    "pandas": (pandas.__name__, hash_python_lines(inspect.getsource(pandas).splitlines())),
    "parquet": (parquet.__name__, hash_python_lines(inspect.getsource(parquet).splitlines())),
    "text": (text.__name__, hash_python_lines(inspect.getsource(text).splitlines())),
}
Repository: huggingface/datasets
File: tests/test_file_utils.py (Python)

Code before:
import zstandard as zstd

from datasets.download.download_config import DownloadConfig
from datasets.utils.file_utils import (
    OfflineModeIsEnabled,
    cached_path,
    fsspec_get,
    fsspec_head,
    ftp_get,
    ftp_head,
    get_from_cache,
    http_get,
    http_head,
)


FILE_CONTENT = """\
    Text data.
    Second line of data."""

FILE_PATH = "file"


@pytest.fixture(scope="session")
def zstd_path(tmp_path_factory):
    path = tmp_path_factory.mktemp("data") / FILE_PATH
    data = bytes(FILE_CONTENT, "utf-8")
    with zstd.open(path, "wb") as f:
        f.write(data)
    return path


@pytest.fixture
def mockfs_file(mockfs):
    with open(os.path.join(mockfs.local_root_dir, FILE_PATH), "w") as f:
        f.write(FILE_CONTENT)
    return mockfs


@pytest.mark.parametrize("compression_format", ["gzip", "xz", "zstd"])
def test_cached_path_extract(compression_format, gz_file, xz_file, zstd_path, tmp_path, text_file):
    input_paths = {"gzip": gz_file, "xz": xz_file, "zstd": zstd_path}
    input_path = input_paths[compression_format]
    cache_dir = tmp_path / "cache"
    download_config = DownloadConfig(cache_dir=cache_dir, extract_compressed_file=True)
    extracted_path = cached_path(input_path, download_config=download_config)
    with open(extracted_path) as f:
        extracted_file_content = f.read()
    with open(text_file) as f:
        expected_file_content = f.read()
    assert extracted_file_content == expected_file_content

Review comment (suggestion):
```suggestion
    path = tmp_path_factory.mktemp("data") / (FILE_PATH + ".zstd")
```

Code after:
import zstandard as zstd

from datasets.download.download_config import DownloadConfig
from datasets.utils.file_utils import (
    OfflineModeIsEnabled,
    cached_path,
    fsspec_get,
    fsspec_head,
    ftp_get,
    ftp_head,
    get_from_cache,
    http_get,
    http_head,
)


FILE_CONTENT = """\
    Text data.
    Second line of data."""

FILE_PATH = "file"


@pytest.fixture(scope="session")
def zstd_path(tmp_path_factory):
    path = tmp_path_factory.mktemp("data") / (FILE_PATH + ".zstd")
    data = bytes(FILE_CONTENT, "utf-8")
    with zstd.open(path, "wb") as f:
        f.write(data)
    return path


@pytest.fixture
def tmpfs_file(tmpfs):
    with open(os.path.join(tmpfs.local_root_dir, FILE_PATH), "w") as f:
        f.write(FILE_CONTENT)
    return FILE_PATH


@pytest.mark.parametrize("compression_format", ["gzip", "xz", "zstd"])
def test_cached_path_extract(compression_format, gz_file, xz_file, zstd_path, tmp_path, text_file):
    input_paths = {"gzip": gz_file, "xz": xz_file, "zstd": zstd_path}
    input_path = input_paths[compression_format]
    cache_dir = tmp_path / "cache"
    download_config = DownloadConfig(cache_dir=cache_dir, extract_compressed_file=True)
    extracted_path = cached_path(input_path, download_config=download_config)
    with open(extracted_path) as f:
        extracted_file_content = f.read()
    with open(text_file) as f:
        expected_file_content = f.read()
    assert extracted_file_content == expected_file_content
Repository: huggingface/datasets
File: src/datasets/io/spark.py (Python)

Code before:
from typing import Optional

import pyspark

from .. import Features, NamedSplit
from ..download import DownloadMode
from ..packaged_modules.spark.spark import Spark
from .abc import AbstractDatasetReader


class SparkDatasetReader(AbstractDatasetReader):
    def __init__(
        self,
        df: pyspark.sql.DataFrame,
        split: Optional[NamedSplit] = None,
        features: Optional[Features] = None,
        cache_dir: str = None,
        force_download: bool = False,
        **kwargs,
    ):
        super().__init__(
            path_or_paths="unused",
            split=split,
            features=features,
            cache_dir=cache_dir,
            **kwargs,
        )
        self._force_download = force_download
        self.builder = Spark(
            df=df,
            features=features,
            cache_dir=cache_dir,
            **kwargs,
        )

    def read(self):
        download_mode = DownloadMode.FORCE_REDOWNLOAD if self._force_download else None
        self.builder.download_and_prepare(download_mode=download_mode)
        # TODO: Support as_streaming_dataset.
        return self.builder.as_dataset(split=self.split)


Review comment (refactor):
I guess you can modify `AbstractDatasetReader`'s typing to allow `None` here
```suggestion
            path_or_paths=None,
```

Code after:
from typing import Optional

import pyspark

from .. import Features, NamedSplit
from ..download import DownloadMode
from ..packaged_modules.spark.spark import Spark
from .abc import AbstractDatasetReader


class SparkDatasetReader(AbstractDatasetReader):
    """A dataset reader that reads from a Spark DataFrame.

    When caching, cache materialization is parallelized over Spark; an NFS that is accessible to the driver must be
    provided. Streaming is not currently supported.
    """

    def __init__(
        self,
        df: pyspark.sql.DataFrame,
        split: Optional[NamedSplit] = None,
        features: Optional[Features] = None,
        cache_dir: str = None,
        load_from_cache_file: bool = True,
        file_format: str = "arrow",
        **kwargs,
    ):
        super().__init__(
            split=split,
            features=features,
            cache_dir=cache_dir,
            **kwargs,
        )
        self._load_from_cache_file = load_from_cache_file
        self._file_format = file_format
        self.builder = Spark(
            df=df,
            features=features,
            cache_dir=cache_dir,
            **kwargs,
        )

    def read(self):
        if self.streaming:
            # TODO: Support as_streaming_dataset.
            raise ValueError("SparkDatasetReader is not streamable.")
        download_mode = None if self._load_from_cache_file else DownloadMode.FORCE_REDOWNLOAD
Repository: huggingface/datasets
File: utils/release.py (Python)

Code before:
    if len(version) == 0:
        version = default_version

    print(f"Updating version to {version}.")
    global_version_update(version)


def post_release_work():
    """Do all the necesarry post-release steps."""
    # First let's get the current version
    current_version = get_version()
    dev_version = f"{current_version.major}.{current_version.minor + 1}.0.dev0"
    current_version = current_version.base_version

    # Check with the user we got that right.
    version = input(f"Which version are we developing now? [{dev_version}]")
    if len(version) == 0:
        version = dev_version

    print(f"Updating version to {version}.")
    global_version_update(version)


if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument("--post_release", action="store_true", help="Whether this is pre or post release.")
    parser.add_argument("--patch", action="store_true", help="Whether or not this is a patch release.")
    args = parser.parse_args()
    if not args.post_release:
        pre_release_work(patch=args.patch)
    elif args.patch:
        print("Nothing to do after a patch :-)")
    else:
        post_release_work()


Review comment (suggestion):
```suggestion
    parser.add_argument("--post_release", action="store_true", help="Whether or not this is post release.")
```

Code after:
    if len(version) == 0:
        version = default_version

    print(f"Updating version to {version}.")
    global_version_update(version)


def post_release_work():
    """Do all the necesarry post-release steps."""
    # First let's get the current version
    current_version = get_version()
    dev_version = f"{current_version.major}.{current_version.minor + 1}.0.dev0"
    current_version = current_version.base_version

    # Check with the user we got that right.
    version = input(f"Which version are we developing now? [{dev_version}]")
    if len(version) == 0:
        version = dev_version

    print(f"Updating version to {version}.")
    global_version_update(version)


if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument("--post_release", action="store_true", help="Whether or not this is post release.")
    parser.add_argument("--patch", action="store_true", help="Whether or not this is a patch release.")
    args = parser.parse_args()
    if not args.post_release:
        pre_release_work(patch=args.patch)
    elif args.patch:
        print("Nothing to do after a patch :-)")
    else:
        post_release_work()
Repository: huggingface/datasets
File: tests/fixtures/hub.py (Python)

Code before:
    HfFolder.delete_token()


@pytest.fixture(scope="session")
def hf_api():
    return HfApi(endpoint=CI_HUB_ENDPOINT)


@pytest.fixture(scope="session")
def hf_token():
    yield CI_HUB_USER_TOKEN


@pytest.fixture
def cleanup_repo(hf_api):
    def _cleanup_repo(repo_id):
        hf_api.delete_repo(repo_id, token=CI_HUB_USER_TOKEN, repo_type="dataset")

    return _cleanup_repo


@pytest.fixture
def temporary_repo(cleanup_repo):
    @contextmanager
    def _temporary_repo(repo_id: Optional[str] = None):
        repo_id = repo_id or f"{CI_HUB_USER}/test-{int(time.time() * 10e6)}"
        try:
            yield repo_id
        finally:
            try:
                cleanup_repo(repo_id)
            except RepositoryNotFoundError:
                pass

    return _temporary_repo


@pytest.fixture(scope="session")
def hf_private_dataset_repo_txt_data_(hf_api: HfApi, hf_token, text_file):
    repo_name = f"repo_txt_data-{int(time.time() * 10e6)}"
    repo_id = f"{CI_HUB_USER}/{repo_name}"
    hf_api.create_repo(repo_id, token=hf_token, repo_type="dataset", private=True)
    hf_api.upload_file(
        token=hf_token,
        path_or_fileobj=str(text_file),
        path_in_repo="data/text_data.txt",
        repo_id=repo_id,
        repo_type="dataset",
    )
    yield repo_id
    try:

Review comment (suggestion):
Maybe instead of modifying the precision, we can use something like [this](https://github.com/huggingface/huggingface_hub/blob/89cc69105074f1d071e0471144605f3cdfe1dab3/tests/testing_utils.py#L54) to be aligned with `huggingface_hub`

Code after:
    yield
    HfFolder.delete_token()


@pytest.fixture(scope="session")
def hf_api():
    return HfApi(endpoint=CI_HUB_ENDPOINT)


@pytest.fixture(scope="session")
def hf_token():
    yield CI_HUB_USER_TOKEN


@pytest.fixture
def cleanup_repo(hf_api):
    def _cleanup_repo(repo_id):
        hf_api.delete_repo(repo_id, token=CI_HUB_USER_TOKEN, repo_type="dataset")

    return _cleanup_repo


@pytest.fixture
def temporary_repo(cleanup_repo):
    @contextmanager
    def _temporary_repo(repo_id: Optional[str] = None):
        repo_id = repo_id or f"{CI_HUB_USER}/test-dataset-{uuid.uuid4().hex[:6]}-{int(time.time() * 10e3)}"
        try:
            yield repo_id
        finally:
            try:
                cleanup_repo(repo_id)
            except RepositoryNotFoundError:
                pass

    return _temporary_repo


@pytest.fixture(scope="session")
def hf_private_dataset_repo_txt_data_(hf_api: HfApi, hf_token, text_file):
    repo_name = f"repo_txt_data-{int(time.time() * 10e6)}"
    repo_id = f"{CI_HUB_USER}/{repo_name}"
    hf_api.create_repo(repo_id, token=hf_token, repo_type="dataset", private=True)
    hf_api.upload_file(
        token=hf_token,
        path_or_fileobj=str(text_file),
        path_in_repo="data/text_data.txt",
        repo_id=repo_id,
        repo_type="dataset",
    )
    yield repo_id
Repository: huggingface/datasets
File: src/datasets/packaged_modules/generator/generator.py (Python)

Code before:
    generator: Optional[Callable] = None
    gen_kwargs: Optional[dict] = None
    features: Optional[datasets.Features] = None

    def __post_init__(self):
        super().__post_init__()
        if self.generator is None:
            raise ValueError("generator must be specified")

        if self.gen_kwargs is None:
            self.gen_kwargs = {}


class Generator(datasets.GeneratorBasedBuilder):
    BUILDER_CONFIG_CLASS = GeneratorConfig

    def __init__(
        self,
        split: Optional[datasets.NamedSplit] = None,
        **kwargs,
    ):
        self.split = split if split is not None else datasets.Split.TRAIN
        return super().__init__(
            **kwargs,
        )

    def _info(self):
        return datasets.DatasetInfo(features=self.config.features)

    def _split_generators(self, dl_manager):
        return [datasets.SplitGenerator(name=self.split, gen_kwargs=self.config.gen_kwargs)]

    def _generate_examples(self, **gen_kwargs):
        for idx, ex in enumerate(self.config.generator(**gen_kwargs)):
            yield idx, ex


Review comment (suggestion):
Do not add the `split` parameter as an attribute of `Generator` class, but as an attribute of `GeneratorConfig` instead.

Code after:
    generator: Optional[Callable] = None
    gen_kwargs: Optional[dict] = None
    features: Optional[datasets.Features] = None
    split: datasets.NamedSplit = datasets.Split.TRAIN

    def __post_init__(self):
        super().__post_init__()
        if self.generator is None:
            raise ValueError("generator must be specified")

        if self.gen_kwargs is None:
            self.gen_kwargs = {}


class Generator(datasets.GeneratorBasedBuilder):
    BUILDER_CONFIG_CLASS = GeneratorConfig

    def _info(self):
        return datasets.DatasetInfo(features=self.config.features)

    def _split_generators(self, dl_manager):
        return [datasets.SplitGenerator(name=self.config.split, gen_kwargs=self.config.gen_kwargs)]

    def _generate_examples(self, **gen_kwargs):
        for idx, ex in enumerate(self.config.generator(**gen_kwargs)):
            yield idx, ex
Repository: huggingface/datasets
File: tests/test_arrow_dataset.py (Python)

Code before:
def test_from_polars_save_to_disk_and_load_from_disk_round_trip_with_large_list(tmp_path):
    import polars as pl

    df = pl.from_dict({"col_1": [[1, 2], [3, 4]]})
    ds = Dataset.from_polars(df)
    dataset_path = tmp_path / "dataset_dir"
    ds.save_to_disk(dataset_path)
    assert (dataset_path / "data-00000-of-00001.arrow").exists()
    loaded_ds = load_from_disk(dataset_path)
    assert len(loaded_ds) == len(ds)
    assert loaded_ds.features == ds.features
    assert loaded_ds.to_dict() == ds.to_dict()


@require_polars
def test_polars_round_trip():
    ds = Dataset.from_dict({"x": [[1, 2], [3, 4, 5]], "y": ["a", "b"]})
    assert isinstance(Dataset.from_polars(ds.to_polars()), Dataset)
    
def test_add_column_and_item_without_fingerprint():
    from datasets import Dataset
    ds = Dataset.from_dict({"a": [1, 2]})

    # add column
    ds2 = ds.add_column("b", [3, 4])
    assert "b"


Review comment (suggestion):
```suggestion
    
def test_add_column():
    from datasets import Dataset
    ds = Dataset.from_dict({"a": [1, 2]})
    ds = ds.add_column("b", [3, 4])
    assert "b" in ds.features
    assert ds[0] == {"a": 1, "b": 3}
    assert ds[1] == {"a": 2, "b": 4}
```

Code after:
def test_from_polars_save_to_disk_and_load_from_disk_round_trip_with_large_list(tmp_path):
    import polars as pl

    df = pl.from_dict({"col_1": [[1, 2], [3, 4]]})
    ds = Dataset.from_polars(df)
    dataset_path = tmp_path / "dataset_dir"
    ds.save_to_disk(dataset_path)
    assert (dataset_path / "data-00000-of-00001.arrow").exists()
    loaded_ds = load_from_disk(dataset_path)
    assert len(loaded_ds) == len(ds)
    assert loaded_ds.features == ds.features
    assert loaded_ds.to_dict() == ds.to_dict()


@require_polars
def test_polars_round_trip():
    ds = Dataset.from_dict({"x": [[1, 2], [3, 4, 5]], "y": ["a", "b"]})
    assert isinstance(Dataset.from_polars(ds.to_polars()), Dataset)


def test_add_column():
    from datasets import Dataset

    ds = Dataset.from_dict({"a": [1, 2]})
    ds = ds.add_column("b", [3, 4])
    assert "b" in ds.features
    assert ds[0] == {"a": 1, "b": 3}
    assert ds[1] == {"a": 2, "b": 4}
Repository: huggingface/diffusers
File: tests/pipelines/text_to_video_synthesis/test_text_to_video_zero.py (Python)

Code before:
    torch_device,
)

from ..test_pipelines_common import assert_mean_pixel_difference


@nightly
@require_torch_accelerator
class TextToVideoZeroPipelineSlowTests(unittest.TestCase):
    def setUp(self):
        # clean up the VRAM before each test
        super().setUp()
        gc.collect()
        backend_empty_cache(torch_device)

    def tearDown(self):
        # clean up the VRAM after each test
        super().tearDown()
        gc.collect()
        backend_empty_cache(torch_device)

    def test_full_model(self):
        model_id = "stable-diffusion-v1-5/stable-diffusion-v1-5"
        pipe = TextToVideoZeroPipeline.from_pretrained(model_id, torch_dtype=torch.float16).to(torch_device)
        pipe.scheduler = DDIMScheduler.from_config(pipe.scheduler.config)
        generator = torch.Generator(device="cpu").manual_seed(0)

        prompt = "A bear is playing a guitar on Times Square"
        result = pipe(prompt=prompt, generator=generator).images

        expected_result = load_pt(
            "https://huggingface.co/datasets/hf-internal-testing/diffusers-images/resolve/main/text-to-video/A bear is playing a guitar on Times Square.pt"
        )

        assert_mean_pixel_difference(result, expected_result)


Review comment (suggestion):
PRNG will behave different across accelerators, set back to "cpu" for cross-device reproducibility, as other cases in diffusers

Code after:
    torch_device,
)

from ..test_pipelines_common import assert_mean_pixel_difference


@nightly
@require_torch_accelerator
class TextToVideoZeroPipelineSlowTests(unittest.TestCase):
    def setUp(self):
        # clean up the VRAM before each test
        super().setUp()
        gc.collect()
        backend_empty_cache(torch_device)

    def tearDown(self):
        # clean up the VRAM after each test
        super().tearDown()
        gc.collect()
        backend_empty_cache(torch_device)

    def test_full_model(self):
        model_id = "stable-diffusion-v1-5/stable-diffusion-v1-5"
        pipe = TextToVideoZeroPipeline.from_pretrained(model_id, torch_dtype=torch.float16).to(torch_device)
        pipe.scheduler = DDIMScheduler.from_config(pipe.scheduler.config)
        generator = torch.Generator(device="cpu").manual_seed(0)

        prompt = "A bear is playing a guitar on Times Square"
        result = pipe(prompt=prompt, generator=generator).images

        expected_result = load_pt(
            "https://huggingface.co/datasets/hf-internal-testing/diffusers-images/resolve/main/text-to-video/A bear is playing a guitar on Times Square.pt",
            weights_only=False,
        )

        assert_mean_pixel_difference(result, expected_result)
Repository: huggingface/diffusers
File: src/diffusers/utils/peft_utils.py (Python)

Code before:
                    f"Loading adapter weights from state_dict led to missing keys in the model:"
                    f" {', '.join(lora_missing_keys)}."
                )

    if warn_msg:
        logger.warning(warn_msg)


def _derive_exclude_modules(model_state_dict, peft_state_dict, adapter_name=None):
    """
    Derives the modules to exclude while initializing `LoraConfig` through `exclude_modules`. It works by comparing the
    `model_state_dict` and `peft_state_dict` and adds a module from `model_state_dict` to the exclusion set if it
    doesn't exist in `peft_state_dict`.
    """
    if model_state_dict is None:
        return
    all_modules = set()
    string_to_replace = f"{adapter_name}." if adapter_name else ""

    for name in model_state_dict.keys():
        if string_to_replace:
            name = name.replace(string_to_replace, "")
        if "." in name:
            module_name = name.rsplit(".", 1)[0]
            all_modules.add(module_name)

    target_modules_set = {name.split(".lora")[0] for name in peft_state_dict.keys()}
    print(f"{target_modules_set=}")
    exclude_modules = list(all_modules - target_modules_set)

    return exclude_modules


Review comment (suggestion):
maybe the if-statement is not needed here because string_to_replace will be an empty string, but no problem keeping as micro optimization

Code after:
        missing_keys = getattr(incompatible_keys, "missing_keys", None)
        if missing_keys:
            lora_missing_keys = [k for k in missing_keys if "lora_" in k and adapter_name in k]
            if lora_missing_keys:
                warn_msg += (
                    f"Loading adapter weights from state_dict led to missing keys in the model:"
                    f" {', '.join(lora_missing_keys)}."
                )

    if warn_msg:
        logger.warning(warn_msg)


def _derive_exclude_modules(model_state_dict, peft_state_dict, adapter_name=None):
    """
    Derives the modules to exclude while initializing `LoraConfig` through `exclude_modules`. It works by comparing the
    `model_state_dict` and `peft_state_dict` and adds a module from `model_state_dict` to the exclusion set if it
    doesn't exist in `peft_state_dict`.
    """
    if model_state_dict is None:
        return
    all_modules = set()
    string_to_replace = f"{adapter_name}." if adapter_name else ""

    for name in model_state_dict.keys():
        if string_to_replace:
            name = name.replace(string_to_replace, "")
        if "." in name:
            module_name = name.rsplit(".", 1)[0]
            all_modules.add(module_name)

    target_modules_set = {name.split(".lora")[0] for name in peft_state_dict.keys()}
    exclude_modules = list(all_modules - target_modules_set)

    return exclude_modules