ronantakizawa/codereview-bench huggingface dataset:
Repository: oumi-ai/oumi
File: tests/scripts/launch_tests.sh (Shell)

Code before:
#!/bin/bash
set -e

SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
E2E_TEST_CONFIG="${SCRIPT_DIR}/lambda_e2e_tests_job.yaml"
echo "Using test config: ${E2E_TEST_CONFIG}"

export E2E_CLUSTER_PREFIX="oumi-${USER}-e2e-tests"
export E2E_USE_SPOT_VM=0 # Whether to use Spot VMs.
export E2E_CLUSTER="" # Cloud provider to use (e.g., "lambda", "aws", etc.)

# An alternative to H100 is A100-80GB, if they are available.
# However, A100-80GB:4 isn't available in Lambda.
declare -a accelerators_arr=("A100:1" "A100:4" "H100:4")

# Reset the variable to make sure that CLI `--resources.use_spot` arg is not ignored.
OUMI_USE_SPOT_VM=""

for CURR_GPU_NAME in "${accelerators_arr[@]}"
do
   echo "Testing with accelerator: ${CURR_GPU_NAME} ..."
   CLUSTER_SUFFIX=$(echo "print('${CURR_GPU_NAME}'.lower().replace(':','-').strip())" | python)
   if (( $E2E_USE_SPOT_VM == 0 )); then
      USE_SPOT_ARG="--resources.use_spot=false"
      CLUSTER_SUFFIX="${CLUSTER_SUFFIX}-nonspot"
   else
      USE_SPOT_ARG="--resources.use_spot=true"
      CLUSTER_SUFFIX="${CLUSTER_SUFFIX}-spot"
   fi
   CLUSTER_NAME="${E2E_CLUSTER_PREFIX}-${CLUSTER_SUFFIX}"

Review comment (suggestion):
We have the E2E_CLUSTER flag which controls which cloud the tests are run on. Since it seems there's not a big difference between runpod_e2e_tests_job.yaml and lambda_e2e_tests_job.yaml, could we just keep one? Could name it e2e_tests_job.yaml to be generic.

Code after:
#!/bin/bash
set -e

SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
E2E_TEST_CONFIG="${SCRIPT_DIR}/runpod_e2e_tests_job.yaml"
echo "Using test config: ${E2E_TEST_CONFIG}"

export E2E_CLUSTER_PREFIX="oumi-${USER}-e2e-tests"
export E2E_USE_SPOT_VM=0 # Whether to use Spot VMs.
export E2E_CLUSTER="" # Cloud provider to use (e.g., "lambda", "aws", etc.)

# An alternative to H100 is A100-80GB, if they are available.
# However, A100-80GB:4 isn't available in Lambda.
declare -a accelerators_arr=("A100-80GB:1" "A100-80GB:4")

# Reset the variable to make sure that CLI `--resources.use_spot` arg is not ignored.
OUMI_USE_SPOT_VM=""

for CURR_GPU_NAME in "${accelerators_arr[@]}"
do
   echo "Testing with accelerator: ${CURR_GPU_NAME} ..."
   CLUSTER_SUFFIX=$(echo "print('${CURR_GPU_NAME}'.lower().replace(':','-').strip())" | python)
   if (( $E2E_USE_SPOT_VM == 0 )); then
      USE_SPOT_ARG="--resources.use_spot=false"
      CLUSTER_SUFFIX="${CLUSTER_SUFFIX}-nonspot"
   else
      USE_SPOT_ARG="--resources.use_spot=true"
      CLUSTER_SUFFIX="${CLUSTER_SUFFIX}-spot"
   fi
   CLUSTER_NAME="${E2E_CLUSTER_PREFIX}-${CLUSTER_SUFFIX}"
Repository: oumi-ai/oumi
File: src/oumi/utils/debug_utils.py (Python)

Code before:
    model_input: dict[str, Any],
) -> None:
    """Logs an example of the data in each step for debugging purposes.

    Args:
        raw_example: The raw example from the dataset.
        formatted_example: The formatted example after processing.
        tokenized_example: The tokenized example after tokenization.
        model_input: The final model input after collating.
    """
    # Log to debug file
    logger.debug("Raw example: %s", raw_example)
    logger.debug("Formatted example: %s", formatted_example)
    logger.debug("Tokenized example: %s", tokenized_example)
    logger.debug("Model input: %s", model_input)

    # Generate timestamp for the debug file
    timestamp = datetime.datetime.now().strftime("%Y-%m-%d_%H-%M-%S")
    session_id = str(uuid.uuid4())[:6]

    # Format data for HTML display
    def format_for_html(obj):
        try:
            return json.dumps(obj, indent=2, default=str)
        except Exception:
            return str(obj)

    # Create simplified HTML content
    html_content = f"""<!DOCTYPE html>
<html>
<head>
    <meta charset="UTF-8">
    <title>Debug Log {timestamp}</title>
    <style>
        body {{ font-family: sans-serif; margin: 20px; }}
        h1 {{ color: #333; }}
        .info {{ color: #666; margin-bottom: 20px; }}
        .section {{ margin-bottom: 30px; }}
        h2 {{ background: #eee; padding: 8px; }}
        pre {{ background: #f8f8f8; padding: 10px; overflow: auto;
             border: 1px solid #ddd; }}
        .copy-btn {{ float: right; padding: 3px 8px; background: #eee;
                  border: 1px solid #ccc; cursor: pointer; }}
    </style>
</head>
<body>
    <h1>Oumi Debug Information</h1>
    <div class="section">
        <h2>Raw Example</h2>
        <pre id="raw">{format_for_html(raw_example)}</pre>
    </div>

Review comment (bug):
Saving as HTML is likely overkill here. I think calling `logger.debug` is likely sufficient.

Code after:

from typing import Any

from oumi.utils.logging import logger


def log_example_for_debugging(
    raw_example: Any,
    formatted_example: str,
    tokenized_example: list[tuple[int, str]],
    model_input: dict[str, Any],
) -> None:
    """Logs an example of the data in each step for debugging purposes.

    Args:
        raw_example: The raw example from the dataset.
        formatted_example: The formatted example after processing.
        tokenized_example: The tokenized example after tokenization.
        model_input: The final model input after collating.
    """
    # Log to debug file
    logger.debug("Raw example: %s", raw_example)
    logger.debug("Formatted example: %s", formatted_example)
    logger.debug("Tokenized example: %s", tokenized_example)
    logger.debug("Model input: %s", model_input)
Repository: oumi-ai/oumi
File: src/oumi/core/collators/vision_language_sft_collator.py (Python)

Code before:
        Raises:
            ValueError: If results have inconsistent keys or non-tensor values
        """
        if not results:
            return {}

        # Get keys from first result and verify consistency
        expected_keys = set(results[0].keys())
        for i, result in enumerate(results[1:], 1):
            if set(result.keys()) != expected_keys:
                raise ValueError(
                    f"Inconsistent keys in batch. Expected {expected_keys}, "
                    f"but result {i} has {set(result.keys())}"
                )

        # Collate each feature
        collated = {}
        for key in expected_keys:
            values = [result[key] for result in results]

            # Determine max variable dimensions based on feature type
            # For multi-image models, we may need 2 variable dims (num_images, seq_len)
            max_var_dims = 2 if self._allow_multi_image_inputs else 1

            # Pad and stack tensors
            collated[key] = pad_to_max_dim_and_stack(
                values, max_variable_sized_dims=max_var_dims
            )

        return collated


Review comment (suggestion):
note that this simple padding logic isn't compatible with all  VLM models. Consider adding a note about it

(delegating everything to `processor` was the original motivation for adding this collator)

Code after:
        Raises:
            ValueError: If results have inconsistent keys or non-tensor values
        """
        if not results or len(results) == 0:
            return {}

        # Get keys from first result and verify consistency
        expected_keys = set(results[0].keys())
        for i, result in enumerate(results):
            if set(result.keys()) != expected_keys:
                raise ValueError(
                    f"Inconsistent keys in batch. Expected {expected_keys}, "
                    f"but result {i} has {set(result.keys())}"
                )

        # Collate each feature
        collated = {}
        for key in expected_keys:
            values = [result[key] for result in results]

            # Determine max variable dimensions based on feature type
            # For multi-image models, we may need 2 variable dims (num_images, seq_len)
            max_var_dims = 2 if self._allow_multi_image_inputs else 1

            # Pad and stack tensors
            collated[key] = pad_to_max_dim_and_stack(
                values, max_variable_sized_dims=max_var_dims
            )

        return collated
Repository: oumi-ai/oumi
File: src/oumi/core/synthesis/document_ingestion.py (Python)

Code before:
from enum import Enum
from pathlib import Path

try:
    import pymupdf4llm  # pyright: ignore[reportMissingImports]
except ImportError:
    pymupdf4llm = None


class DocumentFormat(Enum):
    """Format of document (PDF, TXT, HTML, DOCX, MD)."""

    PDF = "pdf"
    """PDF file"""

    TXT = "txt"
    """Text file"""

    HTML = "html"
    """HTML file"""

    MD = "md"
    """Markdown file"""


class DocumentPath:
    """Path to a document in some storage location."""

    def __init__(self, path: str):
        """Initialize the document path.

        Args:
            path: The path to the document.
        """
        self._path = path
        self._document_format = self._get_document_format(path)

    def _get_document_format(self, path: str) -> DocumentFormat:
        """Get the document format from the path."""
        lower_path = path.lower()
        if lower_path.endswith(".pdf"):
            return DocumentFormat.PDF
        elif lower_path.endswith(".txt"):
            return DocumentFormat.TXT
        elif lower_path.endswith(".html"):
            return DocumentFormat.HTML
        elif lower_path.endswith(".md"):
            return DocumentFormat.MD
        else:
            raise ValueError(f"Unsupported document format: {path}")


Review comment (suggestion):
nit: these two classes (`DocumentPath` and `DocumentFormat`) feel a bit uncessary. If you're using `pathlib.Path`, you can get the same result with `path.suffix`

Code after:
from pathlib import Path

try:
    import pymupdf4llm  # pyright: ignore[reportMissingImports]
except ImportError:
    pymupdf4llm = None


class DocumentReader:
    """Reader for documents."""

    def __init__(self):
        """Initialize the document reader."""
        if pymupdf4llm is None:
            raise ImportError(
                "pymupdf4llm is not installed. Please install it with "
                "`pip install oumi[synthesis]`."
            )
        self._pymupdf4llm = pymupdf4llm

    def read(self, document_path: str) -> list[str]:
        """Read the document."""
        path = Path(document_path)
        if "*" in str(path):
            return self._read_from_glob(path)
        else:
            return [self._read_from_document_format(path)]

    def _read_from_glob(self, path: Path) -> list[str]:
        """Read the document from the glob path."""
        documents = []

        # Find the base directory (longest prefix without glob characters)
        parts = path.parts
        base_parts = []

        for part in parts:
            if "*" in part:
                break
            base_parts.append(part)

        # Determine base directory and pattern
        if base_parts:
            base_dir = Path(*base_parts)
            # Pattern is everything after the base directory
            pattern_parts = parts[len(base_parts) :]
            pattern = "/".join(pattern_parts)
        else:
            # If path starts with glob, use appropriate base
            if path.is_absolute():
                base_dir = Path("/")
Repository: oumi-ai/oumi
File: src/oumi/core/configs/params/judge_params.py (Python)

Code before:
# See the License for the specific language governing permissions and
# limitations under the License.

import string
from dataclasses import dataclass, field
from enum import Enum
from typing import Optional

from oumi.core.configs.params.base_params import BaseParams


def resolve_placeholders(
    text: str,
    values_dict: dict[str, str],
    missing_values_allowed: bool = False,
) -> str:
    """Resolve placeholder {variables} in the provided text from the values_dict."""

    class SafeDict(dict):
        def __missing__(self, key):
            if missing_values_allowed:
                return "{" + key + "}"
            else:
                raise ValueError(f"Missing value for placeholder: {key}")

    return string.Formatter().vformat(text, (), SafeDict(values_dict))


class JudgeResponseFormat(str, Enum):
    """Enumeration of possible response formats for the judge output."""

    JSON = "json"
    """JSON structured response format."""

    XML = "xml"
    """XML-tagged response format."""

    RAW = "raw"
    """Plain text response format."""


class JudgeOutputType(str, Enum):
    """Enumeration of possible output types for the judge's output fields."""

    TEXT = "text"
    """Free-form text judgment."""

    ENUM = "enum"
    """Categorical judgment from predefined options."""

    INT = "int"

Review comment (suggestion):
I don't think the string library is needed here. Instead do the following:
```suggestion
    return text.format_map(SafeDict(values_dict))
```

Code after:
# See the License for the specific language governing permissions and
# limitations under the License.

from dataclasses import dataclass, field
from enum import Enum
from typing import Optional

from oumi.core.configs.params.base_params import BaseParams
from oumi.utils.placeholders import resolve_placeholders


class JudgeResponseFormat(str, Enum):
    """Enumeration of possible response formats for the judge output."""

    JSON = "json"
    """JSON structured response format."""

    XML = "xml"
    """XML-tagged response format."""

    RAW = "raw"
    """Plain text response format."""


class JudgeOutputType(str, Enum):
    """Enumeration of possible output types for the judge's output fields."""

    TEXT = "text"
    """Free-form text judgment."""

    ENUM = "enum"
    """Categorical judgment from predefined options."""

    INT = "int"
    """Integer value judgment."""

    FLOAT = "float"
    """Floating-point value judgment."""

    BOOL = "bool"
    """Boolean judgment (True/False, Yes/No)."""


@dataclass
class JudgeParams(BaseParams):
    """Parameters for the Judge prompt and response format.

    This class holds the parameters for a single-attribute judge,
    including the prompt template and response format.

    Examples:
Repository: oumi-ai/oumi
File: src/oumi/utils/device_utils.py (Python)

Code before:
# Copyright 2025 - Oumi
#
# 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.

from collections.abc import Sequence
from pprint import pformat
from typing import NamedTuple, Optional

from pynvml import NVML_ERROR_NOT_SUPPORTED, NVMLError

from oumi.utils.logging import logger

try:
    # The library is only useful for NVIDIA GPUs, and
    # may not be installed for other vendors e.g., AMD
    import pynvml  # pyright: ignore[reportMissingImports]
except ModuleNotFoundError:
    pynvml = None

# TODO: OPE-562 - Add support for `amdsmi.amdsmi_init()`` for AMD GPUs


def _initialize_pynvml() -> bool:
    """Attempts to initialize pynvml library. Returns True on success."""
    global pynvml
    if pynvml is None:
        return False

    try:
        pynvml.nvmlInit()
    except Exception:
        logger.error(
            "Failed to initialize pynvml library. All pynvml calls will be disabled."
        )

Review comment (suggestion):
Importing explicitely could cause problems if the user does not have `pynvml` installed (it's an optional dependency)

Code after:
# Copyright 2025 - Oumi
#
# 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.

from collections.abc import Sequence
from pprint import pformat
from typing import NamedTuple, Optional

from oumi.utils.logging import logger

try:
    # The library is only useful for NVIDIA GPUs, and
    # may not be installed for other vendors e.g., AMD
    import pynvml  # pyright: ignore[reportMissingImports]
except ModuleNotFoundError:
    pynvml = None

# TODO: OPE-562 - Add support for `amdsmi.amdsmi_init()`` for AMD GPUs


def _initialize_pynvml() -> bool:
    """Attempts to initialize pynvml library. Returns True on success."""
    global pynvml
    if pynvml is None:
        return False

    try:
        pynvml.nvmlInit()
    except Exception:
        logger.error(
            "Failed to initialize pynvml library. All pynvml calls will be disabled."
        )
        pynvml = None
Repository: pallets/click
File: src/click/testing.py (Python)

Code before:
                stdout = outstreams[0].getvalue()
                if self.mix_stderr:
                    stderr = None
                else:
                    stderr = outstreams[1].getvalue()

        return Result(
            runner=self,
            stdout_bytes=stdout,
            stderr_bytes=stderr,
            return_value=return_value,
            exit_code=exit_code,
            exception=exception,
            exc_info=exc_info,
        )

    @contextlib.contextmanager
    def isolated_filesystem(self, tempdir=None):
        """A context manager that creates a temporary folder and changes
        the current working directory to it for isolated filesystem tests.

        :param tempdir: the path where the temporary directory should be created

        .. versionchanged:: 7.1.2
        """
        tempfile.tempdir = tempdir

        cwd = os.getcwd()
        t = tempfile.mkdtemp()
        os.chdir(t)
        try:
            yield t
        finally:
            os.chdir(cwd)
            if tempdir is None:
                try:
                    shutil.rmtree(t)
                except OSError:  # noqa: B014
                    pass


Review comment (suggestion):
This modifies global state and doesn't reset it. It's also not needed, `mkdtemp` takes a `dir` parameter.

Code after:
                stdout = outstreams[0].getvalue()
                if self.mix_stderr:
                    stderr = None
                else:
                    stderr = outstreams[1].getvalue()

        return Result(
            runner=self,
            stdout_bytes=stdout,
            stderr_bytes=stderr,
            return_value=return_value,
            exit_code=exit_code,
            exception=exception,
            exc_info=exc_info,
        )

    @contextlib.contextmanager
    def isolated_filesystem(self, temp_dir=None):
        """A context manager that creates a temporary directory and
        changes the current working directory to it. This isolates tests
        that affect the contents of the CWD to prevent them from
        interfering with each other.

        :param temp_dir: Create the temporary directory under this
            directory. If given, the created directory is not removed
            when exiting.

        .. versionchanged:: 8.0
            Added the ``temp_dir`` parameter.
        """
        cwd = os.getcwd()
        t = tempfile.mkdtemp(dir=temp_dir)
        os.chdir(t)

        try:
            yield t
        finally:
            os.chdir(cwd)

            if temp_dir is None:
                try:
                    shutil.rmtree(t)
                except OSError:  # noqa: B014
                    pass
Repository: pallets/click
File: src/click/utils.py (Python)

Code before:
            pass

    return wrapper


def make_str(value):
    """Converts a value into a valid string."""
    if isinstance(value, bytes):
        try:
            return value.decode(get_filesystem_encoding())
        except UnicodeError:
            return value.decode("utf-8", "replace")
    return str(value)


def make_default_short_help(help, max_length=45):
    """Return a condensed version of help string."""
    line_ending = help.find("\n\n")
    if line_ending != -1:
        help = help[:line_ending]
    words = help.split()
    total_length = 0
    result = []
    done = False

    if words[0] == "\b":
        for i, word in enumerate(words):
            if word != "\b":
                words = words[i:]
                break

    for word in words:
        if word[-1:] == ".":
            done = True
        new_length = 1 + len(word) if result else len(word)
        if total_length + new_length > max_length:
            result.append("...")
            done = True
        else:
            if result:
                result.append(" ")
            result.append(word)
        if done:
            break
        total_length += new_length

    return "".join(result)


class LazyFile:
    """A lazy file works like a regular file but it does not fully open

Review comment (suggestion):
The only valid use case for `\b` in Click is a single character on its own line, and for short help only the first line is relevant (subsequent paragraphs are already discarded). So this can be simplified to

```python
if words[0] == "\b":
    words = words[1:]
```

Code after:
            pass

    return wrapper


def make_str(value):
    """Converts a value into a valid string."""
    if isinstance(value, bytes):
        try:
            return value.decode(get_filesystem_encoding())
        except UnicodeError:
            return value.decode("utf-8", "replace")
    return str(value)


def make_default_short_help(help, max_length=45):
    """Return a condensed version of help string."""
    line_ending = help.find("\n\n")
    if line_ending != -1:
        help = help[:line_ending]
    words = help.split()
    total_length = 0
    result = []
    done = False

    if words[0] == "\b":
        words = words[1:]

    for word in words:
        if word[-1:] == ".":
            done = True
        new_length = 1 + len(word) if result else len(word)
        if total_length + new_length > max_length:
            result.append("...")
            done = True
        else:
            if result:
                result.append(" ")
            result.append(word)
        if done:
            break
        total_length += new_length

    return "".join(result)


class LazyFile:
    """A lazy file works like a regular file but it does not fully open
    the file but it does perform some basic checks early to see if the
    filename parameter does make sense.  This is useful for safely opening
    files for writing.