Skip to content

Feature/configurable solver - #126

Open
adefabian wants to merge 7 commits into
mainfrom
feature/configurable_solver
Open

adefabian wants to merge 7 commits into
mainfrom
feature/configurable_solver

Conversation

@adefabian

@adefabian adefabian commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Impulse always builds DefaultSolver.

A customer who needs the engine to behave differently on
silver-shaped data has no supported way to plug that in.

This PR lets a customer register their own QuerySolver subclass and select it with
query_engine.solver
, with its own typed settings in solver_config. It mirrors the custom
MeasurementDB from #109, and both now share one name registry. The change is additive: if nothing
is configured, DefaultSolver is used, and DeltaSolver / KeyValueStoreSolver keep working as
deprecated aliases. The one breaking change is stricter parsing of unknown keys (below).

Breaking change

Unknown keys are now rejected at the top level of the config and directly under
query_engine.solver_config. query_engine already rejects them, since #109. A config with a
misspelled or misplaced key, for example "solver" outside query_engine, fails at load time with
Extra inputs are not permitted instead of being silently ignored. Without this, forgetting or
misplacing solver silently ran DefaultSolver and dropped the custom solver's settings.

Behaviour changes

  • query_engine.solver is now a str and accepts any registered name. Before, it was a Solvers
    enum member. Solvers is now a StrEnum, so comparisons like == Solvers.DEFAULT_SOLVER still
    work, but .value on the field doesn't.
  • An unregistered solver or measurement_db name fails at load time on its own key
    (query_engine.solver / query_engine.measurement_db), and the error lists the registered names.
  • resolve_measurement_db() returns the shared Registration. Its class field is now impl_cls
    (was db_cls).

Changes

  • Extension point: QuerySolver.from_config(ctx) build hook, taking a SolverBuildContext
    (Spark session, solver_config, RAW-data flags) (solvers/query_solver.py). DefaultSolver
    implements it and registers itself with both aliases. Its constructor is unchanged. The context
    copies the query_engine settings a solver needs, rather than passing the QueryEngine config,
    because the query engine must not depend on the reporting layer.
  • Registry: register_solver(name, config_cls=SolverConfig, aliases=()) / resolve_solver
    (solvers/solver_registry.py). Both are built on a generic Registry
    (impulse_query_engine/registry.py), which the measurement-DB registry now uses too.
  • Config:
    • The solver name is checked against the registry, and solver_config is validated with the
      config class the selected solver registered. Missing or invalid custom settings fail at load time
      with invalid settings for solver 'X': … (impulse_reporting/config/config_parser.py).
    • SolverConfig and ImpulseConfig forbid unknown keys (see Breaking change). Custom config
      classes inherit this.
  • Report: Report.create_solver resolves the name and calls from_config. In telemetry,
    built-in solvers keep their existing names and a custom solver is logged as custom, so customer
    class names aren't sent (impulse_reporting/core/report.py).
  • Docs:
    • New Extensibility → Custom solver page, including when to use a custom solver vs. a custom
      MeasurementDB.
    • Configuration, query-solver and ingestion pages updated, API reference regenerated, and the
      impulse-analyze, impulse-config and impulse-data-model skills updated.
    • The ingestion page and the impulse-data-model skill now recommend a custom MeasurementDB, not
      a custom solver, for structural differences in the source data.

Test Plan

  • Unit tests:
    • generic registry: last registration wins, aliases, base-class checks, unknown-name error
    • solver registry and from_config
    • config parsing:
      • typed custom solver_config, including omitted, missing and invalid settings
      • unknown keys rejected: a misspelled custom setting, custom settings without solver, and
        solver outside query_engine
      • unregistered names reported on their own key
    • Report builds a registered solver; telemetry keeps the built-in names
  • Integration tests (custom_solver_report_test.py): every solve goes through the selected
    solver and its typed setting reaches it; DefaultSolver is unchanged by the registration
  • Full test suite passes locally (1876 passed, pytest tests/ -n auto)
  • Docs site builds without broken links
  • Demo notebooks re-run (checked statically: they only use "DefaultSolver" and contain no
    keys the stricter parsing rejects)

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • No new linter warnings introduced

This pull request and its description were written by Isaac.

@codecov

codecov Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.50%. Comparing base (8fa2124) to head (507feff).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
+ Coverage   90.30%   90.50%   +0.19%     
==========================================
  Files          68       70       +2     
  Lines        6068     6142      +74     
  Branches      758      762       +4     
==========================================
+ Hits         5480     5559      +79     
+ Misses        466      463       -3     
+ Partials      122      120       -2     
Flag Coverage Δ
query_engine 87.03% <100.00%> (+0.23%) ⬆️
reporting 95.09% <100.00%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ery_engine/analyze/query/solvers/default_solver.py 95.52% <100.00%> (+0.59%) ⬆️
...query_engine/analyze/query/solvers/query_solver.py 91.01% <100.00%> (+1.26%) ⬆️
...uery_engine/analyze/query/solvers/solver_config.py 100.00% <100.00%> (ø)
...ry_engine/analyze/query/solvers/solver_registry.py 100.00% <100.00%> (ø)
...rc/impulse_query_engine/measurement_db_registry.py 100.00% <100.00%> (ø)
src/impulse_query_engine/registry.py 100.00% <100.00%> (ø)
src/impulse_reporting/config/config_parser.py 97.46% <100.00%> (+0.25%) ⬆️
src/impulse_reporting/core/report.py 94.96% <100.00%> (+0.79%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@adefabian
adefabian force-pushed the feature/configurable_solver branch from e9fdee6 to 507feff Compare October 16, 2026 10:57
@adefabian
adefabian marked this pull request as ready for review October 16, 2026 11:34
@adefabian
adefabian requested a review from a team as a code owner October 16, 2026 11:34
@adefabian
adefabian requested a review from tombonfert October 16, 2026 11:34

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant