feat(flepimop2-op_system): adopt ModuleBase 0.3.0 API#142
Conversation
Closes #141. - Declare `module` as an explicit `Literal["flepimop2.system.op_system"]` field instead of using the `module="..."` class-keyword shortcut. - Switch the connector's `model_config` to `extra="forbid"`. - Add real class docstring (drop `# noqa: D101`). - Bump version to 0.3.0 and floor `flepimop2` to `>=0.3.0.dev0`. - Add regression tests for `ModuleBase.patch` REPLACE-mode recompile, type-mismatch guard, the explicit `module` Literal field, and the `extra=\"forbid\"` constraint. - Update README with compatibility note and 0.3.0 changelog.
TimothyWillard
left a comment
TimothyWillard
left a comment
There was a problem hiding this comment.
I've made a few notes where I saw some things.
However, bigger picture, what's the plan here? Is this a running PR that you'll keep up to date as flepimop2 v0.3 is developed or are you planning on not doing a release of op_system and this connector until flepimop2 v0.3 is released? I just want to make sure that we're in a position where releases to op_system could be made (say additional as needed features for SMH) and I think tying op_system to an unreleased version of flepimop2 makes this challenging with trunk development. Also want to make sure reviewer time is used effectively, if the plan is to just keep this as a running branch then I think I'd rather wait to look at it until after flepimop2 v0.3 is released.
| class OpSystemSystem(SystemABC): | ||
| """flepimop2 System adapter that compiles an inline ``op_system`` spec. | ||
|
|
||
| The ``module`` discriminator is declared explicitly as a | ||
| ``Literal[...]`` field per the | ||
| ``flepimop2`` module-authoring guide; this is equivalent to the | ||
| ``module="..."`` class-keyword shortcut but makes the field visible | ||
| to static analyzers and to direct readers of the class. | ||
|
|
||
| The compiled RHS is built once during pydantic validation in | ||
| :meth:`model_post_init` and exposed through :meth:`step` / | ||
| :meth:`bind`. The connector accepts no extra top-level fields | ||
| (``extra="forbid"``); unknown keys must live inside ``spec`` itself. | ||
| """ | ||
|
|
||
| module: Literal["flepimop2.system.op_system"] = "flepimop2.system.op_system" |
There was a problem hiding this comment.
| class OpSystemSystem(SystemABC): | |
| """flepimop2 System adapter that compiles an inline ``op_system`` spec. | |
| The ``module`` discriminator is declared explicitly as a | |
| ``Literal[...]`` field per the | |
| ``flepimop2`` module-authoring guide; this is equivalent to the | |
| ``module="..."`` class-keyword shortcut but makes the field visible | |
| to static analyzers and to direct readers of the class. | |
| The compiled RHS is built once during pydantic validation in | |
| :meth:`model_post_init` and exposed through :meth:`step` / | |
| :meth:`bind`. The connector accepts no extra top-level fields | |
| (``extra="forbid"``); unknown keys must live inside ``spec`` itself. | |
| """ | |
| module: Literal["flepimop2.system.op_system"] = "flepimop2.system.op_system" | |
| class OpSystemSystem(SystemABC, module="op_system"): | |
| """flepimop2 System adapter that compiles an inline ``op_system`` spec. | |
| The ``module`` discriminator is declared explicitly as a | |
| ``Literal[...]`` field per the | |
| ``flepimop2`` module-authoring guide; this is equivalent to the | |
| ``module="..."`` class-keyword shortcut but makes the field visible | |
| to static analyzers and to direct readers of the class. | |
| The compiled RHS is built once during pydantic validation in | |
| :meth:`model_post_init` and exposed through :meth:`step` / | |
| :meth:`bind`. The connector accepts no extra top-level fields | |
| (``extra="forbid"``); unknown keys must live inside ``spec`` itself. | |
| """ | |
There was a problem hiding this comment.
Actually, this is more succinct: https://accidda.github.io/flepimop2/0.3/development/pydantic-for-modelers/#the-module-class-keyword
There was a problem hiding this comment.
Bit odd to explicitly reference in the public documentation why implemented a particular way. Implementation details make sense as internal comments, not public API.
| def test_patch_type_mismatch_raises(sir_spec: dict[str, object]) -> None: | ||
| """Patching against a non-matching concrete type raises ``TypeError``.""" | ||
| sys = OpSystemSystem(spec=sir_spec) | ||
| with pytest.raises(TypeError, match="module patching requires matching"): | ||
| sys.patch("not a module", conflict=PatchConflictMode.REPLACE) # type: ignore[arg-type] |
There was a problem hiding this comment.
I don't think you need to test this, this is getting at the behavior of flepimop2 since OpSystemSystem does not override the patch method.
| model_config = ConfigDict(extra="allow") | ||
| model_config = ConfigDict(extra="forbid") |
There was a problem hiding this comment.
Hmm, this might be something to think about more on the flepimop2 side. I didn't realize that the default is extra="ignore", https://pydantic.dev/docs/validation/latest/api/pydantic/config/#pydantic.config.ConfigDict.extra. Is there a strong advantage for extra="forbid" over extra="ignore"? I guess it makes it less likely for users to accidentally specify a configuration file incorrectly, but it will be more strict. Also not sure of knock on effects.
There was a problem hiding this comment.
the default was set to extra ignore assuming people would have custom fields we wouldn't know about. Might make sense to go back to forbid, but offer a class option akin to "module"?
| class OpSystemSystem(SystemABC): | ||
| """flepimop2 System adapter that compiles an inline ``op_system`` spec. | ||
|
|
||
| The ``module`` discriminator is declared explicitly as a | ||
| ``Literal[...]`` field per the | ||
| ``flepimop2`` module-authoring guide; this is equivalent to the | ||
| ``module="..."`` class-keyword shortcut but makes the field visible | ||
| to static analyzers and to direct readers of the class. | ||
|
|
||
| The compiled RHS is built once during pydantic validation in | ||
| :meth:`model_post_init` and exposed through :meth:`step` / | ||
| :meth:`bind`. The connector accepts no extra top-level fields | ||
| (``extra="forbid"``); unknown keys must live inside ``spec`` itself. | ||
| """ | ||
|
|
||
| module: Literal["flepimop2.system.op_system"] = "flepimop2.system.op_system" |
There was a problem hiding this comment.
Bit odd to explicitly reference in the public documentation why implemented a particular way. Implementation details make sense as internal comments, not public API.
| model_config = ConfigDict(extra="allow") | ||
| model_config = ConfigDict(extra="forbid") |
There was a problem hiding this comment.
the default was set to extra ignore assuming people would have custom fields we wouldn't know about. Might make sense to go back to forbid, but offer a class option akin to "module"?
3 participants
Closes #141.
moduleas an explicitLiteral["flepimop2.system.op_system"]field instead of using themodule="..."class-keyword shortcut.model_configtoextra="forbid".# noqa: D101).flepimop2to>=0.3.0.dev0.ModuleBase.patchREPLACE-mode recompile, type-mismatch guard, the explicitmoduleLiteral field, and theextra=\"forbid\"constraint.