wip: feat(engine): dags-as-durable-tasks#4266
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
|
|
|
|
|
|
This reverts commit abc0dc7.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| ), | ||
| cell: ({ row }) => { | ||
| if (row.getCanExpand()) { | ||
| if (row.original.type === 'DAG') { |
There was a problem hiding this comment.
this lets us open the workflow run detail page for an operator DAG before any of the tasks have actually been spawned
| [taskRuns], | ||
| () => | ||
| taskRuns | ||
| .filter((t) => t.isDurable && t.taskExternalId !== id) |
There was a problem hiding this comment.
there's a bunch of stuff like this - basically removing the orchestrator from the ids
| if e.isDagOrchestrator { | ||
| continue | ||
| } |
There was a problem hiding this comment.
don't want to render spans for the orchestrator task since it messes up nesting and is "internal" / not user facing
| d.l.Warn().Ctx(ctx).Err(err).Msg("failed to unmarshal input") | ||
| continue | ||
| if len(currInput.DagParentTaskRunIds) > 0 { | ||
| dagParentOutputs, err := d.repov1.Tasks().GetDagParentOutputs(ctx, tenantId, currInput.DagParentTaskRunIds) |
There was a problem hiding this comment.
not sure what to do about this - seems very suboptimal to need a db lookup here, will circle back
| continue | ||
| } | ||
| } else { | ||
| childEvents, err := s.repov1.Tasks().ListDurableOrchestratorChildOutputEvents(ctx, tenantId, event.TaskExternalId) |
There was a problem hiding this comment.
same here - really not ideal to need the db lookup, will circle back
| return nil | ||
| } | ||
|
|
||
| func (d *DispatcherServiceImpl) TriggerDAGStep(ctx context.Context, tenantId uuid.UUID, req *operator.DAGStepTriggerRequest) (*operator.DAGStepTriggerResult, error) { |
There was a problem hiding this comment.
this is one of the important methods, since it's what gets called by the operator to trigger tasks
| TriggerTaskData: &v1.TriggerTaskData{ | ||
| WorkflowName: req.WorkflowName, | ||
| TargetActionId: &req.ActionId, | ||
| UserMessage: &stepLabel, | ||
| Data: []byte(req.Input), | ||
| AdditionalMetadata: task.AdditionalMetadata, | ||
| ParentExternalId: &task.ExternalID, | ||
| ParentTaskId: &task.ID, | ||
| ParentTaskInsertedAt: &task.InsertedAt.Time, | ||
| ChildIndex: &childIndex, | ||
| DagParentTaskRunIds: req.DagParentTaskRunIds, | ||
| IsSkipped: req.IsSkipped, | ||
| IsCancelled: req.IsCancelled, | ||
| DesiredWorkerLabels: req.DesiredWorkerLabels, | ||
| WorkflowRunId: &orchestratorWorkflowRunId, |
There was a problem hiding this comment.
important stuff here:
- we basically pass most things along (e.g. the metadata, the input, etc.)
- we tie the workflow run id manually to the orchestrator's workflow run id
| if t.output != nil { | ||
| b, err := json.Marshal(t.output) | ||
| if err == nil { | ||
| output[t.readableId] = json.RawMessage(b) |
There was a problem hiding this comment.
this results in a DAG output that maps step ids to outputs, similar to what we see in the dash and over the API
| -- fixme: this might be really slow | ||
| AND NOT EXISTS ( | ||
| SELECT 1 | ||
| FROM v1_tasks_olap t | ||
| WHERE | ||
| t.external_id = r.parent_task_external_id | ||
| AND COALESCE(t.is_dag_orchestrator, FALSE) | ||
| AND t.tenant_id = r.tenant_id | ||
| ) |
There was a problem hiding this comment.
so, this makes me very nervous, but I'm not sure how else to do this. Basically, we need to remove any dag children from the runs list query, since they'll be double counted (we should only include dag parents, dags, and standalone tasks)
There was a problem hiding this comment.
probably a broader conversation, but should we not be treating the OLAP side of things as static, so we just insert data as if we're inserting DAGs?
There was a problem hiding this comment.
ah, I didn't even think about that... 🤔 it would make all the API surface stuff a lot easier
There was a problem hiding this comment.
I guess the negative is we'd still need v1_dags_olap, v1_dag_to_task_olap, etc.?
There was a problem hiding this comment.
yeah - I'm not sure it's a long-term fix, but short-term it would make our lives easier (obviously just moves complexity somewhere else)
There was a problem hiding this comment.
lemme see if I can get it to work. maybe when we eventually do V2, we can do that second push for OLAP since we potentially won't need to support as much old API surface
|
|
|
|
| -- fixme: this might be really slow | ||
| AND NOT EXISTS ( | ||
| SELECT 1 | ||
| FROM v1_tasks_olap t | ||
| WHERE | ||
| lt.tenant_id = $1::uuid | ||
| AND lt.external_id = $11::UUID | ||
| t.external_id = r.parent_task_external_id | ||
| AND COALESCE(t.is_dag_orchestrator, FALSE) | ||
| AND t.tenant_id = r.tenant_id | ||
| ) |
|
|
| type dag struct { | ||
| requestCh chan<- *v1contracts.DurableTaskRequest | ||
| requestCh chan<- *v1contracts.DurableTaskRequest | ||
| matchRepo repository.MatchRepository |
There was a problem hiding this comment.
I think pulling out the EvaluateBoolExpr into a subpackage with essentially no dependencies (particularly no DB dependencies) would make it much easier to test this file.
This also seems like a good candidate for table-test unit testing with a set of comprehensive examples; in particular, we'd likely want code coverage on all of the feature paths (wait conditions, etc).
There was a problem hiding this comment.
I'm aware that building out testing for this file would require the test to sort of act like its own mini workflow engine, since it needs to be able to receive and send durable task req/responses over the channels...I'm assuming that hardcoding when we send data over these channels for each test is simple enough that this is doable, but I could also see a world where it's complex enough that we don't want to do this
| if !ok { | ||
| return fmt.Errorf("durable task session closed") | ||
| } | ||
| d.taskConsumer(resp) |
There was a problem hiding this comment.
q: did we ever merge that "stable callback ordering" PR? That's required for this to work
There was a problem hiding this comment.
not yet, but yeah, agreed - there are some open comments on that one still
| isTriggered bool | ||
| isSkipped bool | ||
| errorMessage string | ||
| output map[string]interface{} |
There was a problem hiding this comment.
I don't think this is tractable, I think we need to find a way to not keep the output of the task in memory for longer than necessary. Perhaps we can invert the expression evaluation to evaluate the output when the parent completes instead of evaluating it on the child.
| for _, parent := range task.parents { | ||
| if !parent.isCompleted { | ||
| areParentsSatisfied = false | ||
| for _, t := range d.tasks { |
There was a problem hiding this comment.
I wonder if it's more beneficial to keep a list of pendingTasks versus finalizedTasks so that we only loop through pendingTasks which could theoretically still emit an event
| } | ||
| } | ||
|
|
||
| result, err := d.triggerStep(ctx, t.actionId, t.workflowName, t.index, parentTaskRunIds, skip, cancelled) |
There was a problem hiding this comment.
does this need to be synchronous / blocking the loop, or can we add to a waitgroup and evaluate at the bottom of taskEmitter?
There was a problem hiding this comment.
Perhaps this wouldn't get us anything though, because we need to guard the event log which I imagine we touch when we call this method, so we serialize it anyway
There was a problem hiding this comment.
nit: a quick pass adding spans/traces throughout this file would be awesome
| -- fixme: this might be really slow | ||
| AND NOT EXISTS ( | ||
| SELECT 1 | ||
| FROM v1_tasks_olap t | ||
| WHERE | ||
| t.external_id = r.parent_task_external_id | ||
| AND COALESCE(t.is_dag_orchestrator, FALSE) | ||
| AND t.tenant_id = r.tenant_id | ||
| ) |
There was a problem hiding this comment.
probably a broader conversation, but should we not be treating the OLAP side of things as static, so we just insert data as if we're inserting DAGs?
| return fmt.Errorf("could not marshal DAG operator config: %w", err) | ||
| } | ||
|
|
||
| _, err = a.repo.Operators().CreateOperator(ctx, tenantId, v1.CreateOperatorOpts{ |
There was a problem hiding this comment.
it looks like we only call this on PutWorkflow. Should we be calling this from other places, or is the thinking that this only really enables when a user restarts their workers?
There was a problem hiding this comment.
I was thinking it was fine to only do this here since the isDagOperator flag is tied to the WorkflowVersion, so until someone registers a new workflow version with a dag, we'd never end up needing to call this, which I think is fine. We could also call it on startup I guess?
|
|
|
|
Description
WIP
Initial implementation of the dag operator, which basically creates an internal durable task that processes the dag via child spawning
to do:
workflow_run_idfor the dag children correspond's to the durable task external idwait_for(sleep), we get a bunch of "could not send task to worker'Fixes # (issue)
Type of change
What's Changed
Checklist
Changes have been:
🤖 AI Disclosure