Features/ibis new design by egillax · Pull Request #20 · OHDSI/Circepy

egillax · 2026-03-18T15:34:44Z

Summary

This PR replaces the legacy builder/context-based Ibis execution prototype with a new layered execution
engine built around normalized models, lowering, Ibis compilation, and explicit cohort orchestration.

The new public execution entrypoints are:

build_cohort(...)
write_cohort(...)

build_cohort(...) returns a lazy Ibis relation in the canonical execution shape.

write_cohort(...) materializes OHDSI cohort-table rows with cohort-scoped semantics:

if_exists="fail" errors only if rows already exist for that cohort_id
if_exists="replace" replaces only that cohort’s rows and preserves rows for other cohorts in the
same target table

What Changed

replaced the legacy builder/context-based Ibis execution path with a layered execution subsystem
introduced explicit execution layers:
- normalize/
- lower/
- ibis/
- engine/
changed the public execution API to function-first entrypoints:
- build_cohort(...)
- write_cohort(...)
standardized compiled domain events into a canonical event schema before cohort orchestration
implemented cohort-scoped write semantics in write_cohort(...)
added focused execution tests and documented the intended testing strategy
added architecture documentation for reviewers and future maintainers

Execution Flow

flowchart LR
    A[Public cohort models] --> B[normalize]
    B --> C[lower]
    C --> D[ibis compile]
    D --> E[engine semantics<br/>primary events -> groups -> inclusion -> end strategy -> censoring ->
collapse]
    E --> F[final Ibis relation]
    F --> G[build_cohort]
    F --> H[write_cohort]

Why

The old execution prototype had too much mutable, builder-specific state, too much coupling between
cohort semantics and backend-specific implementation details, and too much duplicated execution logic.

This redesign aims to make the execution path:

easier to reason about
easier to test by layer
easier to maintain
easier to extend to new semantics and backends
less dependent on mutable executor state
less duplicated across execution concerns

Future Opportunities Enabled by This Design

This layered execution design should make several follow-up capabilities easier to add without re-
entangling cohort semantics with executor state, including:

stage-labeled execution tracing
final and intermediate SQL inspection
optional backend-level executed-statement logging
semantic “explain” views for cohort execution
plan-level diffs for regression review
smarter caching or partial rematerialization
richer provenance and debugging tooling
alternative wrapper shells built on the same execution core

Migration Notes

This PR intentionally changes the execution API shape.

If you used the old execution prototype:

use build_cohort(...) to get the lazy Ibis relation
use backend/relation methods directly for inspection and collection
use write_cohort(...) for cohort-table writes

Examples:

old dataframe collection helpers map to relation methods such as relation.to_pandas() or
relation.to_polars()
old SQL inspection helpers such as capture_sql() are not carried forward as executor-owned APIs
SQL inspection now happens through the returned Ibis relation and backend tooling
old executor-owned write behavior maps to write_cohort(...)

In practice:

inspection and collection now happen through the returned relation and backend
write semantics now live in write_cohort(...), not a mutable executor object

Reviewer Guide

Current Limitation

custom_era end strategy is still unsupported in this execution path and raises an explicit
execution-layer error
this is tracked in Add custom_era support to the new execution engine #24

Testing

Executed locally:

uv run ruff check .
uv run ruff format --check .
uv run pytest -q

Result:

1075 passed
17 skipped
1 xfailed

Note:

pytest still emits DuckDB/Ibis deprecation warnings from upstream fetch_arrow_table() calls in
Ibis’s DuckDB backend

codecov · 2026-03-18T15:36:53Z

Codecov Report

❌ Patch coverage is 95.47206% with 94 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.39%. Comparing base (fa4e9c5) to head (0db6e37).
⚠️ Report is 19 commits behind head on develop.

Files with missing lines	Patch %	Lines
circe/execution/engine/group_demographics.py	82.97%	16 Missing ⚠️
circe/execution/engine/primary.py	78.57%	9 Missing ⚠️
circe/execution/ibis/person_filters.py	89.39%	7 Missing ⚠️
circe/execution/engine/group_operators.py	92.06%	5 Missing ⚠️
circe/execution/engine/groups.py	88.37%	5 Missing ⚠️
circe/execution/normalize/cohort.py	93.24%	5 Missing ⚠️
circe/execution/api.py	91.66%	4 Missing ⚠️
circe/execution/ibis_compat.py	88.57%	4 Missing ⚠️
circe/execution/lower/common.py	95.83%	4 Missing ⚠️
circe/execution/normalize/groups.py	94.44%	4 Missing ⚠️
... and 23 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #20      +/-   ##
===========================================
+ Coverage    76.87%   85.39%   +8.51%     
===========================================
  Files          133      167      +34     
  Lines        12126    12379     +253     
===========================================
+ Hits          9322    10571    +1249     
+ Misses        2804     1808     -996

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

egillax · 2026-03-19T09:13:56Z

Ready for review @azimov

Sorry for amount of code.

azimov · 2026-03-19T15:08:53Z

@egillax The review will probably take me some time but I'm very optimistic about this after these changes. The updated benchmarks show that this is at least a 2x performance increase on real data in databricks/healthverity. We can likely do more too (e.g. if cohorts have shared components we can probably build planners that use them). The fact that we can also use this for a unified feature extraction approach also has a good appeal to me.

The lack of support for custom eras could be a blocker but there are potential workaround - we could use sqlglot in the cases where the windowing logic is difficult/not supported by ibis implementations.

egillax · 2026-03-19T15:32:30Z

I think I can add the custom eras. Just didn't see immediately how to do it and rather than having it block this I'll do it separately once I've fogured it out

azimov

Overall this is pretty strong, I think the code could be tidier and I noted some more patterns that could improve it for extendability.

The planning approach naturally extends itself to feature extraction too, I would expect that we can build some pretty cool stuff with this.

Happy for you to merge when ready

azimov · 2026-03-19T20:10:27Z

circe/execution/README.md

This and the TESTING.md file should probably live in the docs.

In general I think we should configure the LLMs to make as few .md files as possible as it gets pretty annoying and they're frequently outdated (so will just confuse the next agent).

azimov · 2026-03-19T20:12:49Z

circe/execution/README.md

+
+## Canonical Event Schema
+
+All compiled domain event tables are standardized before cohort orchestration.


This seems like it will be very useful going forwards, we can likely build upon this to design ways to understand common patterns an pathways in cohorts

azimov · 2026-03-19T20:36:02Z

circe/execution/ibis/codesets.py

+from ..typing import Table
+
+
+class CachedConceptSetResolver:


This class could likely be adapted to add a persistent caching layer making the resolution instant in many cases. Probably worth doing in a separate PR though.

azimov · 2026-03-19T20:39:33Z

circe/execution/databricks_compat.py

+    setattr(backend_cls, _PATCH_FLAG, True)
+    return True
+
+


strange flow (and funny naming), would this not be better structured when loading the backend to being with?

azimov · 2026-03-19T22:27:40Z

circe/execution/lower/criteria.py

+    ) -> EventPlan: ...
+
+
+LOWERERS: dict[type[Criteria], LowerFn] = {


This could be assigned dynamically at runtime on top of criteria classes - I'm thinking with a decorator pattern.

@register_lowering("ProcedureOccurrence") def lower_procedure_occurrence(): ....

This would naturally be extendable for extension tables, and removes this hard coding linking. This could go further with filters but that might be too much decorator spam.

azimov · 2026-03-19T22:29:06Z

circe/execution/lower/death.py

+    criterion_index: int,
+) -> EventPlan:
+    raw = criterion.raw_criteria
+    if not isinstance(raw, Death):


A decorator could also remove this boilerplate that is in every function here

azimov · 2026-03-19T22:31:09Z

circe/execution/lower/death.py

+        exclude=bool(raw.death_type_exclude),
+    )
+
+    return build_standard_domain_plan(


Again - this feels like boilerplate that could be removed.

azimov · 2026-03-19T22:33:55Z

circe/execution/lower/dose_era.py

+from .common import lower_standard_domain_plan
+
+
+def lower_dose_era(


This is just default behaviour so I don't think it needs a function and a file for every single implementation

azimov · 2026-03-19T22:40:22Z

circe/execution/lower/device_exposure.py

+    if not isinstance(raw, DeviceExposure):
+        raise TypeError("lower_device_exposure requires DeviceExposure criteria")
+
+    steps = lower_common_steps(criterion)


I'm wondering if this too could be structured differently, is this over design here?

return ( DomainPlanBuilder(criterion, criterion_index) .with_concept_filter( "device_type_concept_id", concepts_attr="device_type", codeset_attr="device_type_cs", exclude_attr="device_type_exclude", ) .with_text_filter("unique_device_id", value_attr="unique_device_id") .with_numeric_filter("quantity", value_attr="quantity") .with_provider_specialty_filter() .with_visit_filter() .build() )

My thought is that this type of pattern could be more readable and extendable. We could also add conditions like `min_cdm_ver='5.5' (though this is probably better handled in the model classes).

azimov · 2026-03-19T22:43:14Z

circe/execution/lower/location_region.py

+)
+
+
+def lower_location_region(


this function is totally different to the others, maybe the AI got bored?

egillax · 2026-03-20T12:33:38Z

thanks @azimov , I'm moving the docs and merging. Then I'll create issues for the other suggestions you made for follow up PRs.

egillax added 6 commits March 18, 2026 16:20

refactor(execution): replace builder-based ibis engine

bf396c2

refactor(execution): remove polars compatibility surface

0294744

fix(execution): tighten compatibility typing

c18348b

refactor(execution): remove legacy compatibility surface

d8cf632

docs(execution): remove legacy alias note

eff7a35

chore: remove polars

bba65e7

egillax added 7 commits March 18, 2026 16:38

fix(execution): satisfy ruff import rules

2cc58e3

style(execution): format ibis operations

2b7b7b6

test(execution): expand coverage and document strategy

9990db1

docs(execution): add architecture overview

90b9b03

docs: refresh root readme package status

f2bb0a4

test(execution): cover remaining helper branches

99780d3

test(execution): make keep-first helper test py39-stable

aca1f1a

egillax requested a review from azimov March 19, 2026 09:13

egillax added 4 commits March 19, 2026 22:13

fix(execution): apply nested correlated criteria in groups

810b77b

test(execution): expand nested correlated coverage

579ed22

fix(execution): restore era filter semantics

ffe153c

fix(execution): align collapse tie handling with circe

95386c6

azimov approved these changes Mar 19, 2026

View reviewed changes

docs(execution): move developer docs into sphinx guide

0db6e37

egillax merged commit 39bed84 into develop Mar 20, 2026
9 checks passed

egillax deleted the features/ibis-new-design-develop branch March 20, 2026 12:43


		## Canonical Event Schema

		All compiled domain event tables are standardized before cohort orchestration.

		) -> EventPlan: ...


		LOWERERS: dict[type[Criteria], LowerFn] = {

		from .common import lower_standard_domain_plan


		def lower_dose_era(

Conversation

egillax commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What Changed

Execution Flow

Why

Future Opportunities Enabled by This Design

Migration Notes

Reviewer Guide

Current Limitation

Testing

Uh oh!

codecov bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

egillax commented Mar 19, 2026

Uh oh!

azimov commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

egillax commented Mar 19, 2026

Uh oh!

azimov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

egillax commented Mar 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

egillax commented Mar 18, 2026 •

edited

Loading

codecov bot commented Mar 18, 2026 •

edited

Loading

azimov commented Mar 19, 2026 •

edited

Loading

azimov left a comment •

edited

Loading