Validate explicitly-provided adapter_model path in ModelParams#2321
Validate explicitly-provided adapter_model path in ModelParams#2321idoudali wants to merge 1 commit intoidoudali/config-exceptionsfrom
Conversation
14e626c to
35745d5
Compare
There was a problem hiding this comment.
Pull request overview
This PR makes ModelParams.finalize_and_validate() fail fast when an explicitly provided adapter_model doesn’t resolve to an adapter_config.json, raising ConfigFileNotFoundError to avoid later, more confusing model-loading failures.
Changes:
- Add explicit validation of
adapter_modelviafind_adapter_config_file()and raiseConfigFileNotFoundErrorwhen not found. - Update and extend unit tests to cover valid explicit adapter dirs and multiple failure modes (missing path, missing config, HF validation error).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/oumi/core/configs/params/model_params.py |
Adds explicit adapter_model validation and raises ConfigFileNotFoundError when adapter_config.json can’t be resolved. |
tests/unit/core/configs/params/test_model_params.py |
Updates existing adapter_model test to use a real temp adapter dir and adds new negative tests for explicit adapter_model validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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." |
There was a problem hiding this comment.
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.
| 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." |
| f"'{self.adapter_model}'. Ensure this path points to a valid " | ||
| f"LoRA adapter directory containing adapter_config.json." |
There was a problem hiding this comment.
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.
| 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)." |
| 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" |
There was a problem hiding this comment.
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.
| 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") |
ddc2b75 to
bd15c23
Compare
35745d5 to
9c851f3
Compare
bd15c23 to
b2ddb95
Compare
9c851f3 to
e5f549f
Compare
Fail fast with ConfigFileNotFoundError when adapter_model is set but doesn't point to a directory containing adapter_config.json, preventing confusing downstream errors during model loading. Fixes OPE-1851
b2ddb95 to
5237a20
Compare
| 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." | ||
| ) |
There was a problem hiding this comment.
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.
Depends on #2319
Description
Fail fast with ConfigFileNotFoundError when adapter_model is set but doesn't point to a directory containing adapter_config.json, preventing confusing downstream errors during model loading.
Fixes OPE-1851
Related issues
Fixes # (issue)
Before submitting
Reviewers
At least one review from a member of
oumi-ai/oumi-staffis required.