-
Notifications
You must be signed in to change notification settings - Fork 774
[Refactor]: CreateRun add initial service and response structure #6853
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
base: v2
Are you sure you want to change the base?
Conversation
Signed-off-by: “Alex <alexchien130@gmail.com>
Signed-off-by: “Alex <alexchien130@gmail.com>
Signed-off-by: “Alex <alexchien130@gmail.com>
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Signed-off-by: “Alex <alexchien130@gmail.com>
Signed-off-by: “Alex <alexchien130@gmail.com>
Signed-off-by: Alex Chien <115371007+yuhuan130@users.noreply.github.com>
Signed-off-by: “Alex <alexchien130@gmail.com>
|
Could you please add a script for testing under https://github.com/flyteorg/flyte/tree/v2/runs/test/scripts and test this API manually to ensure the runs are created successfully? |
Signed-off-by: “Alex <alexchien130@gmail.com>
|
Hi @yuhuan130 , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we keep one buf request only and commented out others, so that when we execute this script, there's only one run being created.
We can do things like below. I keep creating run with run_id as default as this is something that we will use the most
# Create run by run ID
# buf curl --schema . "$ENDPOINT/flyteidl2.workflow.RunService/CreateRun" --data @- <<EOF
# {
# "run_id": {
# "org": "$ORG",
# "project": "$PROJECT",
# "domain": "$DOMAIN",
# "name": "my-custom-run-123"
# },
# "task_id": {
# "org": "$ORG",
# "project": "$PROJECT",
# "domain": "$DOMAIN",
# "name": "$TASK_NAME",
# "version": "$VERSION"
# },
# "source": "RUN_SOURCE_WEB"
# }
# EOF
# Create run by project ID
buf curl --schema . "$ENDPOINT/flyteidl2.workflow.RunService/CreateRun" --data @- <<EOF
{
"project_id": {
"organization": "$ORG",
"name": "$PROJECT",
"domain": "$DOMAIN"
},
"task_spec": {
"short_name": "test"
},
"source": "RUN_SOURCE_CLI"
}
EOFThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, super quick confirm, you mentioned run_id as the default since it's most commonly used. But I think your example showed it the other way around, so I wanted to double-check before making the change. Thanks!
| const InitialPhase = "PHASE_QUEUED" | ||
|
|
||
| // CreateRunRequestToModel converts CreateRunRequest protobuf to Run domain model | ||
| func CreateRunRequestToModel(ctx context.Context, req *workflow.CreateRunRequest) (*models.Run, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer isolate service layer and repository layer to prevent the transformer function depends on service request. That is, let's not pass the CreateRunRequest and transform here. Instead, do things in service layer, and just pass runID and actionSpec into here. Like what we did in:
flyte/runs/service/task_service.go
Lines 53 to 54 in 442bcc1
| taskId := request.GetTaskId() | |
| taskModel, err := transformers.NewTaskModel(ctx, taskId, taskSpec) |
In this case, we can reuse the run proto to run model transform function in other places rather than only for create run function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh~ I see. No problem
|
For the check generate CI failure, we removed |
Signed-off-by: “Alex <alexchien130@gmail.com>
Tracking issue
Closes part 1 of #6824
Why are the changes needed?
Currently,
CreateRunpasses protobuf requests directly to the repository layer, violating separation of concerns. This creates tight coupling between the API and data layers, making testing harder and inconsistent with the pattern intask_service.go(PR #6813).What changes were proposed in this pull request?
Refactored
CreateRunto use the transformer pattern:New Files:
runs/repository/transformers/run.go- Converts protobuf → domain modelCreateRunRequestToModel: Transforms incoming request to domain modelRunModelToCreateRunResponse: Builds complete API response with Action hierarchy (ID, Status, Metadata)Modified Files:
runs/service/create_run.go- Uses transformers instead of passing protobuf directlyruns/repository/interfaces/action.go- Changed signature to accept*models.Runruns/repository/impl/action_repo.go- Accepts domain models, removed protobuf logicruns/service/run_service_test.go- Updated mocks and testsruns/repository/mocks/mock_ActionRepo.go- Regenerated mocks to match new interface signatureKey improvements:
task_service.goRunModelToCreateRunResponsetransformer to build full API response hierarchy (includes Action ID, Status with phase/timestamps, and Metadata with source)How was this patch tested?
go test ./runs/... -vScreenshots