Add Pydantic models for configuration validation#510
Add Pydantic models for configuration validation#510jan-janssen wants to merge 2 commits intomainfrom
Conversation
- Introduced `QueueModel` and `ConfigModel` in `pysqa/base/config.py` using Pydantic v2. - Integrated validation into `QueueAdapterWithConfig` initialization. - Added `pydantic>=2.0` to project dependencies. - Updated `_load_templates` to handle queues without a submission script (e.g., remote queues). - Added unit tests for configuration validation. Co-authored-by: jan-janssen <3854739+jan-janssen@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughThe PR adds a Pydantic v2 dependency and introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pyproject.toml (1)
34-39: Inconsistent version pinning strategy.Other runtime dependencies (
jinja2,pandas,pyyaml) use exact version pinning (==), whilepydantic>=2.0uses a minimum version constraint. This is acceptable if intentional (to allow Pydantic v2 minor/patch updates), but consider documenting this choice or aligning with the existing pinning strategy for reproducibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 34 - 39, The dependencies list currently mixes exact pins and a minimum constraint: update the entry for "pydantic>=2.0" to match the project's pinning strategy or document the exception; specifically either change the dependency in the dependencies array to an exact version (e.g., "pydantic==2.x.y") to match jinja2/pandas/pyyaml, or add a brief comment in the pyproject.toml (or project README) next to the dependencies list explaining why pydantic is intentionally left as "pydantic>=2.0" to allow minor/patch upgrades for the Pydantic package.src/pysqa/base/config.py (2)
28-37: Consider validatingqueue_typeagainst allowed values.The
queue_typefield accepts any string, but valid values are constrained to"SGE","TORQUE","SLURM","LSF","MOAB","FLUX", and"REMOTE"(perset_queue_adapterinqueueadapter.py). UsingLiteralwould catch invalid queue types earlier with a clearer error message.💡 Suggested improvement
-from pydantic import BaseModel, ConfigDict +from typing import Literal +from pydantic import BaseModel, ConfigDict +QUEUE_TYPES = Literal["SGE", "TORQUE", "SLURM", "LSF", "MOAB", "FLUX", "REMOTE"] class ConfigModel(BaseModel): model_config = ConfigDict(extra="allow") - queue_type: str + queue_type: QUEUE_TYPES queue_primary: Optional[str] = None queues: dict[str, QueueModel]🤖 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 28 - 37, The ConfigModel currently allows any string for queue_type; change queue_type to a constrained literal type to validate allowed values early (e.g., use typing.Literal or typing_extensions.Literal and set queue_type: Literal["SGE","TORQUE","SLURM","LSF","MOAB","FLUX","REMOTE"]). Update the import(s) as needed and ensure ConfigModel (in src/pysqa/base/config.py) uses this Literal so invalid values are caught by Pydantic; this aligns with the allowed queue types enforced by set_queue_adapter in queueadapter.py.
338-350: Minor: Redundant key check after Pydantic validation.After
model_dump(), the"script"key will always exist inqueue_dict(defaulting toNoneif not provided in the original config). The"script" in queue_dictcheck is now redundant and can be simplified.✨ Simplified condition
for queue_dict in queue_lst_dict.values(): - if "script" in queue_dict and queue_dict["script"] is not None: + if queue_dict.get("script") is not None: with open(os.path.join(directory, queue_dict["script"])) as f:Note: The current code is functionally correct and provides defensive coding if this method is ever called with unvalidated data.
🤖 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 338 - 350, The check for the presence of the "script" key is redundant after Pydantic's model_dump(); update the loop over queue_lst_dict so you only test for a non-None script value (e.g., replace the current if "script" in queue_dict and queue_dict["script"] is not None: with a single check for queue_dict["script"] is not None), then proceed to open os.path.join(directory, queue_dict["script"]) and compile the Template and handle TemplateSyntaxError as before (references: queue_lst_dict, queue_dict["script"], Template, TemplateSyntaxError, model_dump()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pysqa/base/config.py`:
- Around line 103-106: The constructor currently calls ConfigModel(**config)
which raises pydantic.ValidationError on invalid input; wrap that call in a
try/except that catches pydantic.ValidationError and re-raises a ValueError
(including the original error message) before continuing to set self._config and
call super().__init__; reference the call to ConfigModel and the surrounding
code that assigns self._config and calls super().__init__ so the conversion
happens before queue_type is read.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 34-39: The dependencies list currently mixes exact pins and a
minimum constraint: update the entry for "pydantic>=2.0" to match the project's
pinning strategy or document the exception; specifically either change the
dependency in the dependencies array to an exact version (e.g.,
"pydantic==2.x.y") to match jinja2/pandas/pyyaml, or add a brief comment in the
pyproject.toml (or project README) next to the dependencies list explaining why
pydantic is intentionally left as "pydantic>=2.0" to allow minor/patch upgrades
for the Pydantic package.
In `@src/pysqa/base/config.py`:
- Around line 28-37: The ConfigModel currently allows any string for queue_type;
change queue_type to a constrained literal type to validate allowed values early
(e.g., use typing.Literal or typing_extensions.Literal and set queue_type:
Literal["SGE","TORQUE","SLURM","LSF","MOAB","FLUX","REMOTE"]). Update the
import(s) as needed and ensure ConfigModel (in src/pysqa/base/config.py) uses
this Literal so invalid values are caught by Pydantic; this aligns with the
allowed queue types enforced by set_queue_adapter in queueadapter.py.
- Around line 338-350: The check for the presence of the "script" key is
redundant after Pydantic's model_dump(); update the loop over queue_lst_dict so
you only test for a non-None script value (e.g., replace the current if "script"
in queue_dict and queue_dict["script"] is not None: with a single check for
queue_dict["script"] is not None), then proceed to open os.path.join(directory,
queue_dict["script"]) and compile the Template and handle TemplateSyntaxError as
before (references: queue_lst_dict, queue_dict["script"], Template,
TemplateSyntaxError, model_dump()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9676030-bfd4-4101-a875-249e1a5cd7f0
📒 Files selected for processing (3)
pyproject.tomlsrc/pysqa/base/config.pytests/unit/base/test_config.py
| self._config = ConfigModel(**config).model_dump() | ||
| super().__init__( | ||
| queue_type=config["queue_type"], execute_command=execute_command | ||
| queue_type=self._config["queue_type"], execute_command=execute_command | ||
| ) |
There was a problem hiding this comment.
Pydantic ValidationError is not converted to ValueError.
The unit tests expect ValueError for validation failures, but ConfigModel(**config) raises pydantic.ValidationError. Either wrap the validation in a try/except block to convert the exception, or update the tests to catch ValidationError.
🔧 Option 1: Wrap validation to raise ValueError
+from pydantic import BaseModel, ConfigDict, ValidationError
...
def __init__(
self,
config: dict,
directory: str = "~/.queues",
execute_command: Callable = execute_command,
):
- self._config = ConfigModel(**config).model_dump()
+ try:
+ self._config = ConfigModel(**config).model_dump()
+ except ValidationError as e:
+ raise ValueError(f"Invalid configuration: {e}") from e
super().__init__(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self._config = ConfigModel(**config).model_dump() | |
| super().__init__( | |
| queue_type=config["queue_type"], execute_command=execute_command | |
| queue_type=self._config["queue_type"], execute_command=execute_command | |
| ) | |
| try: | |
| self._config = ConfigModel(**config).model_dump() | |
| except ValidationError as e: | |
| raise ValueError(f"Invalid configuration: {e}") from e | |
| super().__init__( | |
| queue_type=self._config["queue_type"], execute_command=execute_command | |
| ) |
🤖 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 103 - 106, The constructor currently
calls ConfigModel(**config) which raises pydantic.ValidationError on invalid
input; wrap that call in a try/except that catches pydantic.ValidationError and
re-raises a ValueError (including the original error message) before continuing
to set self._config and call super().__init__; reference the call to ConfigModel
and the surrounding code that assigns self._config and calls super().__init__ so
the conversion happens before queue_type is read.
1 participant
This change introduces Pydantic models to validate the configuration files (typically
queue.yaml) used bypysqa.Key changes:
QueueModeldefines the schema for individual queue configurations, including optional fields for resource limits and submission scripts.ConfigModelvalidates the top-level configuration structure.QueueAdapterWithConfignow validates its input configuration upon initialization.ConfigDict(extra='allow')is used to maintain backward compatibility with custom template variables and adapter-specific settings.pydanticas a core dependency inpyproject.toml.scriptfield.PR created automatically by Jules for task 8214308980248924915 started by @jan-janssen
Summary by CodeRabbit
Release Notes
Chores
New Features
Bug Fixes