Skip to content

⚙️ FEATURE-#201: Separate Engine from Execution Strategy#202

Merged
FernandoCelmer merged 14 commits intodevelopfrom
feature/201
Apr 8, 2026
Merged

⚙️ FEATURE-#201: Separate Engine from Execution Strategy#202
FernandoCelmer merged 14 commits intodevelopfrom
feature/201

Conversation

@FernandoCelmer
Copy link
Copy Markdown
Member

@FernandoCelmer FernandoCelmer commented Apr 8, 2026

Description

Refactor the task execution architecture by introducing a TaskEngine that encapsulates the task lifecycle, separating it from execution strategies. Moves retry, timeout and backoff from @action to the engine.

Issue: 📌 ISSUE-#201

Summary

Phase 1 — TaskEngine

  • TaskEngine (dotflow/core/engine.py) — manages lifecycle via context manager: status transitions, duration, tracer, error handling
  • Execution — refactored as backward-compatible wrapper around TaskEngine
  • Flow base class_has_checkpoint() and _restore_checkpoint() extracted (DRY)
  • Strategies (Sequential, SequentialGroup, Background) — use TaskEngine directly

Phase 2 — Retry/Timeout in Engine

  • execute_with_retry() — retry loop, timeout via ThreadPoolExecutor, backoff
  • Action._run_action() — simplified to single attempt
  • @action attributesretry, timeout, retry_delay, backoff propagated to closure

Phase 3 — Composable Context Managers

  • checkpoint_context() — automatic checkpoint post-execution
  • Docs updated to reference TaskEngine

Bug Fix

  • Checkpoint example now uses valid UUID

Type of change

  • New feature (non-breaking change which adds functionality)

Validation

  • 427 tests passing (21 new for TaskEngine)
  • All docs_src/ examples verified
  • flake8, ruff check, ruff format — all clean

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have made corresponding changes to the documentation

@FernandoCelmer FernandoCelmer self-assigned this Apr 8, 2026
@FernandoCelmer FernandoCelmer merged commit 23bc739 into develop Apr 8, 2026
10 checks passed
Copy link
Copy Markdown
Member Author

@FernandoCelmer FernandoCelmer left a comment

Choose a reason for hiding this comment

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

🔍 Code Review

Code issues found: 6

# Severity Comment
1 [Blocking] Executor leak on success path
2 [Blocking] Duration None when tracer calls end_task
3 [Blocking] ExecutionWithClassError bypasses retry guard
4 [Suggestion] Double storage read on checkpoint
5 [Suggestion] Action docstring outdated after refactor
6 [Suggestion] Parallel still uses deprecated Execution (see below)

6. [Suggestion] Parallel still uses deprecated Execution

File: dotflow/core/workflow.py ~line 430

Problem — The new Execution docstring explicitly states: "New code should use TaskEngine directly." The strategies Sequential, SequentialGroup, and Background were migrated to TaskEngine, but Parallel.run() still uses Execution:

process = _mp.Process(
    target=Execution,
    args=(task, self.workflow_id, previous_context, self._flow_callback),
)

This creates an architectural inconsistency: 3 out of 4 strategies use the new pattern, 1 uses the old one.

Failure scenario — Future developers may use Parallel as a reference and perpetuate the use of Execution instead of TaskEngine. Additionally, any bug fix or improvement made to TaskEngine will not automatically apply to tasks executed via Parallel, creating behavioral divergence between execution modes.

Fix — If the Parallel migration was intentionally deferred (e.g., multiprocessing pickling complexity), add an explicit TODO comment:

# TODO: Migrate Parallel to use TaskEngine directly.
# Currently uses Execution wrapper due to multiprocessing pickling constraints.
process = _mp.Process(
    target=Execution,
    args=(task, self.workflow_id, previous_context, self._flow_callback),
)

Comment thread dotflow/core/engine.py
Comment thread dotflow/core/engine.py
Comment thread dotflow/core/engine.py
Comment thread dotflow/abc/flow.py
Comment thread dotflow/core/action.py
@FernandoCelmer FernandoCelmer added enhancement New feature or request documentation Improvements or additions to documentation bug Something isn't working labels Apr 8, 2026
FernandoCelmer added a commit that referenced this pull request Apr 8, 2026
🪲 BUG-#205: Fix PR #202 review issues — duration, executor leak, checkpoint, docstring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant