ronantakizawa/codereview-bench huggingface dataset:
Repository: Lightning-AI/pytorch-lightning
File: tests/helpers/skipif.py (Python)

Code before:
            ...

    >>> from pprint import pprint
    >>> pprint(create_skipif(min_torch="99"))
    {'condition': True, 'reason': 'test requires minimal version `torch>=99'}
    >>> pprint(create_skipif(min_torch="0.0"))
    {'condition': False, 'reason': 'no reason, just go test it...'}
    """
    conditions = []
    reasons = []

    if min_gpus:
        conditions.append(torch.cuda.device_count() < min_gpus)
        reasons.append(f"multi-GPU machine with at least {min_gpus}")

    if min_torch:
        torch_version = LooseVersion(get_distribution("torch").version)
        conditions.append(torch_version < LooseVersion(min_torch))
        reasons.append(f"minimal version `torch>={min_torch}")

    if pt_quant:
        conditions.append(not _TORCH_QUANTIZE_AVAILABLE or _MISS_QUANT_DEFAULT)
        reasons.append("PyTorch quantization")

    if not any(conditions):
        return dict(condition=False, reason="Conditions satisfied, going ahead with the test")

    reasons = [rs for cond, rs in zip(conditions, reasons) if cond]
    reason = "test requires " + ' + '.join(reasons)
    return dict(condition=any(conditions), reason=reason)


@pytest.mark.skipif(**create_skipif(min_torch="99"))
def test_always_skip():
    exit(1)


@pytest.mark.skipif(**create_skipif(min_torch="0.0"))
def test_always_pass():
    assert True


Review comment (suggestion):
```suggestion
        return dict(condition=False, reason="Conditions satisfied, going ahead with the test.")
```

Code after:
    >>> from pprint import pprint
    >>> pprint(skipif_args(min_torch="99", min_gpus=0))
    {'condition': True, 'reason': 'Required: [torch>=99]'}
    >>> pprint(skipif_args(min_torch="0.0", min_gpus=0))  # doctest: +NORMALIZE_WHITESPACE
    {'condition': False, 'reason': 'Conditions satisfied, going ahead with the test.'}
    """
    conditions = []
    reasons = []

    if min_gpus:
        conditions.append(torch.cuda.device_count() < min_gpus)
        reasons.append(f"GPUs>={min_gpus}")

    if min_torch:
        torch_version = LooseVersion(get_distribution("torch").version)
        conditions.append(torch_version < LooseVersion(min_torch))
        reasons.append(f"torch>={min_torch}")

    if quant_available:
        _miss_default = 'fbgemm' not in torch.backends.quantized.supported_engines
        conditions.append(not _TORCH_QUANTIZE_AVAILABLE or _miss_default)
        reasons.append("PyTorch quantization is available")

    if not any(conditions):
        return dict(condition=False, reason="Conditions satisfied, going ahead with the test.")

    reasons = [rs for cond, rs in zip(conditions, reasons) if cond]
    return dict(condition=any(conditions), reason=f"Required: [{' + '.join(reasons)}]",)


@pytest.mark.skipif(**skipif_args(min_torch="99"))
def test_always_skip():
    exit(1)


@pytest.mark.skipif(**skipif_args(min_torch="0.0"))
def test_always_pass():
    assert True
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/metrics/utils.py (Python)

Code before:

    if class_reduction == "micro":
        return fraction
    elif class_reduction == "macro":
        return torch.mean(fraction)
    elif class_reduction == "weighted":
        return torch.sum(fraction * (weights.float() / torch.sum(weights)))
    elif class_reduction == "none" or class_reduction is None:
        return fraction

    raise ValueError(
        f"Reduction parameter {class_reduction} unknown."
        f" Choose between one of these: {valid_reduction}"
    )


def _stable_1d_sort(x: torch, N: int = 2049):
    """
    Stable sort of 1d tensors. Pytorch defaults to a stable sorting algorithm
    if number of elements are larger than 2048. This function pads the tensors,
    makes the sort and returns the sorted array (with the padding removed)
    See this discussion: https://discuss.pytorch.org/t/is-torch-sort-stable/20714

    Raises:
        ValueError:
            If dim of x is greater than 1 since stable sort works with only 1d tensors
    """
    if x.ndim > 1:
        raise ValueError('Stable sort only works on 1d tensors')
    n = x.numel()
    if N - n > 0:
        x_max = x.max()
        x_pad = torch.cat([x, (x_max + 1) * torch.ones(2049 - n, dtype=x.dtype, device=x.device)], 0)
    x_sort = x_pad.sort()
    return x_sort.values[:n], x_sort.indices[:n]


Review comment (suggestion):
```suggestion
            If dim of ``x`` is greater than 1 since stable sort works with only 1d tensors.
```

Code after:

    if class_reduction == "micro":
        return fraction
    elif class_reduction == "macro":
        return torch.mean(fraction)
    elif class_reduction == "weighted":
        return torch.sum(fraction * (weights.float() / torch.sum(weights)))
    elif class_reduction == "none" or class_reduction is None:
        return fraction

    raise ValueError(
        f"Reduction parameter {class_reduction} unknown."
        f" Choose between one of these: {valid_reduction}"
    )


def _stable_1d_sort(x: torch, N: int = 2049):
    """
    Stable sort of 1d tensors. Pytorch defaults to a stable sorting algorithm
    if number of elements are larger than 2048. This function pads the tensors,
    makes the sort and returns the sorted array (with the padding removed)
    See this discussion: https://discuss.pytorch.org/t/is-torch-sort-stable/20714

    Raises:
        ValueError:
            If dim of ``x`` is greater than 1 since stable sort works with only 1d tensors.
    """
    if x.ndim > 1:
        raise ValueError('Stable sort only works on 1d tensors')
    n = x.numel()
    if N - n > 0:
        x_max = x.max()
        x_pad = torch.cat([x, (x_max + 1) * torch.ones(2049 - n, dtype=x.dtype, device=x.device)], 0)
    x_sort = x_pad.sort()
    return x_sort.values[:n], x_sort.indices[:n]
Repository: Lightning-AI/pytorch-lightning
File: tests/helpers/skipif.py (Python)

Code before:
    """

    def __new__(
        self,
        *args,
        min_gpus: int = 0,
        min_torch: Optional[str] = None,
        quantization: bool = False,
        **kwargs
    ):
        conditions = []
        reasons = []

        if min_gpus:
            conditions.append(torch.cuda.device_count() < min_gpus)
            reasons.append(f"GPUs>={min_gpus}")

        if min_torch:
            torch_version = LooseVersion(get_distribution("torch").version)
            conditions.append(torch_version < LooseVersion(min_torch))
            reasons.append(f"torch>={min_torch}")

        if quantization:
            _miss_default = 'fbgemm' not in torch.backends.quantized.supported_engines
            conditions.append(not _TORCH_QUANTIZE_AVAILABLE or _miss_default)
            reasons.append("PyTorch quantization is available")

        reasons = [rs for cond, rs in zip(conditions, reasons) if cond]
        return pytest.mark.skipif(
            *args,
            condition=any(conditions),
            reason=f"Requires: [{' + '.join(reasons)}]",
            **kwargs,
        )


@SkipIf(min_torch="99")
def test_always_skip():
    exit(1)


@pytest.mark.parametrize("arg1", [0.5, 1.0, 2.0])
@SkipIf(min_torch="0.0")
def test_wrapper(arg1):
    assert arg1 > 0.0


Review comment (suggestion):
```suggestion
            reasons.append("missing PyTorch quantization")
```

Code after:
    """

    def __new__(
        self,
        *args,
        min_gpus: int = 0,
        min_torch: Optional[str] = None,
        quantization: bool = False,
        **kwargs
    ):
        """
        Args:
            args: native pytest.mark.skipif arguments
            min_gpus: min number of gpus required to run test
            min_torch: minimum pytorch version to run test
            quantization: if `torch.quantization` package is required to run test
            kwargs: native pytest.mark.skipif keyword arguments
        """
        conditions = []
        reasons = []

        if min_gpus:
            conditions.append(torch.cuda.device_count() < min_gpus)
            reasons.append(f"GPUs>={min_gpus}")

        if min_torch:
            torch_version = LooseVersion(get_distribution("torch").version)
            conditions.append(torch_version < LooseVersion(min_torch))
            reasons.append(f"torch>={min_torch}")

        if quantization:
            _miss_default = 'fbgemm' not in torch.backends.quantized.supported_engines
            conditions.append(not _TORCH_QUANTIZE_AVAILABLE or _miss_default)
            reasons.append("missing PyTorch quantization")

        reasons = [rs for cond, rs in zip(conditions, reasons) if cond]
        return pytest.mark.skipif(
            *args,
            condition=any(conditions),
            reason=f"Requires: [{' + '.join(reasons)}]",
            **kwargs,
        )


@SkipIf(min_torch="99")
def test_always_skip():
    exit(1)


@pytest.mark.parametrize("arg1", [0.5, 1.0, 2.0])
@SkipIf(min_torch="0.0")
Repository: Lightning-AI/pytorch-lightning
File: tests/utilities/test_deprecation.py (Python)

Code before:

from pytorch_lightning.utilities.deprecation import _deprecated


def my_sum(a, b=3):
    return a + b


@_deprecated(target=my_sum, ver_deprecate="0.1", ver_remove="0.5")
def dep_sum(a, b):
    pass


@_deprecated(target=my_sum, ver_deprecate="0.1", ver_remove="0.5")
def dep2_sum(a, b):
    pass


def test_deprecated_func():
    with pytest.deprecated_call(
        match='This `dep_sum` was deprecated since v0.1 in favor of `tests.utilities.test_deprecation.my_sum`.'
        ' It will be removed in v0.5.'
    ):
        assert dep_sum(2, b=5) == 7

    with pytest.warns(None) as record:
        assert dep_sum(2, b=5) == 7
    assert len(record) == 0

    with pytest.deprecated_call(
        match='This `dep2_sum` was deprecated since v0.1 in favor of `tests.utilities.test_deprecation.my_sum`.'
        ' It will be removed in v0.5.'
    ):
        assert dep2_sum(2) == 5


Review comment (suggestion):
```suggestion
    # check that the warning is raised only once per function
    with pytest.warns(None) as record:
```

Code after:

from pytorch_lightning.utilities.deprecation import deprecated
from tests.helpers.utils import no_warning_call


def my_sum(a, b=3):
    return a + b


@deprecated(target=my_sum, ver_deprecate="0.1", ver_remove="0.5")
def dep_sum(a, b):
    pass


@deprecated(target=my_sum, ver_deprecate="0.1", ver_remove="0.5")
def dep2_sum(a, b):
    pass


def test_deprecated_func():
    with pytest.deprecated_call(
        match='The `dep_sum` was deprecated since v0.1 in favor of `tests.utilities.test_deprecation.my_sum`.'
        ' It will be removed in v0.5.'
    ):
        assert dep_sum(2, b=5) == 7

    # check that the warning is raised only once per function
    with no_warning_call(DeprecationWarning):
        assert dep_sum(2, b=5) == 7

    # and does not affect other functions
    with pytest.deprecated_call(
        match='The `dep2_sum` was deprecated since v0.1 in favor of `tests.utilities.test_deprecation.my_sum`.'
        ' It will be removed in v0.5.'
    ):
        assert dep2_sum(2) == 5
Repository: Lightning-AI/pytorch-lightning
File: tests/utilities/test_deprecation.py (Python)

Code before:


@_deprecated(target=my_sum, ver_deprecate="0.1", ver_remove="0.5")
def dep_sum(a, b):
    pass


@_deprecated(target=my_sum, ver_deprecate="0.1", ver_remove="0.5")
def dep2_sum(a, b):
    pass


def test_deprecated_func():
    with pytest.deprecated_call(
        match='This `dep_sum` was deprecated since v0.1 in favor of `tests.utilities.test_deprecation.my_sum`.'
        ' It will be removed in v0.5.'
    ):
        assert dep_sum(2, b=5) == 7

    with pytest.warns(None) as record:
        assert dep_sum(2, b=5) == 7
    assert len(record) == 0

    with pytest.deprecated_call(
        match='This `dep2_sum` was deprecated since v0.1 in favor of `tests.utilities.test_deprecation.my_sum`.'
        ' It will be removed in v0.5.'
    ):
        assert dep2_sum(2) == 5


Review comment (suggestion):
```suggestion
    # and does not affect other functions
    with pytest.deprecated_call(
```

Code after:
    return a + b


@deprecated(target=my_sum, ver_deprecate="0.1", ver_remove="0.5")
def dep_sum(a, b):
    pass


@deprecated(target=my_sum, ver_deprecate="0.1", ver_remove="0.5")
def dep2_sum(a, b):
    pass


def test_deprecated_func():
    with pytest.deprecated_call(
        match='The `dep_sum` was deprecated since v0.1 in favor of `tests.utilities.test_deprecation.my_sum`.'
        ' It will be removed in v0.5.'
    ):
        assert dep_sum(2, b=5) == 7

    # check that the warning is raised only once per function
    with no_warning_call(DeprecationWarning):
        assert dep_sum(2, b=5) == 7

    # and does not affect other functions
    with pytest.deprecated_call(
        match='The `dep2_sum` was deprecated since v0.1 in favor of `tests.utilities.test_deprecation.my_sum`.'
        ' It will be removed in v0.5.'
    ):
        assert dep2_sum(2) == 5
Repository: Lightning-AI/pytorch-lightning
File: tests/utilities/test_deprecation.py (Python)

Code before:
from pytorch_lightning.utilities.deprecation import _deprecated


def my_sum(a, b=3):
    return a + b


@_deprecated(target=my_sum, ver_deprecate="0.1", ver_remove="0.5")
def dep_sum(a, b):
    pass


@_deprecated(target=my_sum, ver_deprecate="0.1", ver_remove="0.5")
def dep2_sum(a, b):
    pass


def test_deprecated_func():
    with pytest.deprecated_call(
        match='This `dep_sum` was deprecated since v0.1 in favor of `tests.utilities.test_deprecation.my_sum`.'
        ' It will be removed in v0.5.'
    ):
        assert dep_sum(2, b=5) == 7

    # check that the warning is raised only once per function
    with pytest.warns(None) as record:
        assert dep_sum(2, b=5) == 7
    assert len(record) == 0

    # and does not affect other functions
    with pytest.deprecated_call(
        match='This `dep2_sum` was deprecated since v0.1 in favor of `tests.utilities.test_deprecation.my_sum`.'
        ' It will be removed in v0.5.'
    ):
        assert dep2_sum(2) == 5


Review comment (refactor):
@Borda You could use this https://github.com/PyTorchLightning/pytorch-lightning/blob/master/tests/helpers/utils.py#L121

```python
with no_warning_call(DeprecationWarning):
	...
```

Code after:
from pytorch_lightning.utilities.deprecation import deprecated
from tests.helpers.utils import no_warning_call


def my_sum(a, b=3):
    return a + b


@deprecated(target=my_sum, ver_deprecate="0.1", ver_remove="0.5")
def dep_sum(a, b):
    pass


@deprecated(target=my_sum, ver_deprecate="0.1", ver_remove="0.5")
def dep2_sum(a, b):
    pass


def test_deprecated_func():
    with pytest.deprecated_call(
        match='The `dep_sum` was deprecated since v0.1 in favor of `tests.utilities.test_deprecation.my_sum`.'
        ' It will be removed in v0.5.'
    ):
        assert dep_sum(2, b=5) == 7

    # check that the warning is raised only once per function
    with no_warning_call(DeprecationWarning):
        assert dep_sum(2, b=5) == 7

    # and does not affect other functions
    with pytest.deprecated_call(
        match='The `dep2_sum` was deprecated since v0.1 in favor of `tests.utilities.test_deprecation.my_sum`.'
        ' It will be removed in v0.5.'
    ):
        assert dep2_sum(2) == 5
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/loggers/mlflow.py (Python)

Code before:
from time import time
from typing import Any, Dict, Optional, Union

from pytorch_lightning.loggers.base import LightningLoggerBase, rank_zero_experiment
from pytorch_lightning.utilities import _module_available, rank_zero_only, rank_zero_warn

log = logging.getLogger(__name__)
LOCAL_FILE_URI_PREFIX = "file:"
_MLFLOW_AVAILABLE = _module_available("mlflow")
try:
    import mlflow
    from mlflow.tracking import MlflowClient
    from mlflow.tracking import context
# todo: there seems to be still some remaining import error with Conda env
except ImportError:
    _MLFLOW_AVAILABLE = False
    mlflow, MlflowClient, context = None, None, None


if _MLFLOW_AVAILABLE:
    # before v1.1.0
    if hasattr(context, 'resolve_tags'):
        from mlflow.tracking.context import resolve_tags
    # since v1.1.0
    elif hasattr(context, 'registry'):
        from mlflow.tracking.context.resolve_tags import resolve_tags
    else:

        def resolve_tags(tags=None):
            return tags


class MLFlowLogger(LightningLoggerBase):
    """
    Log using `MLflow <https://mlflow.org>`_.

    Install it with pip:

    .. code-block:: bash

        pip install mlflow

    .. code-block:: python

        from pytorch_lightning import Trainer
        from pytorch_lightning.loggers import MLFlowLogger
        mlf_logger = MLFlowLogger(
            experiment_name="default",
            tracking_uri="file:./ml-runs"
        )
        trainer = Trainer(logger=mlf_logger)

Review comment (suggestion):
Oops, it's actually 
```python
from mlflow.tracking.context.registry import resolve_tags
```
With the fix above it works as intended! 

Code after:
from time import time
from typing import Any, Dict, Optional, Union

from pytorch_lightning.loggers.base import LightningLoggerBase, rank_zero_experiment
from pytorch_lightning.utilities import _module_available, rank_zero_only, rank_zero_warn

log = logging.getLogger(__name__)
LOCAL_FILE_URI_PREFIX = "file:"
_MLFLOW_AVAILABLE = _module_available("mlflow")
try:
    import mlflow
    from mlflow.tracking import MlflowClient
    from mlflow.tracking import context
# todo: there seems to be still some remaining import error with Conda env
except ImportError:
    _MLFLOW_AVAILABLE = False
    mlflow, MlflowClient, context = None, None, None


# before v1.1.0
if hasattr(context, 'resolve_tags'):
    from mlflow.tracking.context import resolve_tags
# since v1.1.0
elif hasattr(context, 'registry'):
    from mlflow.tracking.context.registry import resolve_tags
else:

    def resolve_tags(tags=None):
        return tags


class MLFlowLogger(LightningLoggerBase):
    """
    Log using `MLflow <https://mlflow.org>`_.

    Install it with pip:

    .. code-block:: bash

        pip install mlflow

    .. code-block:: python

        from pytorch_lightning import Trainer
        from pytorch_lightning.loggers import MLFlowLogger
        mlf_logger = MLFlowLogger(
            experiment_name="default",
            tracking_uri="file:./ml-runs"
        )
        trainer = Trainer(logger=mlf_logger)
Repository: Lightning-AI/pytorch-lightning
File: tests/utilities/test_enums.py (Python)

Code before:
from pytorch_lightning.utilities import DeviceType


def test_consistency():
    assert DeviceType.TPU not in ('GPU', 'CPU')
    assert DeviceType.TPU in ('TPU', 'CPU')
    assert DeviceType.TPU in ('tpu', 'CPU')
    assert DeviceType.TPU not in {'GPU', 'CPU'}
    assert DeviceType.TPU in {'TPU', 'CPU'}
    assert DeviceType.TPU in {'tpu', 'CPU'}


Review comment (refactor):
@tchaton The issue is that 'TPU' is hashed as uppercase but DeviceType.TPU is hashed as lower case, so I think we can just remove this line:
```suggestion
    # hash cannot be case invariant
    assert DeviceType.TPU not in {'TPU', 'CPU'}
```

Code after:
from pytorch_lightning.utilities import DeviceType


def test_consistency():
    assert DeviceType.TPU not in ('GPU', 'CPU')
    assert DeviceType.TPU in ('TPU', 'CPU')
    assert DeviceType.TPU in ('tpu', 'CPU')
    assert DeviceType.TPU not in {'GPU', 'CPU'}
    # hash cannot be case invariant
    assert DeviceType.TPU not in {'TPU', 'CPU'}
    assert DeviceType.TPU in {'tpu', 'CPU'}
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/loops/utilities.py (Python)

Code before:

    # handle dict return
    if isinstance(training_step_output, dict):
        # this should not modify the `training_step_output`, as the user could be using it after `training_step_end`
        loss = training_step_output.get("loss")
        hiddens = training_step_output.get("hiddens")
        # detach hiddens to avoid `RuntimeError: Trying to backward through the graph a second time`
        hiddens = apply_to_collection(hiddens, torch.Tensor, lambda t: t.detach())
        # use the setter instead of `dict.update` because it calls `detach` on the tensor items
        results.extra = {k: v for k, v in training_step_output.items() if k not in ("loss", "hiddens")}

    # handle scalar return
    elif isinstance(training_step_output, torch.Tensor):
        loss = training_step_output

    # map to results under the hood
    results.minimize = loss

    if trainer.move_metrics_to_cpu:
        results.cpu()
    return results, hiddens


def _prepare_dataloader_iter(dataloader_iter: Iterator, batch_idx: int) -> Iterator:
    """Attach the dataloader"""
    if not isinstance(dataloader_iter, DataLoaderIterDataFetcher):
        dataloader_iter = enumerate(dataloader_iter, batch_idx)
    # restore iteration
    return dataloader_iter


Review comment (performance):
why not simply move this into the contract of this iterator? basically make the contract such that these data fetchers will always return the batch_idx as part of it. 

Code after:

    # handle dict return
    if isinstance(training_step_output, dict):
        # this should not modify the `training_step_output`, as the user could be using it after `training_step_end`
        loss = training_step_output.get("loss")
        hiddens = training_step_output.get("hiddens")
        # detach hiddens to avoid `RuntimeError: Trying to backward through the graph a second time`
        hiddens = apply_to_collection(hiddens, torch.Tensor, lambda t: t.detach())
        # use the setter instead of `dict.update` because it calls `detach` on the tensor items
        results.extra = {k: v for k, v in training_step_output.items() if k not in ("loss", "hiddens")}

    # handle scalar return
    elif isinstance(training_step_output, torch.Tensor):
        loss = training_step_output

    # map to results under the hood
    results.minimize = loss

    if trainer.move_metrics_to_cpu:
        results.cpu()
    return results, hiddens


def _prepare_dataloader_iter(data_fetcher: AbstractDataFetcher, batch_idx: int) -> Iterator:
    """Attach the dataloader"""
    if not isinstance(data_fetcher, DataLoaderIterDataFetcher):
        # restore iteration
        dataloader_iter = enumerate(data_fetcher, batch_idx)
    else:
        dataloader_iter = iter(data_fetcher)
    return dataloader_iter