Skip to content

[TIDY-FIRST] Add typing to BaseRunner and subclasses (Part 1)#12803

Merged
QMalcolm merged 14 commits intomainfrom
qmalcolm--tidy-first-type-the-runners
Apr 10, 2026
Merged

[TIDY-FIRST] Add typing to BaseRunner and subclasses (Part 1)#12803
QMalcolm merged 14 commits intomainfrom
qmalcolm--tidy-first-type-the-runners

Conversation

@QMalcolm
Copy link
Copy Markdown
Contributor

@QMalcolm QMalcolm commented Apr 8, 2026

Summary

  • Makes BaseRunner generic over its result type via BaseRunner[RunnerResultT] where RunnerResultT = TypeVar("RunnerResultT", bound=NodeResult)
  • Converts before_execute, after_execute, and execute from raise NotImplementedError to true @abstractmethods
  • Types all runner subclasses with their concrete result types (RunResult, FreshnessNodeResult, etc.)
  • Broadens call_runner and _handle_result in runnable.py to NodeResult to correctly handle FreshnessRunner's result type

Commits

Each commit is independently reviewable:

  1. a3b564a — Add type annotations to ExecutionContext and BaseRunner.__init__
  2. 284e1e5 — Make before_execute, after_execute, execute abstract in BaseRunner
  3. e6df3e0 — Add RunnerResultT generic TypeVar to BaseRunner
  4. 5e8f601 — Type CompileRunner as BaseRunner[RunResult]
  5. 52fe025 — Document GenericSqlRunner's LSP violation with explicit type: ignore markers
  6. 2ed5917 — Type ModelRunner, MicrobatchBatchRunner, MicrobatchModelRunner
  7. 1853ac0 — Type TestRunner.execute and after_execute
  8. fa59711 — Type FreshnessRunner as BaseRunner[FreshnessNodeResult]
  9. 4ba5156 — Type remaining runners: CloneRunner, NoOpRunner, ShowRunner
  10. 85502ca — Type runnable.py: broaden call_runner and _handle_result to NodeResult

Notable design decisions

  • GenericSqlRunner: Uses RemoteCompileResultMixin as its result type, which is not a NodeResult subtype, so it cannot satisfy the RunnerResultT bound. It is kept in the CompileRunner MRO and its overriding methods are annotated with # type: ignore[override] with an explanatory comment block.
  • FreshnessRunner: Typed as BaseRunner[FreshnessNodeResult] where FreshnessNodeResult = Union[PartialSourceFreshnessResult, SourceFreshnessResult] — both are NodeResult subtypes.
  • Pre-existing violations: MicrobatchModelRunner and TestRunner access subtype-specific attributes (ModelNode, TestNode) on self.node: ResultNode; these are suppressed with targeted # type: ignore[union-attr/attr-defined].

Test plan

  • Pre-commit hooks (isort, black, flake8, mypy) pass on all changed files
  • Existing runner unit/integration tests pass

🤖 Generated with Claude Code

@cla-bot cla-bot bot added the cla:yes label Apr 8, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 8, 2026

Codecov Report

❌ Patch coverage is 79.79798% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.36%. Comparing base (2384e7c) to head (43f38ea).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12803      +/-   ##
==========================================
- Coverage   91.41%   91.36%   -0.05%     
==========================================
  Files         203      203              
  Lines       25879    25884       +5     
==========================================
- Hits        23657    23649       -8     
- Misses       2222     2235      +13     
Flag Coverage Δ
integration 88.16% <76.76%> (-0.13%) ⬇️
unit 65.53% <63.63%> (+<0.01%) ⬆️

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

Components Coverage Δ
Unit Tests 65.53% <63.63%> (+<0.01%) ⬆️
Integration Tests 88.16% <76.76%> (-0.13%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@QMalcolm QMalcolm force-pushed the qmalcolm--tidy-first-type-the-runners branch from 16805ec to 02a4a97 Compare April 8, 2026 22:16
@QMalcolm QMalcolm marked this pull request as ready for review April 8, 2026 22:21
@QMalcolm QMalcolm requested a review from a team as a code owner April 8, 2026 22:21
@QMalcolm QMalcolm force-pushed the qmalcolm--tidy-first-type-the-runners branch from 122b135 to ce3f829 Compare April 9, 2026 00:10
@dbt-labs dbt-labs deleted a comment from github-actions bot Apr 9, 2026
@QMalcolm QMalcolm changed the title [TIDY-FIRST] Add typing to BaseRunner and all runner subclasses [TIDY-FIRST] Add typing to BaseRunner and subclasses (Part 1) Apr 9, 2026
@QMalcolm
Copy link
Copy Markdown
Contributor Author

QMalcolm commented Apr 9, 2026

QMalcolm and others added 14 commits April 9, 2026 15:06
Types node in ExecutionContext, and all parameters and instance variables
in BaseRunner.__init__, using ResultNode and BaseAdapter.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
These methods raised NotImplementedError at runtime but were not decorated
with @AbstractMethod, giving no static guarantee that subclasses implement
them. Converting them to proper abstract methods catches missing
implementations at class-definition time rather than at runtime.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Introduces RunnerResultT = TypeVar("RunnerResultT", bound=NodeResult) and
makes BaseRunner generic over it. All result-producing methods (safe_run,
run_with_hooks, error_result, ephemeral_result, from_run_result,
compile_and_execute, execute, run, on_skip, _build_run_result) now declare
their return types in terms of RunnerResultT.

The base _build_run_result and on_skip implementations construct RunResult
directly (valid for all RunResult-returning subclasses); subclasses with
different result types (FreshnessRunner, GenericSqlRunner) override these
methods entirely. Targeted type: ignore comments are placed only at those
two concrete construction points.

Also moves the skip_cause None guard before its first use in on_skip,
fixing a latent runtime error if the guard condition were ever triggered
in a different order.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Declares CompileRunner(BaseRunner[RunResult]) so that all inherited
result-producing methods are statically known to return RunResult.
Adds return type annotations to before_execute, after_execute, and execute.

The compile() method's type: ignore[arg-type] documents a pre-existing
semantic constraint: CompileRunner is only used with compileable node types,
but the ResultNode union includes SeedNode and SourceDefinition which
compile_node does not accept.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…markers

GenericSqlRunner uses SQLResult (bound to RemoteCompileResultMixin) which
belongs to a completely separate type hierarchy from NodeResult/RunResult.
It cannot be a valid BaseRunner[RunnerResultT] and its execute/from_run_result
overrides intentionally return a different type than CompileRunner declares.

Mark each override point with # type: ignore[override] and add a class-level
comment explaining the violation, making the boundary visible rather than
silently wrong.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds return type annotations to ModelRunner.after_execute, ModelRunner.execute,
MicrobatchBatchRunner.on_skip, and MicrobatchBatchRunner.error_result.

MicrobatchModelRunner.execute(model: ModelNode) already had a narrowed type
annotation; marks it # type: ignore[override] to document that it intentionally
accepts a more specific argument type than the base class requires.

The remaining type: ignore[union-attr/arg-type] markers document pre-existing
issues surfaced by typing self.node as ResultNode: MicrobatchBatchRunner and
MicrobatchModelRunner access ModelNode-specific attributes (batch, has_this,
previous_batch_results, config.batch_size, config.concurrent_batches) that
are not present on the full ResultNode union.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TestRunner.execute accepts Union[TestNode, UnitTestNode] which narrows the
first parameter relative to BaseRunner; marks it # type: ignore[override].
TestRunner uses TestStatus internally but still returns RunResult, so the
return type is compatible.

Adds # type: ignore[attr-defined] on describe_node_name for the UnitTestNode
branch, where model/versioned_name attributes exist on UnitTestDefinition but
not on all ResultNode union members.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
FreshnessRunner is the only runner that returns a completely different
result type from the standard RunResult hierarchy. By declaring it as
BaseRunner[FreshnessNodeResult], the return types of execute, error_result,
_build_run_result, from_run_result, and on_skip are all statically known
to return FreshnessNodeResult.

_build_run_result carries # type: ignore[override] because FreshnessRunner's
signature omits the agate_table/adapter_response/failures/batch_results
parameters that the base declares; this is intentional — freshness results
do not need those fields.

Pre-existing type issues surfaced by self.node: ResultNode (source_name only
on SourceDefinition, FreshnessResponse typing in execute) are documented with
targeted type: ignore markers.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Declares CloneRunner(BaseRunner[RunResult]) and NoOpRunner(BaseRunner[RunResult]).
Adds return type annotations to execute and after_execute on each.
Adds return type to ShowRunner.execute.

SeedRunner and SnapshotRunner inherit from the already-typed ModelRunner
and do not override the result-returning methods, so no changes needed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- call_runner() -> NodeResult (was RunResult): runner.run_with_hooks() can
  return FreshnessNodeResult for FreshnessRunner, so RunResult was too narrow
- _handle_result(result: NodeResult): only accesses .node, .status, and
  .to_msg_dict() which are all available on NodeResult
- _mark_dependent_errors now takes NodeResult / Optional[NodeResult] for
  result and cause parameters
- _skipped_children dict broadened to Dict[str, Optional[NodeResult]]
- Add # type: ignore[arg-type] on run_with_hooks(self.manifest) since
  self.manifest is Optional[Manifest] but guaranteed non-None at runtime

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- build.py: call_model_and_unit_tests_runner() -> NodeResult (was RunResult),
  since it returns call_runner() which now returns NodeResult; drop unused
  RunResult import, add NodeResult import
- function.py: add # type: ignore[override] on FunctionRunner.execute() which
  narrows compiled_node to FunctionNode (an LSP violation like other runners
  that operate on specific node subtypes)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
After making before_execute, after_execute, and execute abstract in BaseRunner,
MockRunner in the test file needs stub implementations to be instantiable.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
call_runner() now returns NodeResult, but MicrobatchModelRunner.execute()
always returns RunResult. Use cast() to satisfy the List[RunResult] type
at the batch_results.append call site.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@QMalcolm QMalcolm force-pushed the qmalcolm--tidy-first-type-the-runners branch from ce3f829 to 43f38ea Compare April 9, 2026 20:06
@QMalcolm QMalcolm merged commit 26c9c8c into main Apr 10, 2026
124 checks passed
@QMalcolm QMalcolm deleted the qmalcolm--tidy-first-type-the-runners branch April 10, 2026 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants