ronantakizawa/codereview-bench huggingface dataset:
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/callbacks/lr_monitor.py (Python)

Code before:
                while True:
                    if name not in names:
                        break
                    i, name = i + 1, f'{opt_name}-{i}'

            # Multiple param groups for the same schduler
            param_groups = sch.optimizer.param_groups

            if len(param_groups) != 1:
                for i, pg in enumerate(param_groups):
                    temp = f'{name}/pg{i + 1}'
                    names.append(temp)
            else:
                names.append(name)

            self.lr_sch_names.append(name)

        return names

    def _should_log(self, trainer) -> bool:
        should_log = (
            (trainer.global_step + 1) % trainer.log_every_n_steps == 0
            or trainer.should_stop
        )

        should_log = should_log and trainer.fast_dev_run
        return should_log


Review comment (suggestion):
```suggestion
        should_log = should_log and not trainer.fast_dev_run
```

Code after:
                while True:
                    if name not in names:
                        break
                    i, name = i + 1, f'{opt_name}-{i}'

            # Multiple param groups for the same schduler
            param_groups = sch.optimizer.param_groups

            if len(param_groups) != 1:
                for i, pg in enumerate(param_groups):
                    temp = f'{name}/pg{i + 1}'
                    names.append(temp)
            else:
                names.append(name)

            self.lr_sch_names.append(name)

        return names

    @staticmethod
    def _should_log(trainer) -> bool:
        should_log = (
            (trainer.global_step + 1) % trainer.log_every_n_steps == 0
            or trainer.should_stop
        )

        should_log = should_log and not trainer.fast_dev_run
        return should_log
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/core/hooks.py (Python)

Code before:
# Copyright The PyTorch Lightning team.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""Various Hooks to be used in the Lightning code."""

from typing import Any, Dict, List, Union

import torch
from pytorch_lightning.utilities import AMPType, move_data_to_device, rank_zero_warn
from torch import Tensor
from torch.optim.optimizer import Optimizer
from torch.utils.data import DataLoader


try:
    from apex import amp
except ImportError:
    amp = None


class ModelHooks:
    """Hooks to be used in LightningModule."""
    def setup(self, stage: str):
        """
        Called at the beginning of fit and test.
        This is a good hook when you need to build models dynamically or adjust something about them.
        This hook is called on every process when using DDP.

        Args:

Review comment (suggestion):
```suggestion
"""Various hooks to be used in the Lightning code."""
```

Code after:
# Copyright The PyTorch Lightning team.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""Various hooks to be used in the Lightning code."""

from typing import Any, Dict, List, Union

import torch
from pytorch_lightning.utilities import AMPType, move_data_to_device, rank_zero_warn
from torch import Tensor
from torch.optim.optimizer import Optimizer
from torch.utils.data import DataLoader


try:
    from apex import amp
except ImportError:
    amp = None


class ModelHooks:
    """Hooks to be used in LightningModule."""
    def setup(self, stage: str):
        """
        Called at the beginning of fit and test.
        This is a good hook when you need to build models dynamically or adjust something about them.
        This hook is called on every process when using DDP.

        Args:
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/metrics/regression/mean_squared_error.py (Python)

Code before:

    def __init__(
        self,
        compute_on_step: bool = True,
        ddp_sync_on_step: bool = False,
        process_group: Optional[Any] = None,
    ):
        super().__init__(
            compute_on_step=compute_on_step,
            ddp_sync_on_step=ddp_sync_on_step,
            process_group=process_group,
        )

        self.add_state("sum_squared_error", default=torch.tensor(0.0), dist_reduce_fx="sum")
        self.add_state("total", default=torch.tensor(0), dist_reduce_fx="sum")

    def update(self, preds: torch.Tensor, target: torch.Tensor):
        """
        Update state with predictions and targets.

        Args:
            preds: Predictions from model
            target: Ground truth values
        """
        assert preds.shape == target.shape, \
            'Predictions are targets are expected to have same shape'
        squared_error = torch.pow(preds - target, 2)

        self.sum_squared_error += torch.sum(squared_error)
        self.total += target.numel()

    def compute(self):
        """
        Computes mean squared error over state.
        """
        return self.sum_squared_error / self.total


Review comment (suggestion):
```suggestion
            'Predictions and targets are expected to have the same shape'
```

Code after:

    def __init__(
        self,
        compute_on_step: bool = True,
        ddp_sync_on_step: bool = False,
        process_group: Optional[Any] = None,
    ):
        super().__init__(
            compute_on_step=compute_on_step,
            ddp_sync_on_step=ddp_sync_on_step,
            process_group=process_group,
        )

        self.add_state("sum_squared_error", default=torch.tensor(0.0), dist_reduce_fx="sum")
        self.add_state("total", default=torch.tensor(0), dist_reduce_fx="sum")

    def update(self, preds: torch.Tensor, target: torch.Tensor):
        """
        Update state with predictions and targets.

        Args:
            preds: Predictions from model
            target: Ground truth values
        """
        assert preds.shape == target.shape, \
            'Predictions and targets are expected to have the same shape'
        squared_error = torch.pow(preds - target, 2)

        self.sum_squared_error += torch.sum(squared_error)
        self.total += target.numel()

    def compute(self):
        """
        Computes mean squared error over state.
        """
        return self.sum_squared_error / self.total
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/metrics/regression/mean_squared_log_error.py (Python)

Code before:

    def __init__(
        self,
        compute_on_step: bool = True,
        ddp_sync_on_step: bool = False,
        process_group: Optional[Any] = None,
    ):
        super().__init__(
            compute_on_step=compute_on_step,
            ddp_sync_on_step=ddp_sync_on_step,
            process_group=process_group,
        )

        self.add_state("sum_squared_log_error", default=torch.tensor(0.0), dist_reduce_fx="sum")
        self.add_state("total", default=torch.tensor(0), dist_reduce_fx="sum")

    def update(self, preds: torch.Tensor, target: torch.Tensor):
        """
        Update state with predictions and targets.

        Args:
            preds: Predictions from model
            target: Ground truth values
        """
        assert preds.shape == target.shape, \
            'Predictions are targets are expected to have same shape'
        squared_log_error = torch.pow(torch.log1p(preds) - torch.log1p(target), 2)

        self.sum_squared_log_error += torch.sum(squared_log_error)
        self.total += target.numel()

    def compute(self):
        """
        Compute mean squared logarithmic error over state.
        """
        return self.sum_squared_log_error / self.total


Review comment (suggestion):
```suggestion
            'Predictions and targets are expected to have the same shape'
```

Code after:

    def __init__(
        self,
        compute_on_step: bool = True,
        ddp_sync_on_step: bool = False,
        process_group: Optional[Any] = None,
    ):
        super().__init__(
            compute_on_step=compute_on_step,
            ddp_sync_on_step=ddp_sync_on_step,
            process_group=process_group,
        )

        self.add_state("sum_squared_log_error", default=torch.tensor(0.0), dist_reduce_fx="sum")
        self.add_state("total", default=torch.tensor(0), dist_reduce_fx="sum")

    def update(self, preds: torch.Tensor, target: torch.Tensor):
        """
        Update state with predictions and targets.

        Args:
            preds: Predictions from model
            target: Ground truth values
        """
        assert preds.shape == target.shape, \
            'Predictions and targets are expected to have the same shape'
        squared_log_error = torch.pow(torch.log1p(preds) - torch.log1p(target), 2)

        self.sum_squared_log_error += torch.sum(squared_log_error)
        self.total += target.numel()

    def compute(self):
        """
        Compute mean squared logarithmic error over state.
        """
        return self.sum_squared_log_error / self.total
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/trainer/connectors/env_vars_connector.py (Python)

Code before:
# limitations under the License.

from functools import wraps
from typing import Callable

from pytorch_lightning.utilities.argparse_utils import parse_env_variables, get_init_arguments_and_types


def overwrite_by_env_vars(fn: Callable) -> Callable:
    """
    Decorator for :class:`~pytorch_lightning.trainer.trainer.Trainer` methods for which
    input arguments should be moved automatically to the correct device.

    """
    @wraps(fn)
    def overwrite_by_env_vars(self, *args, **kwargs):
        # get the class
        cls = self.__class__
        if args:  # inace any args passed move them to kwargs
            # parse only the argument names
            cls_arg_names = [arg[0] for arg in get_init_arguments_and_types(cls)]
            # convert args to kwargs
            kwargs.update({k: v for k, v in zip(cls_arg_names, args)})
        # update the kwargs by env variables
        kwargs.update(vars(parse_env_variables(cls)))

        # all args were already moved to kwargs
        return fn(self, **kwargs)

    return overwrite_by_env_vars


Review comment (suggestion):
maybe add a warning that some init args were overwritten by Env arguments
```suggestion
        # todo: maybe add a warning that some init args were overwritten by Env arguments
        kwargs.update(vars(parse_env_variables(cls)))
```

Code after:
# limitations under the License.

from functools import wraps
from typing import Callable

from pytorch_lightning.utilities.argparse_utils import parse_env_variables, get_init_arguments_and_types


def overwrite_by_env_vars(fn: Callable) -> Callable:
    """
    Decorator for :class:`~pytorch_lightning.trainer.trainer.Trainer` methods for which
    input arguments should be moved automatically to the correct device.

    """
    @wraps(fn)
    def overwrite_by_env_vars(self, *args, **kwargs):
        # get the class
        cls = self.__class__
        if args:  # inace any args passed move them to kwargs
            # parse only the argument names
            cls_arg_names = [arg[0] for arg in get_init_arguments_and_types(cls)]
            # convert args to kwargs
            kwargs.update({k: v for k, v in zip(cls_arg_names, args)})
        # update the kwargs by env variables
        # todo: maybe add a warning that some init args were overwritten by Env arguments
        kwargs.update(vars(parse_env_variables(cls)))

        # all args were already moved to kwargs
        return fn(self, **kwargs)

    return overwrite_by_env_vars
Repository: Lightning-AI/pytorch-lightning
File: tests/checkpointing/test_torch_saving.py (Python)

Code before:
    """Test to ensure torch save does not fail for model and trainer using cpu ddp."""
    model = EvalModelTemplate()
    num_epochs = 1
    trainer = Trainer(
        default_root_dir=tmpdir,
        max_epochs=num_epochs,
        distributed_backend="ddp_cpu",
        num_processes=2,
    )
    temp_path = os.path.join(tmpdir, 'temp.pt')
    trainer.fit(model)

    # Ensure these do not fail
    torch.save(trainer.model, temp_path)
    torch.save(trainer, temp_path)


@pytest.mark.skipif(torch.cuda.device_count() < 2, reason="test requires multi-GPU machine")
def test_model_torch_save_ddp_cuda(tmpdir):
    """Test to ensure torch save does not fail for model and trainer using gpu ddp."""
    model = EvalModelTemplate()
    num_epochs = 1
    trainer = Trainer(
        default_root_dir=tmpdir,
        max_epochs=num_epochs,
        distributed_backend="ddp",
        gpus=2
    )
    temp_path = os.path.join(tmpdir, 'temp.pt')
    trainer.fit(model)

    # Ensure these do not fail
    torch.save(trainer.model, temp_path)
    torch.save(trainer, temp_path)


Review comment (suggestion):
Here you need to change to ddp_spawn
or rewrite the whole test to launch a script
This is the reason the test hangs.

Code after:
    """Test to ensure torch save does not fail for model and trainer using cpu ddp."""
    model = EvalModelTemplate()
    num_epochs = 1
    trainer = Trainer(
        default_root_dir=tmpdir,
        max_epochs=num_epochs,
        accelerator="ddp_cpu",
        num_processes=2,
    )
    temp_path = os.path.join(tmpdir, 'temp.pt')
    trainer.fit(model)

    # Ensure these do not fail
    torch.save(trainer.model, temp_path)
    torch.save(trainer, temp_path)


@pytest.mark.skipif(torch.cuda.device_count() < 2, reason="test requires multi-GPU machine")
def test_model_torch_save_ddp_cuda(tmpdir):
    """Test to ensure torch save does not fail for model and trainer using gpu ddp."""
    model = EvalModelTemplate()
    num_epochs = 1
    trainer = Trainer(
        default_root_dir=tmpdir,
        max_epochs=num_epochs,
        accelerator="ddp_spawn",
        gpus=2
    )
    temp_path = os.path.join(tmpdir, 'temp.pt')
    trainer.fit(model)

    # Ensure these do not fail
    torch.save(trainer.model, temp_path)
    torch.save(trainer, temp_path)
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/tuner/auto_gpu_select.py (Python)

Code before:
# Copyright The PyTorch Lightning team.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import torch


def pick_multiple_gpus(nb):
    if nb == 0:
        ResourceWarning(
            "Set `auto_select_gpus=True, gpus=0` is misconfiguration. Please select gpu resources"
        )

    nb = torch.cuda.device_count() if nb == -1 else nb

    picked = []
    for _ in range(nb):
        picked.append(pick_single_gpu(exclude_gpus=picked))

    return picked


def pick_single_gpu(exclude_gpus: list):
    for i in range(torch.cuda.device_count()):
        if i in exclude_gpus:
            continue
        # Try to allocate on device:
        device = torch.device(f"cuda:{i}")
        try:
            torch.ones(1).to(device)
        except RuntimeError:
            continue
        return i
    raise RuntimeError("No GPUs available.")


Review comment (suggestion):
Maybe replace this with a `MisconfigurationException`? 

Message: `auto_select_gpus=True, gpus=0` is not a valid configuration. Please select a valid number of GPU resources when using `auto_select_gpus`.

Code after:
# Copyright The PyTorch Lightning team.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import torch

from pytorch_lightning.utilities.exceptions import MisconfigurationException


def pick_multiple_gpus(nb):
    if nb == 0:
        raise MisconfigurationException(
            r"auto_select_gpus=True, gpus=0 is not a valid configuration.\
            Please select a valid number of GPU resources when using auto_select_gpus."
        )

    nb = torch.cuda.device_count() if nb == -1 else nb

    picked = []
    for _ in range(nb):
        picked.append(pick_single_gpu(exclude_gpus=picked))

    return picked


def pick_single_gpu(exclude_gpus: list):
    for i in range(torch.cuda.device_count()):
        if i in exclude_gpus:
            continue
        # Try to allocate on device:
        device = torch.device(f"cuda:{i}")
        try:
            torch.ones(1).to(device)
        except RuntimeError:
            continue
Repository: Lightning-AI/pytorch-lightning
File: tests/tuner/test_auto_gpu_select.py (Python)

Code before:
@pytest.mark.skipif(
    torch.cuda.device_count() < 1, reason="test requires a number of GPU machine greater than 1"
)
@pytest.mark.parametrize(
    ["auto_select_gpus", "gpus", "expected_error"],
    [
        pytest.param(True, 0, MisconfigurationException),
        pytest.param(True, -1, None),
        pytest.param(False, 0, None),
        pytest.param(False, -1, None),
    ],
)
def test_combination_gpus_options(auto_select_gpus, gpus, expected_error):
    model = BoringModel()

    if expected_error:
        with pytest.raises(expected_error):
            trainer = Trainer(auto_select_gpus=auto_select_gpus, gpus=gpus)
    else:
        trainer = Trainer(auto_select_gpus=auto_select_gpus, gpus=gpus)


@pytest.mark.skipif(not torch.cuda.is_available(), reason="test requires GPU machine")
@pytest.mark.skipif(
    torch.cuda.device_count() < 1, reason="test requires a number of GPU machine greater than 1"
)
@pytest.mark.parametrize(
    ["nb", "expected_gpu_idxs"],
    [
        pytest.param(0, []),
        pytest.param(-1, [i for i in range(torch.cuda.device_count())]),
        pytest.param(1, [0]),
    ],
)
def test_pick_multiple_gpus(nb, expected_gpu_idxs):
    assert expected_gpu_idxs == pick_multiple_gpus(nb)

Review comment (suggestion):
```suggestion
@pytest.mark.skipif(torch.cuda.device_count() < 2, reason="test requires multi-GPU machine")
```

Code after:

@pytest.mark.skipif(
    torch.cuda.device_count() < 2, reason="test requires a number of GPU machine greater than 1"
)
@pytest.mark.parametrize(
    ["auto_select_gpus", "gpus", "expected_error"],
    [
        (True, 0, MisconfigurationException),
        (True, -1, None),
        (False, 0, None),
        (False, -1, None),
    ],
)
def test_trainer_with_gpus_options_combination_at_available_gpus_env(
    auto_select_gpus, gpus, expected_error
):
    if expected_error:
        with pytest.raises(
            expected_error,
            match=re.escape(
                r"auto_select_gpus=True, gpus=0 is not a valid configuration.\
            Please select a valid number of GPU resources when using auto_select_gpus."
            ),
        ):
            trainer = Trainer(auto_select_gpus=auto_select_gpus, gpus=gpus)
    else:
        trainer = Trainer(auto_select_gpus=auto_select_gpus, gpus=gpus)


@pytest.mark.skipif(
    torch.cuda.device_count() < 2, reason="test requires a number of GPU machine greater than 1"
)
@pytest.mark.parametrize(
    ["nb", "expected_gpu_idxs", "expected_error"],
    [
        (0, [], MisconfigurationException),
        (-1, [i for i in range(torch.cuda.device_count())], None),
        (1, [0], None),
    ],
)
def test_pick_multiple_gpus(nb, expected_gpu_idxs, expected_error):
    if expected_error:
        with pytest.raises(
            expected_error,
            match=re.escape(
                r"auto_select_gpus=True, gpus=0 is not a valid configuration.\
            Please select a valid number of GPU resources when using auto_select_gpus."
            ),
        ):
            pick_multiple_gpus(nb)
    else:
Repository: Lightning-AI/pytorch-lightning
File: tests/tuner/test_auto_gpu_select.py (Python)

Code before:
    if expected_error:
        with pytest.raises(
            expected_error,
            match=r"auto_select_gpus=True, gpus=0 is not a valid configuration. Please select a valid number of GPU resources when using auto_select_gpus.",
        ):
            trainer = Trainer(auto_select_gpus=auto_select_gpus, gpus=gpus)
    else:
        trainer = Trainer(auto_select_gpus=auto_select_gpus, gpus=gpus)


@pytest.mark.skipif(
    torch.cuda.device_count() < 2, reason="test requires a number of GPU machine greater than 1"
)
@pytest.mark.parametrize(
    ["nb", "expected_gpu_idxs", "expected_error"],
    [
        pytest.param(0, [], MisconfigurationException),
        pytest.param(-1, [i for i in range(torch.cuda.device_count())], None),
        pytest.param(1, [0], None),
    ],
)
def test_pick_multiple_gpus(nb, expected_gpu_idxs, expected_error):
    if expected_error:
        with pytest.raises(
            expected_error,
            match=r"auto_select_gpus=True, gpus=0 is not a valid configuration. Please select a valid number of GPU resources when using auto_select_gpus.",
        ):
            pick_multiple_gpus(nb)
    else:
        assert expected_gpu_idxs == pick_multiple_gpus(nb)


Review comment (suggestion):
```suggestion
            match=r"auto_select_gpus=True, gpus=0 is not a valid configuration.\
            Please select a valid number of GPU resources when using auto_select_gpus.",
```

Code after:
    auto_select_gpus, gpus, expected_error
):
    if expected_error:
        with pytest.raises(
            expected_error,
            match=re.escape(
                r"auto_select_gpus=True, gpus=0 is not a valid configuration.\
            Please select a valid number of GPU resources when using auto_select_gpus."
            ),
        ):
            trainer = Trainer(auto_select_gpus=auto_select_gpus, gpus=gpus)
    else:
        trainer = Trainer(auto_select_gpus=auto_select_gpus, gpus=gpus)


@pytest.mark.skipif(
    torch.cuda.device_count() < 2, reason="test requires a number of GPU machine greater than 1"
)
@pytest.mark.parametrize(
    ["nb", "expected_gpu_idxs", "expected_error"],
    [
        (0, [], MisconfigurationException),
        (-1, [i for i in range(torch.cuda.device_count())], None),
        (1, [0], None),
    ],
)
def test_pick_multiple_gpus(nb, expected_gpu_idxs, expected_error):
    if expected_error:
        with pytest.raises(
            expected_error,
            match=re.escape(
                r"auto_select_gpus=True, gpus=0 is not a valid configuration.\
            Please select a valid number of GPU resources when using auto_select_gpus."
            ),
        ):
            pick_multiple_gpus(nb)
    else:
        assert expected_gpu_idxs == pick_multiple_gpus(nb)