update flepimop2 provider pacakge by jc-macdonald · Pull Request #12 · ACCIDDA/op_engine

jc-macdonald · 2026-02-13T12:33:27Z

This pull request consolidates and refactors the integration between flepimop2 and op_engine, moving all engine logic and configuration into a single file (__init__.py). It removes the separate config.py and engine.py modules, simplifies dependency management in pyproject.toml, and updates linting rules. The new implementation provides a streamlined, self-contained flepimop2 engine backed by op_engine, with improved configuration validation and runtime checks.

Engine integration and refactoring:

Merged all engine logic and configuration into src/flepimop2/engine/op_engine/__init__.py, replacing the previous split between engine.py and config.py. The new implementation defines the OpEngineFlepimop2Engine and its configuration in a single file, with improved runtime validation and operator handling.

Dependency and build configuration:

Updated pyproject.toml to reference op-engine directly from its GitHub repository, removing the sibling directory wiring and simplifying dependency management.
Linting and code quality:
Added per-file ignore rules in pyproject.toml for the new consolidated __init__.py, disabling specific lint and docstyle checks to accommodate the new implementation.
Aligned provider package with the expectations setup in flepimop2 #118

Closes #6 obsoletes #10, #11 due to changes introduced in flepimop2 #118

TimothyWillard

It would nice to see a bit more DRY, especially for low hanging fruit like using types from flepimop2 or helpers already defined in op_engine.

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

TimothyWillard · 2026-02-17T16:28:23Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

+    dt_min: float = Field(default=0.0, ge=0.0)
+    dt_max: float = Field(default=float("inf"), gt=0.0)


Does dt_min == 0.0 mean infer lower bound and dt_max == math.inf mean infer upper bound? Minor, but seems like a more natural choice would be None since that indicates no user statement.

I kept dt_min=0.0 and dt_max=inf intentionally because those are the native op_engine DtControllerConfig sentinel values for ‘no additional bound’. Switching to None in the provider would require extra normalization logic and would diverge from core config semantics without changing behavior. If we want None ergonomics, we should introduce it consistently in op_engine itself first, then mirror it here.

Yeah, that seems reasonable to mirror the API of op_engine here. I guess just noting its a bit weird to use concrete values rather than values that indicate "no statement", but I guess this is outside the scope of this PR then.

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

TimothyWillard · 2026-02-17T16:38:20Z

flepimop2-op_engine/tests/data/sir.py

This is not a TODO for you @MacdonaldJoshuaCaleb instead for me, but probably need to provide some kind of default system/engine as part of the integration testing utilities bundled with flepimop2.

flepimop2-op_engine/tests/test_engine.py

pearsonca

Minor questions, but seems fine?

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

pearsonca · 2026-02-17T19:16:15Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

+    return None
+
+
+def _rhs_from_stepper(


work for another day: if adapter is necessary, we should probably be working the pipeline definition a bit. i imagine generally engines may need to adapt SystemABC somewhat, but all this re-shaping seems problematic.

This actually seems fine to me, and within the scope of engines. This is taking a stepper function and then vectorizing it because that's the type of "stepper" that op_engine expects. I don't think flepimop2 should get into the business of trying to figure out the correct way(s) to generalize steppers. However, I do think this could be simplified a bit by the introduction of proper "build" mechanics, see ACCIDDA/flepimop2#130, ACCIDDA/flepimop2#129 (comment).

I'm mostly bothered by the shaping related checks than the reshaping itself. I should have said if this amount of adaptor required

I see, I think this is a consequence of the outputs being unstructured numpy arrays. To address this would require adding more structure on that front. Perhaps ACCIDDA/flepimop2#147 is a first step to that, but definitely more work required beyond that.

Agreed this is work for another day. The shape reshaping (1D <-> column-vector) exists because op_engine's CoreSolver expects (n_state, 1) columns while flepimop2 steppers work with 1D arrays. If the pipeline formalizes a stepper contract for shape, this goes away.

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

jc-macdonald

Why not use op_engine.types.as_float_1d?

as part of the simplifications I got rid of the types.py file

TimothyWillard · 2026-02-18T13:50:13Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

 from flepimop2.exceptions import ValidationIssue
-from flepimop2.system.abc import SystemABC, SystemProtocol
-from pydantic import BaseModel, ConfigDict, Field, model_validator
+from flepimop2.typing import StateChangeEnum  # noqa: TC002


Instead of noqaing, why not move this under the type checking imports as the lint rule suggests? See https://docs.astral.sh/ruff/rules/typing-only-third-party-import/.

StateChangeEnum is used as a Pydantic field type (state_change: StateChangeEnum on the model class). Pydantic v2 needs the type at module scope at runtime to resolve annotations during model creation, even with from __future__ import annotations. Moving it under TYPE_CHECKING would cause a NameError. The noqa: TC002 is the correct pattern here.

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

TimothyWillard · 2026-02-18T14:03:14Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

+    return None
+
+
+def _rhs_from_stepper(


This actually seems fine to me, and within the scope of engines. This is taking a stepper function and then vectorizing it because that's the type of "stepper" that op_engine expects. I don't think flepimop2 should get into the business of trying to figure out the correct way(s) to generalize steppers. However, I do think this could be simplified a bit by the introduction of proper "build" mechanics, see ACCIDDA/flepimop2#130, ACCIDDA/flepimop2#129 (comment).

TimothyWillard · 2026-02-18T14:06:39Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

+    dt_min: float = Field(default=0.0, ge=0.0)
+    dt_max: float = Field(default=float("inf"), gt=0.0)


Yeah, that seems reasonable to mirror the API of op_engine here. I guess just noting its a bit weird to use concrete values rather than values that indicate "no statement", but I guess this is outside the scope of this PR then.

TimothyWillard · 2026-02-18T14:14:57Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

+    """flepimop2 engine adapter backed by op_engine.CoreSolver."""
+
+    module: Literal["flepimop2.engine.op_engine"] = "flepimop2.engine.op_engine"
+    state_change: StateChangeEnum


Okay, I'm a bit confused about this one. It seems like this seems to indicate that op_engine handles multiple different kinds of state change steppers, but requires user configuration to do so? If op_engine can handle multiple kinds of state change steppers it should just use system.state_change to infer the settings for op_engine. However, it appears like a validation is done, and then self.state_change is never used? So can op_engine actually handle steppers of different state changes, from your description of the package I thought it could only handle "flow"?

Good question. op_engine currently only handles flow — you're right about that. The state_change field on the engine follows flepimop2 convention (engine/wrapper does the same pattern: declare state_change, validate via validate_system()). The config declares what the engine supports so the pipeline catches mismatches at validation time rather than producing silent wrong results.

self.state_change isn't used in run() because validation already ran — by the time run() executes, the system/engine pairing is guaranteed compatible. If op_engine grows to support delta or state steppers in the future, this field already has the right shape. But you're right that for now it's effectively always flow.

TimothyWillard · 2026-02-18T14:20:46Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

+            if isinstance(system_axis, str | int):
+                operator_axis = system_axis
+
+        stepper: SystemProtocol = system._stepper  # noqa: SLF001


Another argument for pushing ACCIDDA/flepimop2#130 forward in the workplan

TimothyWillard · 2026-02-18T14:23:20Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/config.py

-            tr=operators.get("tr"),
-            bdf2=operators.get("bdf2"),
-        )
+__all__ = ["OpEngineEngineConfig", "_coerce_operator_specs", "_has_operator_specs"]


Why export _coerce_operator_specs & _has_operator_specs? These seem like private utilities.

TimothyWillard · 2026-02-18T14:27:24Z

flepimop2-op_engine/pyproject.toml


 [tool.ruff.lint.per-file-ignores]
 "tests/**/*" = ["INP001", "S101"]
+"src/flepimop2/engine/op_engine/__init__.py" = ["C901", "DOC201", "DOC501", "PLR0912", "PLR0913", "PLR0914", "PLR0915", "RUF067"]


This makes me a bit weary to ignore so many linting/formatting suggestions. Perhaps outside the scope of this PR, but I think these rules exist to indicate where there might be refactoring/documentation opportunities. Although, for RUF067 we also need to think about that on the flepimop2 front, the way we've structured the package as being an implicit namespace package with dynamic module loading makes it a little annoying to work around this one.

jc-macdonald · 2026-04-05T09:00:06Z

Responded to all outstanding second-round review comments. Summary:

noqa: TC002: Explained — StateChangeEnum must be at module scope for Pydantic runtime model creation
Inline helpers scope: Acknowledged duplication; tracked in Simplify typing in flepimop2 provider package #6 for cleanup once op_engine exposes validation helpers
state_change on engine: Follows flepimop2 convention (engine/wrapper does the same); validation-only, not used in run()
Carl's adapter/export scope: Acknowledged as future work

No code changes in this round — the feedback was design discussion. All 14 provider tests pass. @TimothyWillard @pearsonca ready for another look when convenient.

jc-macdonald self-assigned this Feb 13, 2026

jc-macdonald requested review from TimothyWillard and pearsonca February 13, 2026 12:33

jc-macdonald changed the title ~~reduce introspection in flepimop2 provider package~~ update flepimop2 provider pacakge Feb 17, 2026

TimothyWillard reviewed Feb 17, 2026

View reviewed changes

pearsonca reviewed Feb 17, 2026

View reviewed changes

jc-macdonald commented Feb 17, 2026

View reviewed changes

jc-macdonald requested review from TimothyWillard and pearsonca February 17, 2026 21:09

TimothyWillard mentioned this pull request Feb 18, 2026

Add example engine/system for integration testing purposes ACCIDDA/flepimop2#150

Open

TimothyWillard reviewed Feb 18, 2026

View reviewed changes

jc-macdonald and others added 5 commits April 5, 2026 05:03

reduce introspection in flepimop2 provider package

27e7dc7

Simplify flepimop2 provider and align contracts

4a5cece

Align flepimop2 provider with latest module APIs

4509ffe

Use direct system.option lookups in op_engine provider

feaa349

chore: add .vscode/ and .Rhistory to .gitignore

62c4a79

jc-macdonald force-pushed the reduce_introspection branch from b0e8ec9 to 62c4a79 Compare April 5, 2026 09:10

jc-macdonald requested a review from TimothyWillard April 5, 2026 09:12

jc-macdonald added this to the Provider Integration milestone Apr 5, 2026

This was referenced Apr 5, 2026

Replace getattr introspection with system.option() for mixing kernels #42

Closed

Add op_system metadata passthrough to op_engine connector #10

Closed

jc-macdonald merged commit d42f227 into main Apr 7, 2026
5 checks passed

jc-macdonald deleted the reduce_introspection branch April 7, 2026 06:43

jc-macdonald mentioned this pull request Apr 7, 2026

InsightNet April 2026 — Tracking Issue ACCIDDA/flepimop2#200

Open

61 tasks

		dt_min: float = Field(default=0.0, ge=0.0)
		dt_max: float = Field(default=float("inf"), gt=0.0)

Conversation

jc-macdonald commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyWillard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pearsonca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jc-macdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jc-macdonald commented Apr 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jc-macdonald commented Feb 13, 2026 •

edited

Loading