[Fix] Add dataclass fallback for ResourceDictValidation when pydantic is missing#939
Conversation
Modified src/executorlib/standalone/validate.py to use a Python dataclass as a fallback when pydantic is not installed. The class definition is shared between both cases. Updated tests to verify the fallback behavior. 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
📝 WalkthroughWalkthroughAdds runtime detection of pydantic and a fallback path: defines Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #939 +/- ##
==========================================
+ Coverage 94.07% 94.10% +0.03%
==========================================
Files 39 39
Lines 2041 2053 +12
==========================================
+ Hits 1920 1932 +12
Misses 121 121 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/executorlib/standalone/validate.py`:
- Around line 39-46: The fallback in _get_accepted_keys currently returns []
which lets validate_resource_dict_with_optional_keys silently accept all keys;
change the function to raise a RuntimeError (or assert) when
ResourceDictValidation exposes none of model_fields, __fields__, or
__dataclass_fields__, e.g. raise RuntimeError("ResourceDictValidation has no
accessible field metadata") so callers like
validate_resource_dict_with_optional_keys fail fast and surface the unexpected
state instead of stripping every key.
- Around line 35-36: The dataclass fallback when HAS_PYDANTIC is false
(ResourceDictValidation) does not perform field-type validation, so
validate_resource_dict({"cores": "not_an_int"}) can silently succeed; update the
code by adding a clear docstring on validate_resource_dict (or on the
ResourceDictValidation class) stating that when pydantic is unavailable the
dataclass fallback only enforces parameter names and not field types, and
recommend callers either enable pydantic, perform manual type checks, or
validate types upstream; keep the note brief and mention HAS_PYDANTIC,
ResourceDictValidation, and validate_resource_dict by name so callers can find
the behavior.
- Around line 49-50: validate_resource_dict currently lets different exception
types leak (pydantic.ValidationError when Pydantic is used, TypeError from the
dataclass fallback), forcing callers to handle multiple exception classes;
update validate_resource_dict to normalise errors by catching
pydantic.ValidationError and the dataclass/TypeError path and re-raising a
consistent exception (e.g., ValueError) with a clear message and original
exception chained (use "from" to preserve context). Reference the
ResourceDictValidation construction inside validate_resource_dict and ensure
both failure branches wrap/convert their exceptions to the chosen unified
exception type.
- Around line 4-5: Replace the Pydantic v1 config pattern: change the import to
use ConfigDict (e.g. from pydantic import BaseModel, ConfigDict) and remove any
inner class Config definitions in the BaseModel subclasses in this file; instead
add model_config = ConfigDict(extra="ignore") (or extra=Extra.ignore if you
still import Extra) on those model classes so the code uses Pydantic v2
configuration; keep _get_accepted_keys() using model_fields as-is.
| try: | ||
| from pydantic import BaseModel, Extra |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
pydantic v2 ConfigDict replace Extra Config class
💡 Result:
In Pydantic v2, the old inner class Config: (and Extra) is replaced by model_config = ConfigDict(...) (the Config class is still supported but deprecated). (docs.pydantic.dev)
v1 → v2 mapping for extra
v1
from pydantic import BaseModel, Extra
class M(BaseModel):
class Config:
extra = Extra.forbid # or allow / ignorev2 (recommended)
from pydantic import BaseModel, ConfigDict
class M(BaseModel):
model_config = ConfigDict(extra='forbid') # or 'allow' / 'ignore'Pydantic v2 supports exactly these values: 'ignore' (default), 'allow', 'forbid'. (docs.pydantic.dev)
Alternative v2 syntax (class kwargs)
from pydantic import BaseModel
class M(BaseModel, extra='forbid'):
...If you used extra='allow' and need the extras
In v2, allowed extras are stored in __pydantic_extra__ (and may also be accessible as attributes). (docs.pydantic.dev)
Citations:
- 1: https://docs.pydantic.dev/2.0/usage/model_config/?utm_source=openai
- 2: https://docs.pydantic.dev/2.0/usage/model_config/?utm_source=openai
- 3: https://docs.pydantic.dev/2.0/usage/model_config/?utm_source=openai
- 4: https://docs.pydantic.dev/dev/api/config/?utm_source=openai
🏁 Script executed:
head -50 src/executorlib/standalone/validate.pyRepository: pyiron/executorlib
Length of output: 1465
Use Pydantic V2's ConfigDict instead of deprecated Extra and inner Config class
The code imports Pydantic V1 APIs (Extra and inner class Config) which are deprecated in Pydantic V2. With pydantic 2.12.4, these patterns emit PydanticDeprecatedSince20 warnings on every module load. Since _get_accepted_keys() already uses the V2 model_fields attribute, the codebase is V2-aware but the configuration pattern hasn't been modernized.
Replace with Pydantic V2's native ConfigDict:
♻️ Refactor to V2 API
-try:
- from pydantic import BaseModel, Extra
-
- HAS_PYDANTIC = True
-except ImportError:
+try:
+ from pydantic import BaseModel, ConfigDict
+
+ HAS_PYDANTIC = True
+except ImportError:
from dataclasses import dataclass
BaseModel = object
- Extra = None
+ ConfigDict = None
HAS_PYDANTIC = False if HAS_PYDANTIC:
-
- class Config:
- extra = Extra.forbid
+ model_config = ConfigDict(extra="forbid")Also applies to: lines 29–32.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/executorlib/standalone/validate.py` around lines 4 - 5, Replace the
Pydantic v1 config pattern: change the import to use ConfigDict (e.g. from
pydantic import BaseModel, ConfigDict) and remove any inner class Config
definitions in the BaseModel subclasses in this file; instead add model_config =
ConfigDict(extra="ignore") (or extra=Extra.ignore if you still import Extra) on
those model classes so the code uses Pydantic v2 configuration; keep
_get_accepted_keys() using model_fields as-is.
| if not HAS_PYDANTIC: | ||
| ResourceDictValidation = dataclass(ResourceDictValidation) |
There was a problem hiding this comment.
Dataclass fallback skips type validation
When pydantic is absent, validate_resource_dict({"cores": "not_an_int"}) silently succeeds — the dataclass constructor accepts any value for typed fields. Only unexpected keyword names raise TypeError. This is a weaker guarantee than pydantic's field-type validation. Consider adding a docstring note so callers are aware.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/executorlib/standalone/validate.py` around lines 35 - 36, The dataclass
fallback when HAS_PYDANTIC is false (ResourceDictValidation) does not perform
field-type validation, so validate_resource_dict({"cores": "not_an_int"}) can
silently succeed; update the code by adding a clear docstring on
validate_resource_dict (or on the ResourceDictValidation class) stating that
when pydantic is unavailable the dataclass fallback only enforces parameter
names and not field types, and recommend callers either enable pydantic, perform
manual type checks, or validate types upstream; keep the note brief and mention
HAS_PYDANTIC, ResourceDictValidation, and validate_resource_dict by name so
callers can find the behavior.
jan-janssen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/executorlib/standalone/validate.py`:
- Around line 49-50: The no-op validate_resource_dict in task_scheduler.base is
still present and causing other modules to import the stub instead of the real
validator; remove the empty function from task_scheduler.base and update all
modules that import validate_resource_dict (e.g., dependency.py,
blockallocation.py, onetoone.py, file/task_scheduler.py, spawner_pysqa.py) to
import the real implementation from standalone.validate (the
validate_resource_dict in standalone/validate.py which instantiates
ResourceDictValidation) or, if you intend to keep a documented fallback, replace
the stub with a delegating import that explicitly re-exports the real function
and add a comment documenting the intentional fallback. Ensure references to
validate_resource_dict resolve to the standalone.validate function name to avoid
the no-op.
---
Duplicate comments:
In `@src/executorlib/standalone/validate.py`:
- Around line 39-46: The function _get_accepted_keys currently returns an empty
list when ResourceDictValidation lacks model_fields, __fields__, and
__dataclass_fields__, silently disabling validation; change it to raise a
RuntimeError (or assert) instead, e.g., in _get_accepted_keys() after the three
hasattr checks raise a RuntimeError with a clear message referencing
ResourceDictValidation and that no field metadata was found so validation cannot
proceed; this ensures callers of _get_accepted_keys (and any validation paths
relying on it) fail loudly rather than silently accepting all keys.
- Around line 49-50: validate_resource_dict currently lets
ResourceDictValidation (a pydantic model) raise its own exception type;
normalize exceptions by catching pydantic.error_wrappers.ValidationError (or
BaseModel validation error) inside validate_resource_dict and re-raise a single,
consistent exception type (e.g., ValueError) with a clear message and original
error attached (or str() of it); update validate_resource_dict to wrap the
ResourceDictValidation(**resource_dict) call in a try/except that converts the
pydantic validation error to ValueError so both pydantic and dataclass
validation paths produce the same exception type.
- Around line 4-13: The code currently imports Extra and relies on an inner
class Config (deprecated in pydantic v2); update the pydantic branch to import
ConfigDict from pydantic (e.g. from pydantic import BaseModel, ConfigDict) and
replace any inner class Config definitions on your models with a module-level or
class attribute using model_config = ConfigDict(extra="forbid"). Keep the
fallback behavior for when HAS_PYDANTIC is False (BaseModel = object,
HAS_PYDANTIC = False) but remove the Extra import and any usage of Extra; ensure
any model classes referenced in this file (e.g., subclasses of BaseModel) use
model_config = ConfigDict(...) instead of class Config.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/executorlib/standalone/validate.py (1)
29-32: Conditional class body using a module-level flag is fragile and non-obvious.The
if HAS_PYDANTIC:inside the class body works because it executes at class-definition time, but it's an unusual pattern that may confuse readers and static analysis tools. If you adopt theConfigDictapproach from the prior review feedback, this conditional block collapses to a singlemodel_config = ...line guarded by the same flag, which is easier to follow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/executorlib/standalone/validate.py` around lines 29 - 32, Replace the inline conditional class body (the conditional "class Config: extra = Extra.forbid" guarded by HAS_PYDANTIC) with a module-level model_config assignment when pydantic is available: remove the inner "class Config" block and, if HAS_PYDANTIC is true, set model_config = ConfigDict({"extra": Extra.forbid}) or model_config = ConfigDict(extra=Extra.forbid) (importing ConfigDict/Extra from pydantic as needed); keep no conditional class definitions so callers use model_config consistently and static analysis/readers won't be surprised by a class-body conditional.
🤖 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/executorlib/standalone/validate.py`:
- Around line 47-48: The project defines a no-op validate_resource_dict in
task_scheduler.base that shadows the real validator in standalone.validate
(which constructs ResourceDictValidation(**resource_dict)), causing five modules
(blockallocation.py, dependency.py, spawner_pysqa.py, onetoone.py,
task_scheduler.py) to skip validation; remove the no-op from task_scheduler.base
and update those modules to import validate_resource_dict from
src.executorlib.standalone.validate (or directly call ResourceDictValidation in
their flow) so all callers use the real validation implementation (keep the
existing correct implementation in standalone.validate.validate_resource_dict
which calls ResourceDictValidation).
---
Duplicate comments:
In `@src/executorlib/standalone/validate.py`:
- Around line 4-13: Summary: Importing Extra from pydantic triggers deprecation
warnings for pydantic ≥2.0; migrate to using ConfigDict instead. Fix: in
validate.py replace the try/except import block so you import BaseModel and
ConfigDict (not Extra) from pydantic and set HAS_PYDANTIC = True when available;
remove or stop using the Extra symbol and any inner class Config patterns in
code that consumes BaseModel; in the except branch provide safe fallbacks (e.g.,
BaseModel = object, ConfigDict = dict or None) and keep HAS_PYDANTIC = False so
downstream code can check and adapt. Reference symbols: BaseModel, Extra,
HAS_PYDANTIC, ConfigDict.
---
Nitpick comments:
In `@src/executorlib/standalone/validate.py`:
- Around line 29-32: Replace the inline conditional class body (the conditional
"class Config: extra = Extra.forbid" guarded by HAS_PYDANTIC) with a
module-level model_config assignment when pydantic is available: remove the
inner "class Config" block and, if HAS_PYDANTIC is true, set model_config =
ConfigDict({"extra": Extra.forbid}) or model_config =
ConfigDict(extra=Extra.forbid) (importing ConfigDict/Extra from pydantic as
needed); keep no conditional class definitions so callers use model_config
consistently and static analysis/readers won't be surprised by a class-body
conditional.
| def validate_resource_dict(resource_dict: dict) -> None: | ||
| _ = ResourceDictValidation(**resource_dict) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the no-op validate_resource_dict still exists in task_scheduler/base.py
echo "=== task_scheduler/base.py validate_resource_dict ==="
rg -n 'def validate_resource_dict' --type=py -C 3
echo ""
echo "=== Imports of validate_resource_dict from task_scheduler ==="
rg -n 'from.*task_scheduler.*import.*validate_resource_dict' --type=py
echo ""
echo "=== All imports from task_scheduler.base ==="
rg -n 'from.*task_scheduler\.base.*import' --type=py
echo ""
echo "=== All imports from standalone.validate ==="
rg -n 'from.*standalone\.validate.*import' --type=py
echo ""
echo "=== Check task_scheduler/base.py content around line 16-17 ==="
sed -n '1,30p' src/executorlib/task_scheduler/base.py 2>/dev/null || echo "File not found at expected location"Repository: pyiron/executorlib
Length of output: 4644
Remove the no-op validate_resource_dict from task_scheduler/base.py (lines 17-18).
The no-op implementation persists and creates a shadowing problem. Five modules—blockallocation.py, dependency.py, spawner_pysqa.py, onetoone.py, and task_scheduler.py—import the no-op from task_scheduler.base and will silently skip validation. Only the three executor modules (flux.py, single.py, slurm.py) use the proper implementation from standalone.validate. Either remove the no-op entirely or update all affected imports to use standalone.validate.validate_resource_dict.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/executorlib/standalone/validate.py` around lines 47 - 48, The project
defines a no-op validate_resource_dict in task_scheduler.base that shadows the
real validator in standalone.validate (which constructs
ResourceDictValidation(**resource_dict)), causing five modules
(blockallocation.py, dependency.py, spawner_pysqa.py, onetoone.py,
task_scheduler.py) to skip validation; remove the no-op from task_scheduler.base
and update those modules to import validate_resource_dict from
src.executorlib.standalone.validate (or directly call ResourceDictValidation in
their flow) so all callers use the real validation implementation (keep the
existing correct implementation in standalone.validate.validate_resource_dict
which calls ResourceDictValidation).
1 participant
This change introduces a fallback mechanism for resource dictionary validation in
executorlib.standalone.validate.Key changes:
ResourceDictValidationnow inherits frompydantic.BaseModelif available, otherwise it falls back to a standard Pythondataclass._get_accepted_keys()was added to handle field introspection for Pydantic V1, V2, and dataclasses.tests/unit/standalone/test_validate.pywere updated to reflect thatstandalone.validateis now always used instead of falling back to a no-op intask_scheduler.base.TypeError) and handles optional keys with warnings.pydanticinstalled.PR created automatically by Jules for task 6201971495027274614 started by @jan-janssen
Summary by CodeRabbit
New Features
Bug Fixes