Single task execution (admin-generated workflow implementation) #96
Conversation
katrogan
requested review from
anandswaminathan,
EngHabu,
kumare3,
matthewphsmith and
wild-endeavor
as code owners
|
Is this really "v1". I'd rather not call it that. |
katrogan
changed the title
|
@wild-endeavor updated but names are hard. let me know if you have suggestions |
Codecov Report
@@ Coverage Diff @@
## master #96 +/- ##
==========================================
- Coverage 63.14% 60.46% -2.69%
==========================================
Files 100 101 +1
Lines 7038 7451 +413
==========================================
+ Hits 4444 4505 +61
- Misses 2082 2409 +327
- Partials 512 537 +25
Continue to review full report at Codecov.
|
|
|
||
| func generateNodeNameFromTask(taskName string) string { | ||
| if len(taskName) >= maxNodeIDLength { | ||
| taskName = taskName[:maxNodeIDLength] |
There was a problem hiding this comment.
nit: python tasks are typically on the form "app.workflows.tasks.......my_module.my_task", I think naming the node "my-task" would make it much easier to use and recognize. no?
if so then maybe taskName[len(taskName)-maxNodeIDLength:] (I might be off by 1 :))
There was a problem hiding this comment.
excellent point! updated
| ctx context.Context, request admin.ExecutionCreateRequest, db repositories.RepositoryInterface, | ||
| workflowManager interfaces.WorkflowInterface, namedEntityManager interfaces.NamedEntityInterface, taskIdentifier *core.Identifier, | ||
| task *admin.Task) (*models.Workflow, error) { | ||
| workflowModel, err := db.WorkflowRepo().Get(ctx, repositoryInterfaces.GetResourceInput{ |
There was a problem hiding this comment.
Doesn't this introduce a race condition? if I tried to run the same task twice, both of them go past the "Get" call and realize there isn't an existing WF model, so they will both try to create one... Should the error check for Create() call below exclude "ErrExistingRow" or whatever the error is for that?
There was a problem hiding this comment.
yes, good call. done
Added the filter to the ListNamedEntity endpoint which is what the console uses to list workflows and is the entrypoint for individual workflows I'm more hesitant about adding it to the existing ListWorkflows endpoint by default because now that will force an additional join and I don't want to add a performance penalty to an endpoint that is really only available in the cli (aka power users who hopefully know what they're doing :) ) |
|
friendly ping @EngHabu |
Co-authored-by: Haytham AbuelFutuh <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #96 +/- ##
==========================================
+ Coverage 63.14% 64.33% +1.19%
==========================================
Files 100 101 +1
Lines 7038 7678 +640
==========================================
+ Hits 4444 4940 +496
- Misses 2082 2185 +103
- Partials 512 553 +41
Continue to review full report at Codecov.
|
|
@EngHabu mind reviewing one last time? your suggested commit dismissed your review :) |
TL;DR
This PR implements single task execution for CreateExecutionRequests with reference entities of type 'TASK'.
Because this is v1 of single task execution this code is compartmentalized and designed to be easy to rip out. Therefore not everything that could necessarily be refactored from the existing CreateExecution, CreateWorfklow and CreateLaunchPlan logic is changed and some duplicate code exists. This is (ugly) by design.
TODO If this overall design looks okay I will add more unit tests to this PR - I just wanted to get an overall approval of the code before investing time in unit tests in case it changes significantly.
Type
Are all requirements met?
Complete description
This code takes an existing, registered task and generates a skeleton workflow definition with a single node referencing the task. The workflow inputs and outputs are mapped to those of the task definition. For UI purposes, a default launch plan with a matching interface is also generated.
Tracking Issue
flyteorg/flyte#256
Follow-up issue
flyteorg/flyte#221