Fix PEtab v2 extension config model - #506
Merged
Merged
Conversation
ProblemConfig.extensions was typed as `list[ExtensionConfig] | dict`, but only the dict branch was ever used or supported downstream. On top of that, the extension config models didn't match the v2 schema: neither ExtensionConfig nor SciMLConfig had the schema-mandated `required` field, and the generic ExtensionConfig wrongly nested extra keys under a `config` field instead of allowing them directly. Writing a problem with a `sciml` extension to YAML and reading it back therefore failed schema validation. - Add a shared `ExtensionConfig` base (version, required, extra fields allowed) in petab/v2/extensions/__init__.py, and make SciMLConfig subclass it (required defaults to True, since a SciML hybrid model is virtually always load-bearing). - Type `ProblemConfig.extensions` as `dict[str, ExtensionConfig]` with `SerializeAsAny` so subclass fields survive serialization. - Fix a broken import in SciMLConfig.to_yaml() that made it crash unconditionally. - Problem.validate() still only warns about unsupported extensions (rejecting based on `required` is left to consumers like simulators that actually interpret the extension mathematically). Closes PEtab-dev#474 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #506 +/- ##
==========================================
+ Coverage 75.30% 75.54% +0.23%
==========================================
Files 64 65 +1
Lines 7353 7359 +6
Branches 1327 1325 -2
==========================================
+ Hits 5537 5559 +22
+ Misses 1310 1296 -14
+ Partials 506 504 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
BSnelling
approved these changes
Aug 10, 2026
dilpath
approved these changes
Aug 11, 2026
Comment on lines
+2556
to
+2569
| parsed_extensions = {} | ||
| for ext_name, ext_config in v.items(): | ||
| if ext_name == C.EXT_ID_SCIML: | ||
| parsed_extensions[ext_name] = ( | ||
| ext_config | ||
| if isinstance(ext_config, SciMLConfig) | ||
| else SciMLConfig(**ext_config) | ||
| ) | ||
| else: | ||
| parsed_extensions[ext_name] = ( | ||
| ext_config | ||
| if isinstance(ext_config, ExtensionConfig) | ||
| else ExtensionConfig(**ext_config) | ||
| ) |
Member
There was a problem hiding this comment.
Hm, could all of this be moved to a function inside petab.v2.extensions so that there is no explicit dependency on specific extensions here?
| """The `sciml` extension config, once written to YAML via | ||
| `ProblemConfig.to_yaml()`, is schema-valid and can be read back. | ||
| """ | ||
| from petab.v1.yaml import load_yaml, validate_yaml_syntax |
Member
There was a problem hiding this comment.
Strange to see petab.v1 here but fine if intended.
Member
Author
There was a problem hiding this comment.
I am fine with moving them to a common module, but not in this PR.
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
- Revert extensions field_validator to raise ValueError instead of TypeError: pydantic only converts ValueError/AssertionError raised in a validator into a ValidationError, not TypeError, so the TypeError variant broke test_problem_config_extensions_rejects_non_dict in CI. - Move the per-extension-ID config dispatch (sciml vs. generic) out of ProblemConfig._parse_extensions into petab.v2.extensions.parse_extension_config, so core.py no longer needs to import SciMLConfig or check C.EXT_ID_SCIML directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ProblemConfig.extensionswas typed aslist[ExtensionConfig] | dict, but only the dict branch was ever used/supported downstream (Fix petab.v2.core.ProblemConfig.extensions #474).ExtensionConfignorSciMLConfighad the schema-mandatedrequiredfield, and the genericExtensionConfigwrongly nested extra keys under aconfigfield instead of allowing them directly alongsideversion/required. As a result, writing a problem with ascimlextension to YAML and reading it back failed schema validation.ExtensionConfigbase (version,required, arbitrary extra fields allowed) inpetab/v2/extensions/__init__.py;SciMLConfignow subclasses it (requireddefaults toTrue, since a SciML hybrid model is virtually always load-bearing for the problem's interpretation).ProblemConfig.extensionsis nowdict[str, ExtensionConfig](usingSerializeAsAnyso subclass-specific fields survive serialization).SciMLConfig.to_yaml()that made it crash unconditionally.Problem.validate()still only warns about unsupported extensions — rejecting a problem based on an extension'srequiredflag is left to consumers (e.g. simulators) that actually interpret the extension mathematically; libpetab-python itself doesn't.Closes #474
Test plan
python -m pytest tests/v2/— 106 passed, 1 skippedpre-commit run --all-fileson changed files — ruff/ruff-format cleansciml-extension problem throughProblemConfig.to_yaml()+ schema validationextensions, and unsupported-extension warning severity🤖 Generated with Claude Code