-
Notifications
You must be signed in to change notification settings - Fork 8
Add Pydantic models for configuration validation #510
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
19aa45f
443d16a
49eb154
98867e5
a5b23a0
4c9b071
0291f12
b4d01c5
747e1d3
e4448ba
6345890
7399922
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,3 +10,4 @@ dependencies: | |
| - tqdm =4.67.3 | ||
| - hatchling =1.29.0 | ||
| - hatch-vcs =0.5.0 | ||
| - pydantic =2.12.5 | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,13 @@ | |||||||||||||||||||||||||||||||
| from pysqa.base.core import QueueAdapterCore, execute_command | ||||||||||||||||||||||||||||||||
| from pysqa.base.validate import check_queue_parameters, value_error_if_none | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||
| from pysqa.base.models import validate_config | ||||||||||||||||||||||||||||||||
| except ImportError: | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| def validate_config(config: dict) -> dict: | ||||||||||||||||||||||||||||||||
| return config | ||||||||||||||||||||||||||||||||
|
Comment on lines
+14
to
+17
|
||||||||||||||||||||||||||||||||
| except ImportError: | |
| def validate_config(config: dict) -> dict: | |
| return config | |
| except ModuleNotFoundError as e: | |
| # Handle the optional dependency on pydantic explicitly; re-raise other import errors. | |
| if e.name == "pydantic": | |
| def validate_config(config: dict) -> dict: | |
| raise ModuleNotFoundError( | |
| "Configuration validation requires 'pydantic'. " | |
| "Install the appropriate extra (e.g. 'pysqa[validation]') to enable validation." | |
| ) from e | |
| else: | |
| raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify dependency declaration and locate all validation fallback/import patterns.
fd 'pyproject.toml' --exec sed -n '1,260p' {}
rg -nP "pydantic|from pysqa\\.base\\.models import validate_config|except ImportError|except ModuleNotFoundError" -C2Repository: pyiron/pysqa
Length of output: 5952
🏁 Script executed:
# Find all usages of validate_config to understand where it's called
rg -n "validate_config" -B2 -A2
# Check if pydantic is conditionally required or truly optional
rg -n "pydantic" --type python
# Look at the actual validation function in models.py
cat -n src/pysqa/base/models.pyRepository: pyiron/pysqa
Length of output: 2792
🏁 Script executed:
# Search for any documentation or comments about optional pydantic
rg -n "optional|pydantic" -i --type-list | head -5
cat README.md | head -100
# Look for any conditional imports or environment checks related to pydantic
rg -n "pydantic" -B3 -A3
# Check if config is accessed with square bracket notation after validate_config
sed -n '80,90p' src/pysqa/base/config.pyRepository: pyiron/pysqa
Length of output: 10580
🏁 Script executed:
# Look at test_config.py to see if it handles missing pydantic
cat -n tests/unit/base/test_config.py | head -50
# Check if there are any skip decorators or conditional imports in test_config
rg -n "skip|ImportError|pydantic" tests/unit/base/test_config.py -B2 -A2Repository: pyiron/pysqa
Length of output: 2038
Replace broad ImportError catch with specific error handling for missing pydantic.
Catching ImportError without specificity silently disables config validation when pydantic is unavailable, deferring failures to KeyError at line 87 instead of providing a clear validation/installation error. The tests (test_pydantic_validation_missing_queues, test_pydantic_validation_wrong_type) demonstrate that validation is core functionality and pydantic is not truly optional despite being listed in optional-dependencies.
Proposed fix
try:
from pysqa.base.models import validate_config
-except ImportError:
-
- def validate_config(config: dict) -> dict:
- return config
+except ModuleNotFoundError as exc:
+ if exc.name == "pydantic":
+ raise RuntimeError(
+ "pydantic is required for configuration validation."
+ ) from exc
+ raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pysqa/base/config.py` around lines 12 - 17, The current broad ImportError
catch around "from pysqa.base.models import validate_config" suppresses all
import errors and hides missing-pydantic issues; instead, if the import raises
because pydantic is not present, raise a clear error advising to install
pydantic; for other import failures re-raise the original exception. Update the
except block that defines the fallback validate_config to inspect the caught
ImportError (or ModuleNotFoundError) and if the missing module is "pydantic"
raise a descriptive RuntimeError/ImportError explaining pydantic is required for
validate_config (mentioning validate_config and pysqa.base.models), otherwise
re-raise the original exception so unrelated import problems are not silently
ignored.
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,59 @@ | ||||||||||
| from typing import Optional, Union | ||||||||||
|
|
||||||||||
| from pydantic import BaseModel, ConfigDict | ||||||||||
|
|
||||||||||
|
|
||||||||||
| class QueueModel(BaseModel): | ||||||||||
| """ | ||||||||||
| Pydantic model for a single queue configuration. | ||||||||||
| """ | ||||||||||
|
|
||||||||||
| model_config = ConfigDict(extra="allow") | ||||||||||
|
|
||||||||||
| script: Optional[str] = None | ||||||||||
| cores_min: Optional[int] = None | ||||||||||
| cores_max: Optional[int] = None | ||||||||||
| run_time_max: Optional[int] = None | ||||||||||
| memory_max: Optional[Union[int, str]] = None | ||||||||||
|
||||||||||
| memory_max: Optional[Union[int, str]] = None | |
| memory_max: Optional[Union[int, float, str]] = None |
Copilot
AI
Mar 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The defaults for ssh_continous_connection and ssh_delete_file_on_remote don’t match the existing runtime defaults in RemoteQueueAdapter (currently defaults to False and True respectively via config.get(...)). Because validate_config() uses model_dump() without excluding defaults, these values can be injected into qa.config even when the user didn’t specify them, creating confusing/incorrect apparent configuration. Align the defaults with current behavior and/or exclude defaults in model_dump().
| ssh_continous_connection: bool = True | |
| ssh_delete_file_on_remote: bool = False | |
| ssh_continous_connection: bool = False | |
| ssh_delete_file_on_remote: bool = True |
Copilot
AI
Mar 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate_config() returns model_dump() with default settings, which includes fields set to None/defaults (e.g., queue entries get script: None). This changes the shape of config compared to what was loaded from YAML and is what necessitated extra is not None checks elsewhere. Consider using model_dump(exclude_none=True, exclude_defaults=True) (or exclude_unset=True) to preserve the original config structure while still validating/coercing types.
| return ConfigModel(**config).model_dump() | |
| return ConfigModel(**config).model_dump(exclude_none=True, exclude_defaults=True) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,61 @@ | ||
| import unittest | ||
| from pysqa.base.config import QueueAdapterWithConfig | ||
| import sys | ||
| from unittest.mock import patch | ||
|
|
||
|
|
||
| class TestConfig(unittest.TestCase): | ||
| def test_bad_queue_type(self): | ||
| from pysqa.base.config import QueueAdapterWithConfig | ||
|
|
||
| with self.assertRaises(ValueError): | ||
| QueueAdapterWithConfig(config={"queue_type": "error", "queues": {}}) | ||
|
|
||
| def test_pydantic_validation_missing_queues(self): | ||
| from pysqa.base.config import QueueAdapterWithConfig | ||
|
|
||
| with self.assertRaises(ValueError): | ||
| QueueAdapterWithConfig(config={"queue_type": "SLURM"}) | ||
|
|
||
| def test_pydantic_validation_wrong_type(self): | ||
| from pysqa.base.config import QueueAdapterWithConfig | ||
|
|
||
| with self.assertRaises(ValueError): | ||
| QueueAdapterWithConfig( | ||
| config={ | ||
| "queue_type": "SLURM", | ||
| "queues": {"sq": {"script": "slurm.sh", "cores_min": "not_an_int"}}, | ||
| } | ||
| ) | ||
|
|
||
| def test_pydantic_validation_extra_fields(self): | ||
| from pysqa.base.config import QueueAdapterWithConfig | ||
|
|
||
| config = { | ||
| "queue_type": "SLURM", | ||
| "queue_primary": "sq", | ||
| "queues": {"sq": {"extra_field": "allowed"}}, | ||
| "extra_top_level": "also_allowed", | ||
| } | ||
| qa = QueueAdapterWithConfig(config=config) | ||
| self.assertEqual(qa.config["queues"]["sq"]["extra_field"], "allowed") | ||
| self.assertEqual(qa.config["extra_top_level"], "also_allowed") | ||
|
|
||
| def test_no_pydantic_validation_extra_fields(self): | ||
| with patch.dict('sys.modules', {'pydantic': None}): | ||
| if 'pysqa.base.models' in sys.modules: | ||
| del sys.modules['pysqa.base.models'] | ||
|
|
||
| if 'pysqa.base.config' in sys.modules: | ||
| del sys.modules['pysqa.base.config'] | ||
|
|
||
| from pysqa.base.config import QueueAdapterWithConfig | ||
|
|
||
| config = { | ||
| "queue_type": "SLURM", | ||
| "queue_primary": "sq", | ||
| "queues": {"sq": {"extra_field": "allowed"}}, | ||
| "extra_top_level": "also_allowed", | ||
| } | ||
| qa = QueueAdapterWithConfig(config=config) | ||
| self.assertEqual(qa.config["queues"]["sq"]["extra_field"], "allowed") | ||
| self.assertEqual(qa.config["extra_top_level"], "also_allowed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI environment files pin
pydanticto different versions (2.5.3here vs2.12.5in the main/doc environments and inpyproject.toml). This can lead to inconsistent validation behavior and CI results across environments. Align the pinned versions (or use a compatible range) across all environment files.