Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): EmbeddedFlow task #6625

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Conversation

loicmathieu
Copy link
Member

@loicmathieu loicmathieu commented Jan 2, 2025

Adds an EmbeddedFlow that allow to embed subflow tasks into a parent task.

After discussion with @Ben8t, the task has been called EmbeddedFlow and not TaskGroup as it better describes what it does (embedding a subflow inside a parent flow).

Fixes #6518

Limitation of the current prototypes:

  • Users need to set the subflow tenantId as the subflow is sometimes fetched by methods that don't have the execution context. If it's not set, an exception will be thrown.
  • Task ID from the subflow are not remapped so the task ID must be unique across the parent flow and all its subflow, transitively. A validation error will occur if it's not the case.

@Ben8t
Copy link
Member

Ben8t commented Jan 6, 2025

I think we need @anna-geller view on the naming before merging as she's the one who have design the issue #6518 in details.
I don't have strong opinion on whether TaskGroup (cons: sounds similar to either WorkerGroup or WorkingDir) or EmbeddedFlow (cons: maybe not explicit enough) 🤔

@anna-geller
Copy link
Member

anna-geller commented Jan 6, 2025

We decided on TaskGroup with Ludo and it's easier for people migrating from Airflow as it's an equivalent concept

I have a strong preference for TaskGroup for many reasons, it will be easier to discuss in a call

Some of the limitations mentioned are a blocker 😢 e.g. this one -- we need to find a solution, we can't merge without it -- did you look at suggestions for implementation from the issue?

Task ID from the subflow are not remapped so the task ID must be unique across the parent flow and all its subflow, transitively. A validation error will occur if it's not the case.

@anna-geller anna-geller added the kind/do-not-merge Don't merge label Jan 6, 2025
@anna-geller
Copy link
Member

anna-geller commented Jan 7, 2025

Next steps after a meeting

  1. Call this task EmbeddedSubflow
  2. Investigate taskrun.value implementation to prevent task ID collisions + using a different execution context per subflow
  3. Review allowed namespaces functionality and reintroduce if missing for Subflow, ForEachItem and the new EmbeddedSubflow
  4. Clarify implementation details on tenant ID handling
  5. Ensure embedded subflows cannot embed themselves to prevent infinite loops

Example showing why this needs to be a dedicated task:

- id: subflow_call
  type: io.kestra.plugin.core.flow.EmbeddedSubflow
  namespace: company.team
  flowId: critical_service
  inputs:
    myInput1: "{{ outputs.t1.myInput }}"
    myInput2: "{{ inputs.myparentInput }}"
  
  # Not available properties
  wait: true
  transmitFailed: true
  labels: map

Adds an EmbeddedFlow that allow to embed subflow tasks into a parent tasks.

Fixes #6518
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: To review
Development

Successfully merging this pull request may close these issues.

Add a flowable EmbeddedSubflow allowing to run subflow's tasks within the same parent execution
3 participants