ronantakizawa/codereview-bench huggingface dataset:
Repository: hiyouga/LLaMA-Factory
File: src/llamafactory/extras/constants.py (Python)

Code before:
            DownloadSource.DEFAULT: "openbmb/MiniCPM-2B-sft-bf16",
            DownloadSource.MODELSCOPE: "OpenBMB/miniCPM-bf16",
        },
        "MiniCPM-2B-DPO-Chat": {
            DownloadSource.DEFAULT: "openbmb/MiniCPM-2B-dpo-bf16",
            DownloadSource.MODELSCOPE: "OpenBMB/MiniCPM-2B-dpo-bf16",
        },
    },
    template="cpm",
)


register_model_group(
    models={
        "MiniCPM3-4B-Chat": {
            DownloadSource.DEFAULT: "openbmb/MiniCPM3-4B",
            DownloadSource.MODELSCOPE: "OpenBMB/MiniCPM3-4B",
            DownloadSource.OPENMIND: "LlamaFactory/MiniCPM3-4B",
        },
    },
    template="cpm3",
)

register_model_group(
    models={
        "MiniCPM4-8B": {
            DownloadSource.DEFAULT: "openbmb/MiniCPM4-0_5B",
            DownloadSource.MODELSCOPE: "OpenBMB/MiniCPM4-0_5B",
        },
    },
    template="cpm4",
)

register_model_group(
    models={
        "MiniCPM4-4B": {
            DownloadSource.DEFAULT: "openbmb/MiniCPM4-8B",
            DownloadSource.MODELSCOPE: "OpenBMB/MiniCPM4-8B",
        },
    },
    template="cpm4",
)

register_model_group(
    models={
        "MiniCPM-o-2_6": {
            DownloadSource.DEFAULT: "openbmb/MiniCPM-o-2_6",
            DownloadSource.MODELSCOPE: "OpenBMB/MiniCPM-o-2_6",
        },
    },
    template="minicpm_o",

Review comment (suggestion):
Please change name to `MiniCPM4-8B-Chat` or `MiniCPM4-8B-Instruct` so llamafactory can enable chat template for it. https://github.com/hiyouga/LLaMA-Factory/blob/239ced076c93bd08ab2d1de1696447d07a1f9f9f/src/llamafactory/extras/constants.py#L145-L147

Code after:
            DownloadSource.DEFAULT: "openbmb/MiniCPM-2B-sft-bf16",
            DownloadSource.MODELSCOPE: "OpenBMB/miniCPM-bf16",
        },
        "MiniCPM-2B-DPO-Chat": {
            DownloadSource.DEFAULT: "openbmb/MiniCPM-2B-dpo-bf16",
            DownloadSource.MODELSCOPE: "OpenBMB/MiniCPM-2B-dpo-bf16",
        },
    },
    template="cpm",
)


register_model_group(
    models={
        "MiniCPM3-4B-Chat": {
            DownloadSource.DEFAULT: "openbmb/MiniCPM3-4B",
            DownloadSource.MODELSCOPE: "OpenBMB/MiniCPM3-4B",
            DownloadSource.OPENMIND: "LlamaFactory/MiniCPM3-4B",
        },
    },
    template="cpm3",
)

register_model_group(
    models={
        "MiniCPM4-0_5B-Chat": {
            DownloadSource.DEFAULT: "openbmb/MiniCPM4-0.5B",
            DownloadSource.MODELSCOPE: "OpenBMB/MiniCPM4-0.5B",
        },
    },
    template="cpm4",
)

register_model_group(
    models={
        "MiniCPM4-8B-Chat": {
            DownloadSource.DEFAULT: "openbmb/MiniCPM4-8B",
            DownloadSource.MODELSCOPE: "OpenBMB/MiniCPM4-8B",
        },
    },
    template="cpm4",
)

register_model_group(
    models={
        "MiniCPM-o-2_6": {
            DownloadSource.DEFAULT: "openbmb/MiniCPM-o-2_6",
            DownloadSource.MODELSCOPE: "OpenBMB/MiniCPM-o-2_6",
        },
    },
    template="minicpm_o",
Repository: hiyouga/LLaMA-Factory
File: src/llamafactory/train/trainer_utils.py (Python)

Code before:
    elif device_name == "npu":
        remote_config["resources"] = {"NPU": 1}

    return remote_config


def get_free_port() -> tuple[str, str]:
    with socket.socket() as sock:
        sock.bind(("", 0))
        port = sock.getsockname()[1]
    return str(port)


@cache
def get_device_name() -> str:
    if is_torch_cuda_available():
        device = "gpu"
    elif is_torch_npu_available():
        device = "npu"
    else:
        device = "cpu"
    return device


def get_torch_device():
    device_name = "cuda" if get_device_name() == "gpu" else get_device_name()
    try:
        return getattr(torch, device_name)
    except AttributeError:
        logger.warning_rank0(f"xx")
        return torch.cuda
    

def sort_placement_group_by_node_ip(placement_group, master_addr=None) -> list[int]:
    @ray.remote
    def _get_node_ip():
        return ray.util.get_node_ip_address().strip("[]")
    
    tasks = []
    for bundle_idx in range(placement_group.bundle_count):
        task = _get_node_ip.options(
            scheduling_strategy = PlacementGroupSchedulingStrategy(
                placement_group = placement_group,
                placement_group_bundle_index = bundle_idx,
            ),
        ).remote()
        tasks.append(task)
        
    bundle_ips = ray.get(tasks)
    bundle_node_ip_list = list(enumerate(bundle_ips))


Review comment (suggestion):
you can use `'cpu' if not torch.accelerator.is_available() else torch.accelerator.current_accelerator().type`  to get the device name, don't need `get_device_name` function.

Code after:
    return remote_config


def get_ray_head_node_ip() -> str:
    r"""Get the IP address of the Ray head node."""
    head_ip = next(node["NodeManagerAddress"] for node in ray.nodes() if node.get("IsHead", False))
    return head_ip


def sort_placement_group_by_node_ip(placement_group: "PlacementGroup", master_addr: str = None) -> list[int]:
    r"""Sort the placement group bundles by their node IP addresses."""

    @ray.remote
    def _get_node_ip():
        return ray.util.get_node_ip_address().strip("[]")

    tasks = []
    for bundle_idx in range(placement_group.bundle_count):
        task = _get_node_ip.options(
            scheduling_strategy=PlacementGroupSchedulingStrategy(
                placement_group=placement_group,
                placement_group_bundle_index=bundle_idx,
            ),
        ).remote()
        tasks.append(task)

    bundle_ips = ray.get(tasks)
    bundle_node_ip_list = list(enumerate(bundle_ips))

    sorted_bundle_node_ip_list = sorted(bundle_node_ip_list, key=lambda x: x[1])
    sorted_bundle_indices = [item[0] for item in sorted_bundle_node_ip_list]

    if master_addr is not None:
        preferred_indices = [idx for idx, ip in bundle_node_ip_list if ip == master_addr]
        if preferred_indices:
            remaining = [i for i in sorted_bundle_indices if i not in preferred_indices]
            sorted_bundle_indices = preferred_indices + remaining

    return sorted_bundle_indices
Repository: home-assistant/core
File: homeassistant/components/melcloud/diagnostics.py (Python)

Code before:
"""Diagnostics support for MelCloud."""

from __future__ import annotations

from typing import Any

from homeassistant.config_entries import ConfigEntry
from homeassistant.core import HomeAssistant
from homeassistant.helpers import entity_registry as er


async def async_get_config_entry_diagnostics(
    hass: HomeAssistant, entry: ConfigEntry
) -> dict[str, Any]:
    """Return diagnostics for the config entry."""
    ent_reg = er.async_get(hass)
    entities = [
        entity.entity_id
        for entity in er.async_entries_for_config_entry(ent_reg, entry.entry_id)
    ]

    entity_states = {entity: hass.states.get(entity) for entity in entities}

    return {
        "entry": entry.as_dict(),
        "entities": entity_states,
    }


Review comment (suggestion):
This entry contains a username and a token. These should not end up in diagnostics.

Diagnostic data must be stripped from personal data.

What would be interesting about the entry data in this case that it is useful to put it into the diagnostics?

../Frenck

Code after:
"""Diagnostics support for MelCloud."""

from __future__ import annotations

from typing import Any

from homeassistant.components.diagnostics.util import async_redact_data
from homeassistant.config_entries import ConfigEntry
from homeassistant.const import CONF_TOKEN, CONF_USERNAME
from homeassistant.core import HomeAssistant
from homeassistant.helpers import entity_registry as er

TO_REDACT = {
    CONF_USERNAME,
    CONF_TOKEN,
}


async def async_get_config_entry_diagnostics(
    hass: HomeAssistant, entry: ConfigEntry
) -> dict[str, Any]:
    """Return diagnostics for the config entry."""
    ent_reg = er.async_get(hass)
    entities = [
        entity.entity_id
        for entity in er.async_entries_for_config_entry(ent_reg, entry.entry_id)
    ]

    entity_states = {entity: hass.states.get(entity) for entity in entities}

    entry_dict = entry.as_dict()
    if "data" in entry_dict:
        entry_dict["data"] = async_redact_data(entry_dict["data"], TO_REDACT)

    return {
        "entry": entry_dict,
        "entities": entity_states,
    }
Repository: home-assistant/core
File: tests/helpers/test_selector.py (Python)

Code before:
)
def test_attribute_selector_schema(
    schema, valid_selections, invalid_selections
) -> None:
    """Test attribute selector."""
    _test_selector("attribute", schema, valid_selections, invalid_selections)


@pytest.mark.parametrize(
    ("schema", "valid_selections", "invalid_selections"),
    [
        (
            {},
            (
                {"seconds": 10},
                {"days": 10},  # Days is allowed also if `enable_day` is not set
            ),
            (None, {}),
        ),
        (
            {"enable_day": True},
            ({"seconds": 10}, {"days": 10}),
            (None, {}),
        ),
        (
            {"positive": True},
            ({"seconds": 10}, {"days": 10}),
            (None, {}, {"seconds": -1}),
        ),
    ],
)
def test_duration_selector_schema(schema, valid_selections, invalid_selections) -> None:
    """Test duration selector."""
    _test_selector("duration", schema, valid_selections, invalid_selections)


@pytest.mark.parametrize(
    ("schema", "valid_selections", "invalid_selections"),
    [
        (
            {},
            ("mdi:abc",),
            (None,),
        ),
    ],
)
def test_icon_selector_schema(schema, valid_selections, invalid_selections) -> None:
    """Test icon selector."""
    _test_selector("icon", schema, valid_selections, invalid_selections)



Review comment (suggestion):
```suggestion
            {"allow_negative": False},
```

Code after:
        ),
    ],
)
def test_attribute_selector_schema(
    schema, valid_selections, invalid_selections
) -> None:
    """Test attribute selector."""
    _test_selector("attribute", schema, valid_selections, invalid_selections)


@pytest.mark.parametrize(
    ("schema", "valid_selections", "invalid_selections"),
    [
        (
            {},
            (
                {"seconds": 10},
                {"days": 10},  # Days is allowed also if `enable_day` is not set
            ),
            (None, {}),
        ),
        (
            {"enable_day": True},
            ({"seconds": 10}, {"days": 10}),
            (None, {}),
        ),
        (
            {"allow_negative": False},
            ({"seconds": 10}, {"days": 10}),
            (None, {}, {"seconds": -1}),
        ),
    ],
)
def test_duration_selector_schema(schema, valid_selections, invalid_selections) -> None:
    """Test duration selector."""
    _test_selector("duration", schema, valid_selections, invalid_selections)


@pytest.mark.parametrize(
    ("schema", "valid_selections", "invalid_selections"),
    [
        (
            {},
            ("mdi:abc",),
            (None,),
        ),
    ],
)
def test_icon_selector_schema(schema, valid_selections, invalid_selections) -> None:
    """Test icon selector."""
    _test_selector("icon", schema, valid_selections, invalid_selections)
Repository: home-assistant/core
File: homeassistant/components/local_calendar/config_flow.py (Python)

Code before:

from homeassistant.components.file_upload import process_uploaded_file
from homeassistant.config_entries import ConfigFlow, ConfigFlowResult
from homeassistant.core import HomeAssistant
from homeassistant.exceptions import HomeAssistantError
from homeassistant.helpers import selector
from homeassistant.util import slugify

from .const import (
    ATTR_CREATE_EMPTY,
    ATTR_IMPORT_ICS_FILE,
    CONF_CALENDAR_COLOR,
    CONF_CALENDAR_NAME,
    CONF_ICS_FILE,
    CONF_IMPORT,
    CONF_STORAGE_KEY,
    DOMAIN,
    STORAGE_PATH,
)

_LOGGER = logging.getLogger(__name__)

STEP_USER_DATA_SCHEMA = vol.Schema(
    {
        vol.Required(CONF_CALENDAR_NAME): str,
        vol.Optional(CONF_CALENDAR_COLOR, default=None): vol.Any(str, None),
        vol.Optional(CONF_IMPORT, default=ATTR_CREATE_EMPTY): selector.SelectSelector(
            selector.SelectSelectorConfig(
                options=[
                    ATTR_CREATE_EMPTY,
                    ATTR_IMPORT_ICS_FILE,
                ],
                translation_key=CONF_IMPORT,
            )
        ),
    }
)

STEP_IMPORT_DATA_SCHEMA = vol.Schema(
    {
        vol.Required(CONF_ICS_FILE): selector.FileSelector(
            config=selector.FileSelectorConfig(accept=".ics")
        ),
    }
)


class LocalCalendarConfigFlow(ConfigFlow, domain=DOMAIN):
    """Handle a config flow for Local Calendar."""

    VERSION = 1

Review comment (suggestion):
This should be in the options flow and `options` config entry, and not the config  flow, since its the kind of thing to change after the integration is setup.

Code after:

from homeassistant.components.file_upload import process_uploaded_file
from homeassistant.config_entries import ConfigFlow, ConfigFlowResult
from homeassistant.core import HomeAssistant
from homeassistant.exceptions import HomeAssistantError
from homeassistant.helpers import selector
from homeassistant.util import slugify

from .const import (
    ATTR_CREATE_EMPTY,
    ATTR_IMPORT_ICS_FILE,
    CONF_CALENDAR_NAME,
    CONF_ICS_FILE,
    CONF_IMPORT,
    CONF_STORAGE_KEY,
    DOMAIN,
    STORAGE_PATH,
)

_LOGGER = logging.getLogger(__name__)

STEP_USER_DATA_SCHEMA = vol.Schema(
    {
        vol.Required(CONF_CALENDAR_NAME): str,
        vol.Optional(CONF_IMPORT, default=ATTR_CREATE_EMPTY): selector.SelectSelector(
            selector.SelectSelectorConfig(
                options=[
                    ATTR_CREATE_EMPTY,
                    ATTR_IMPORT_ICS_FILE,
                ],
                translation_key=CONF_IMPORT,
            )
        ),
    }
)

STEP_IMPORT_DATA_SCHEMA = vol.Schema(
    {
        vol.Required(CONF_ICS_FILE): selector.FileSelector(
            config=selector.FileSelectorConfig(accept=".ics")
        ),
    }
)


class LocalCalendarConfigFlow(ConfigFlow, domain=DOMAIN):
    """Handle a config flow for Local Calendar."""

    VERSION = 1

    def __init__(self) -> None:
Repository: home-assistant/core
File: tests/components/calendar/test_init.py (Python)

Code before:
    hass: HomeAssistant,
    service_data: dict[str, str],
    error_msg: str,
) -> None:
    """Test listing events from the service call using the same start and end time."""

    with pytest.raises(vol.error.MultipleInvalid, match=re.escape(error_msg)):
        await hass.services.async_call(
            DOMAIN,
            SERVICE_GET_EVENTS,
            service_data={
                "entity_id": "calendar.calendar_1",
                **service_data,
            },
            blocking=True,
            return_response=True,
        )


async def test_calendar_color_attribute(
    hass: HomeAssistant, test_entities: list[MockCalendarEntity]
) -> None:
    """Test that the color attribute is present and propagated if set."""
    entity = test_entities[0]
    # By default, no color
    assert entity.color is None
    # Add color and check state attributes
    entity._color = "#123456"
    entity.async_write_ha_state()
    await hass.async_block_till_done()
    state = hass.states.get(entity.entity_id)
    assert state is not None
    assert state.attributes["color"] == "#123456"
    # Remove color and check state attributes
    entity._color = None
    entity.async_write_ha_state()
    await hass.async_block_till_done()
    state = hass.states.get(entity.entity_id)
    assert state is not None
    assert "color" not in state.attributes or state.attributes["color"] is None


Review comment (suggestion):
We don't unittest things based on directly asserting on entity attributes. Make tests in terms of `state.attributes` only.

Code after:
)
async def test_list_events_service_same_dates(
    hass: HomeAssistant,
    service_data: dict[str, str],
    error_msg: str,
) -> None:
    """Test listing events from the service call using the same start and end time."""

    with pytest.raises(vol.error.MultipleInvalid, match=re.escape(error_msg)):
        await hass.services.async_call(
            DOMAIN,
            SERVICE_GET_EVENTS,
            service_data={
                "entity_id": "calendar.calendar_1",
                **service_data,
            },
            blocking=True,
            return_response=True,
        )


async def test_calendar_initial_color_valid(
    hass: HomeAssistant,
    entity_registry: er.EntityRegistry,
    test_entities: list[MockCalendarEntity],
) -> None:
    """Test that initial_color creates initial entity options."""
    # Entity 3 was created with an initial_color
    entity = test_entities[2]

    # Check that entity registry was populated with the initial_color
    entry = entity_registry.async_get(entity.entity_id)
    assert entry is not None
    assert entry.options.get(DOMAIN, {}).get("color") == "#FF0000"


@pytest.mark.parametrize(
    "invalid_initial_color",
    [
        "FF0000",  # Missing #
        "#FF00",  # Too short
        "#FF00000",  # Too long
        "#GGGGGG",  # Invalid hex
        "red",  # Not hex
        "",  # Empty
    ],
)
async def test_calendar_initial_color_invalid(
    hass: HomeAssistant,
    entity_registry: er.EntityRegistry,
    invalid_initial_color: str,
Repository: home-assistant/core
File: homeassistant/components/intent/timers.py (Python)

Code before:
from enum import StrEnum
import logging
import time
from typing import Any

from propcache.api import cached_property
import voluptuous as vol

from homeassistant.const import ATTR_DEVICE_ID, ATTR_ID, ATTR_NAME
from homeassistant.core import Context, HomeAssistant, callback
from homeassistant.helpers import (
    area_registry as ar,
    config_validation as cv,
    device_registry as dr,
    intent,
)
from homeassistant.util import ulid as ulid_util

from .const import TIMER_DATA

_LOGGER = logging.getLogger(__name__)

TIMER_NOT_FOUND_RESPONSE = "timer_not_found"
MULTIPLE_TIMERS_MATCHED_RESPONSE = "multiple_timers_matched"
NO_TIMER_SUPPORT_RESPONSE = "no_timer_support"
NO_INTENT_MATCH_RESPONSE = "no_intent_match"


@dataclass
class TimerInfo:
    """Information for a single timer."""

    id: str
    """Unique id of the timer."""

    name: str | None
    """User-provided name for timer."""

    seconds: int
    """Total number of seconds the timer should run for."""

    device_id: str | None
    """Id of the device where the timer was set.

    May be None only if conversation_command is set.
    """

    start_hours: int | None
    """Number of hours the timer should run as given by the user."""

    start_minutes: int | None

Review comment (suggestion):
```suggestion
NO_TIMER_COMMAND_RESPONSE = "no_timer_command"
```

Code after:
from enum import StrEnum
import logging
import time
from typing import Any

from propcache.api import cached_property
import voluptuous as vol

from homeassistant.const import ATTR_DEVICE_ID, ATTR_ID, ATTR_NAME
from homeassistant.core import Context, HomeAssistant, callback
from homeassistant.helpers import (
    area_registry as ar,
    config_validation as cv,
    device_registry as dr,
    intent,
)
from homeassistant.util import ulid as ulid_util

from .const import TIMER_DATA

_LOGGER = logging.getLogger(__name__)

TIMER_NOT_FOUND_RESPONSE = "timer_not_found"
MULTIPLE_TIMERS_MATCHED_RESPONSE = "multiple_timers_matched"
NO_TIMER_SUPPORT_RESPONSE = "no_timer_support"
NO_TIMER_COMMAND_RESPONSE = "no_timer_command"


@dataclass
class TimerInfo:
    """Information for a single timer."""

    id: str
    """Unique id of the timer."""

    name: str | None
    """User-provided name for timer."""

    seconds: int
    """Total number of seconds the timer should run for."""

    device_id: str | None
    """Id of the device where the timer was set.

    May be None only if conversation_command is set.
    """

    start_hours: int | None
    """Number of hours the timer should run as given by the user."""

    start_minutes: int | None
Repository: home-assistant/core
File: tests/components/intent/test_timers.py (Python)

Code before:
"""Tests for intent timers."""

import asyncio
from unittest.mock import AsyncMock, MagicMock, patch

import pytest

from homeassistant.components import conversation
from homeassistant.components.homeassistant.exposed_entities import async_expose_entity
from homeassistant.components.intent.timers import (
    MultipleTimersMatchedError,
    NoTimerCommandError,
    TimerEventType,
    TimerInfo,
    TimerManager,
    TimerNotFoundError,
    TimersNotSupportedError,
    _round_time,
    async_device_supports_timers,
    async_register_timer_handler,
)
from homeassistant.const import ATTR_DEVICE_ID, ATTR_NAME
from homeassistant.core import HomeAssistant, callback
from homeassistant.helpers import (
    area_registry as ar,
    device_registry as dr,
    floor_registry as fr,
    intent,
)
from homeassistant.setup import async_setup_component

from tests.common import MockConfigEntry


@pytest.fixture
async def init_components(hass: HomeAssistant) -> None:
    """Initialize required components for tests."""
    assert await async_setup_component(hass, "homeassistant", {})

Review comment (suggestion):
```suggestion
    TIMER_DATA,
    TimerEventType,
```

Code after:
"""Tests for intent timers."""

import asyncio
from unittest.mock import AsyncMock, MagicMock, patch

import pytest

from homeassistant.components import conversation
from homeassistant.components.intent.timers import (
    TIMER_DATA,
    MultipleTimersMatchedError,
    NoTimerCommandError,
    TimerEventType,
    TimerInfo,
    TimerManager,
    TimerNotFoundError,
    TimersNotSupportedError,
    _round_time,
    async_device_supports_timers,
    async_register_timer_handler,
)
from homeassistant.const import ATTR_DEVICE_ID, ATTR_NAME
from homeassistant.core import HomeAssistant, callback
from homeassistant.helpers import (
    area_registry as ar,
    device_registry as dr,
    floor_registry as fr,
    intent,
)
from homeassistant.setup import async_setup_component

from tests.common import MockConfigEntry


@pytest.fixture
async def init_components(hass: HomeAssistant) -> None:
    """Initialize required components for tests."""
    assert await async_setup_component(hass, "homeassistant", {})