Skip to content

Validate explicitly-provided adapter_model path in ModelParams #2321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
idoudali wants to merge 1 commit into from
Closed

Validate explicitly-provided adapter_model path in ModelParams #2321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/oumi/core/configs/params/model_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,18 @@ def __finalize_and_validate__(self):
logger.info(
f"Setting `model_name` to {model_name} found in adapter config."
)
else:
# Validate the explicitly-provided adapter_model path.
try:
adapter_config_file = find_adapter_config_file(self.adapter_model)
except (OSError, HFValidationError):
adapter_config_file = None
if not adapter_config_file or not Path(adapter_config_file).is_file():
raise ConfigFileNotFoundError(
f"adapter_config.json not found for adapter_model: "
f"'{self.adapter_model}'. Ensure this path points to a valid "
f"LoRA adapter directory containing adapter_config.json."
Comment on lines +319 to +325
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception handling here discards the underlying exception by setting adapter_config_file = None. Consider capturing the exception as e and raising ConfigFileNotFoundError(...) from e (or at least logging type(e).__name__ at debug) so users can tell whether the failure was a missing local path, an invalid HF repo id, permission error, etc.

Suggested change
except (OSError, HFValidationError):
adapter_config_file = None
if not adapter_config_file or not Path(adapter_config_file).is_file():
raise ConfigFileNotFoundError(
f"adapter_config.json not found for adapter_model: "
f"'{self.adapter_model}'. Ensure this path points to a valid "
f"LoRA adapter directory containing adapter_config.json."
except (OSError, HFValidationError) as e:
raise ConfigFileNotFoundError(
f"adapter_config.json not found for adapter_model: "
f"'{self.adapter_model}'. Ensure this path points to a valid "
f"LoRA adapter directory containing adapter_config.json."
) from e
if not Path(adapter_config_file).is_file():
raise ConfigFileNotFoundError(
f"adapter_config.json not found for adapter_model: "
f"'{self.adapter_model}'. Ensure this path points to a valid "
f"LoRA adapter directory containing adapter_config.json."

Copilot uses AI. Check for mistakes.
Comment on lines +324 to +325
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error text says "Ensure this path points to a valid LoRA adapter directory" but adapter_model can also be a Hugging Face Hub repo id (and failures can be auth/network-related). Consider wording this as "local directory or HF Hub repo id containing adapter_config.json" to avoid misleading guidance.

Suggested change
f"'{self.adapter_model}'. Ensure this path points to a valid "
f"LoRA adapter directory containing adapter_config.json."
f"'{self.adapter_model}'. Ensure this value is a local directory "
f"or HF Hub repo id containing adapter_config.json and that it is "
f"accessible (e.g., correct name, permissions, and network)."

Copilot uses AI. Check for mistakes.
)
Comment on lines +322 to +326
Copy link
Copy Markdown

@sentry sentry bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The code raises ConfigFileNotFoundError, which is not defined, causing a NameError at runtime. The correct exception class is OumiConfigFileNotFoundError.
Severity: CRITICAL

Suggested Fix

Replace the undefined ConfigFileNotFoundError on line 322 with the correct, imported exception class OumiConfigFileNotFoundError. Also, update the corresponding test cases in tests/unit/core/configs/params/test_model_params.py to expect OumiConfigFileNotFoundError.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/oumi/core/configs/params/model_params.py#L322-L326

Potential issue: The validation logic for `adapter_model` raises
`ConfigFileNotFoundError` if an `adapter_config.json` file is not found. However, this
exception class is not defined or imported. The correct, imported exception class is
`OumiConfigFileNotFoundError`, which is used in a similar check elsewhere in the same
file. This will cause the application to crash with a `NameError: name
'ConfigFileNotFoundError' is not defined` instead of providing the intended
user-friendly error message. The associated tests also incorrectly try to catch the
undefined exception.

Did we get this right? 👍 / 👎 to inform future reviews.


# Check if flash-attention-2 is requested and supported
if (self.attn_implementation == "flash_attention_2") and (
Expand Down
50 changes: 47 additions & 3 deletions tests/unit/core/configs/params/test_model_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,22 @@
from unittest.mock import call, patch

import pytest
from huggingface_hub.errors import HFValidationError

from oumi.core.configs.params.model_params import ModelParams
from oumi.exceptions import OumiConfigError, OumiConfigFileNotFoundError


def test_post_init_adapter_model_present():
params = ModelParams(model_name="base_model", adapter_model="adapter_model")
def test_post_init_adapter_model_present(tmp_path: Path):
adapter_dir = tmp_path / "my_adapter"
adapter_dir.mkdir()
(adapter_dir / "adapter_config.json").write_text('{"r": 16}')

params = ModelParams(model_name="base_model", adapter_model=str(adapter_dir))
params.finalize_and_validate()

assert params.model_name == "base_model"
assert params.adapter_model == "adapter_model"
assert params.adapter_model == str(adapter_dir)


def test_post_init_adapter_model_not_present(tmp_path: Path):
Expand Down Expand Up @@ -93,6 +98,45 @@ def test_post_init_config_file_empty(mock_logger, tmp_path: Path):
params.finalize_and_validate()


def test_explicit_adapter_model_nonexistent_path():
"""Explicitly-set adapter_model that doesn't exist raises error."""
params = ModelParams(
model_name="base_model", adapter_model="/nonexistent/adapter_path"
Comment on lines +101 to +104
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test hardcodes an absolute "/nonexistent/..." path. To avoid brittleness across environments (and to keep tests hermetic), prefer using tmp_path / "does_not_exist" (without creating it) as the nonexistent adapter path.

Suggested change
def test_explicit_adapter_model_nonexistent_path():
"""Explicitly-set adapter_model that doesn't exist raises error."""
params = ModelParams(
model_name="base_model", adapter_model="/nonexistent/adapter_path"
def test_explicit_adapter_model_nonexistent_path(tmp_path: Path):
"""Explicitly-set adapter_model that doesn't exist raises error."""
params = ModelParams(
model_name="base_model", adapter_model=str(tmp_path / "does_not_exist")

Copilot uses AI. Check for mistakes.
)
with pytest.raises(
ConfigFileNotFoundError,
match="adapter_config.json not found for adapter_model",
):
params.finalize_and_validate()


def test_explicit_adapter_model_missing_adapter_config(tmp_path: Path):
"""Adapter dir exists but has no adapter_config.json."""
adapter_dir = tmp_path / "incomplete_adapter"
adapter_dir.mkdir()

params = ModelParams(model_name="base_model", adapter_model=str(adapter_dir))
with pytest.raises(
ConfigFileNotFoundError,
match="adapter_config.json not found for adapter_model",
):
params.finalize_and_validate()


@patch("oumi.core.configs.params.model_params.find_adapter_config_file")
def test_explicit_adapter_model_hf_validation_error(mock_find):
"""HFValidationError during adapter_model validation raises error."""
mock_find.side_effect = HFValidationError("Invalid repo id")
params = ModelParams(
model_name="base_model", adapter_model="not///a///valid///repo"
)
with pytest.raises(
ConfigFileNotFoundError,
match="adapter_config.json not found for adapter_model",
):
params.finalize_and_validate()


def _get_invalid_field_name_lists() -> list[list[str]]:
all_fields: set[str] = {f.name for f in dataclasses.fields(ModelParams())}
result = [[field_name] for field_name in all_fields]
Expand Down
Loading