Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions flepimop2-op_system/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,24 @@
`flepimop2-op_system` provides the `flepimop2` system adapter for `op_system`.

It packages the `flepimop2.system.op_system` provider so `flepimop2` can load and execute RHS specifications compiled by the core `op_system` package.

## Compatibility

This version (`0.3.0`) requires `flepimop2 >= 0.3.0.dev0` (the consolidated
`ModuleBase` API).

## Changelog

### 0.3.0

- Adopted the consolidated `flepimop2.module.ModuleBase` API. The
`OpSystemSystem` connector now declares its `module` discriminator
explicitly as a `Literal[...]` field rather than via the
`module="..."` class-keyword shortcut.
- Switched the connector's `model_config` to `extra="forbid"`. Unknown
top-level keys are rejected; engine-specific knobs continue to live
inside `spec`.
- Verified compatibility with the new `ModuleBase.patch(...)` method and
the `flepimop2 patch` CLI; added regression tests for both the
`REPLACE` mode (recompiles the RHS) and the type-mismatch guard.
- Bumped `flepimop2` floor from `>=0.2.0` to `>=0.3.0.dev0`.
4 changes: 2 additions & 2 deletions flepimop2-op_system/pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[project]
name = "flepimop2-op_system"
version = "0.2.0"
version = "0.3.0"
description = "flepimop2 system provider package for op_system"
readme = "README.md"
requires-python = ">=3.11,<3.15"
Expand All @@ -12,7 +12,7 @@ authors = [
license = { file = "LICENSE" }
dependencies = [
"op-system>=0.1.0",
"flepimop2>=0.2.0",
"flepimop2>=0.3.0.dev0",
"pydantic>=2.0,<3",
"numpy>=1.26",
"PyYAML>=6.0",
Expand Down
23 changes: 20 additions & 3 deletions flepimop2-op_system/src/flepimop2/system/op_system/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
from typing import (
TYPE_CHECKING,
Any,
Literal,
NamedTuple,
cast,
)
Expand All @@ -54,7 +55,7 @@

from op_system import CompiledRhs, compile_spec

__version__ = "0.2.0"
__version__ = "0.3.0"

if TYPE_CHECKING:
from collections.abc import Callable
Expand All @@ -69,14 +70,30 @@ class _AxesMeta(NamedTuple):
axis_coords: dict[str, np.ndarray]


class OpSystemSystem(SystemABC, module="flepimop2.system.op_system"): # noqa: D101
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"
Comment on lines +73 to +88

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.
"""

https://accidda.github.io/flepimop2/0.3/development/creating-an-external-provider-package/#step-4-implement-the-npzbackend-class

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit odd to explicitly reference in the public documentation why implemented a particular way. Implementation details make sense as internal comments, not public API.


state_change: StateChangeEnum = StateChangeEnum.FLOW

spec: dict[str, object] = Field(
default=..., description="Inline op_system RHS specification (already loaded)"
)

model_config = ConfigDict(extra="allow")
model_config = ConfigDict(extra="forbid")
Comment on lines -79 to +96

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?


def model_post_init(self, context: Any) -> None: # noqa: ANN401
"""Compile `op_system` specification and prepare stepper and shape helpers.
Expand Down
36 changes: 36 additions & 0 deletions flepimop2-op_system/tests/test_system.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@
import numpy as np
import pytest
from flepimop2.axis import AxisCollection
from flepimop2.module import PatchConflictMode
from flepimop2.parameter.abc import ModelStateSpecification, ParameterRequest
from flepimop2.typing import SystemProtocol
from pydantic import ValidationError

from flepimop2.system.op_system import OpSystemSystem

Expand Down Expand Up @@ -580,3 +582,37 @@ def test_requested_parameters_honours_time_axis_option() -> None:
sys = OpSystemSystem(spec=spec)
requested = sys.requested_parameters(AxisCollection())
assert requested["beta"].axes == ("day",)


def test_module_field_is_literal() -> None:
"""The `module` discriminator is exposed as an explicit Literal field."""
field = OpSystemSystem.model_fields["module"]
assert field.default == "flepimop2.system.op_system"


def test_extra_top_level_fields_rejected(sir_spec: dict[str, object]) -> None:
"""Unknown top-level kwargs are rejected (``extra='forbid'``)."""
with pytest.raises(ValidationError):
OpSystemSystem(spec=sir_spec, bogus_field=1) # type: ignore[call-arg]


def test_patch_replace_recompiles(sir_spec: dict[str, object]) -> None:
"""Patching with REPLACE swaps in the new spec and recompiles the RHS."""
sys_a = OpSystemSystem(spec=sir_spec)
si_spec: dict[str, object] = {
"kind": "expr",
"state": ["S", "I"],
"equations": {"S": "-beta * S * I", "I": "beta * S * I"},
}
sys_b = OpSystemSystem(spec=si_spec)
patched = sys_a.patch(sys_b, conflict=PatchConflictMode.REPLACE)
y0 = np.array([0.9, 0.1], dtype=np.float64)
out = patched.step(np.float64(0.0), y0, beta=1.0)
np.testing.assert_allclose(out, np.array([-0.09, 0.09]), rtol=1e-12)


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]
Comment on lines +614 to +618

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading