More data in workflow exists err msg #2522 #487
Conversation
jerempy
requested review from
katrogan,
wild-endeavor,
anandswaminathan,
EngHabu,
kumare3 and
pmahindrakar-oss
as code owners
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
|
Should we do the same for tasks? |
pkg/manager/impl/workflow_manager.go
Outdated
| // A workflow exists with different structure - sending much data to help debug on sdk end | ||
| return nil, errors.NewFlyteAdminErrorf( | ||
| codes.InvalidArgument, | ||
| "workflow with different structure already exists with that id: %v. workflow model: %+v.", |
There was a problem hiding this comment.
if I'm understanding the original issue correctly, we want flytekit to be able to fetch the existing workflow with an identical digest and then do the comparison on the client-side in order to surface what in the workflow structure has specifically changed.
Right now the existingMatchingWorkflow type is an internal db model, with a serialized closure that's not particularly parseable by the flytekit client
I think what we should do is add a proper error type with a detailed structure, similar to these to return the existing matching workflow identifier and return this new error in this block. That way the flytekit client can then do a follow-up api call to Get the matching workflow, which will return the full workflow closure in a structured format that flytekit error handling can sensibly use to compare to the workflow definition it is trying to register
There was a problem hiding this comment.
@katrogan awesome feedback. thanks much! I took another shot at it. Let me know what you think and looks good, then I'll re-do the python end to in this PR: flyteorg/flytekit#1235
|
awesome! I think for completeness you might first need to make a change in flyteidl, much like we have for the current EventFailureReasons which allow us to specify the Since your change isn't related to event handling I think the best thing would be to define a new message in the flyteadmin idl, perhaps in workflow.proto to capture WorkflowCreateFailureReason or something similar. |
|
hey @jerempy thanks for making the idl changes! I think you should be able to pick them up here by pinning the idl version in go.mod with your new workflow error |
|
@katrogan :high-five-emoji: 😎 and we're back. Ready for review: I added in the new protos |
|
@EngHabu I created the new issue for tracking doing this with tasks: flyteorg/flyte#3027 🙏 if you could review this one again plz. |
pkg/errors/errors.go
Outdated
| @@ -16,10 +16,16 @@ type FlyteAdminError interface { | |||
| Error() string | |||
| Code() codes.Code | |||
| GRPCStatus() *status.Status | |||
| WithDetails(details *admin.EventFailureReason) (FlyteAdminError, error) | |||
| WithDetails(details ProtoMessager) (FlyteAdminError, error) | |||
There was a problem hiding this comment.
I believe there is a proto.Message interface that looks awfully similar. maybe take a look at that?
There was a problem hiding this comment.
@EngHabu you're right! Found it and plugged it int. Thanks
Codecov Report
@@ Coverage Diff @@
## master #487 +/- ##
==========================================
+ Coverage 61.62% 61.66% +0.03%
==========================================
Files 158 158
Lines 11500 11524 +24
==========================================
+ Hits 7087 7106 +19
- Misses 3679 3682 +3
- Partials 734 736 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Any insight what I should do here about the missing project code coverage? 😅 |
pkg/manager/impl/workflow_manager.go
Outdated
| "workflow with different structure already exists with id %v", request.Id) | ||
| // A workflow exists with different structure | ||
| existingCtx := getWorkflowContext(ctx, request.Id) | ||
| existingWorkflow, err := util.GetWorkflow(existingCtx, w.db, w.storageClient, *request.Id) |
There was a problem hiding this comment.
instead of calling get again on the existing workflow, can we just have NewWorkflowExistsDifferentStructureError take in a core.Identifier, since it doesn't need the full admin protobuf def?
return more data if workflow already exists with different structure Signed-off-by: jerempy <[email protected]>
So we send back the workflow instead of request id Signed-off-by: jerempy <[email protected]>
new proto buffers in flyteidl Signed-off-by: jerempy <[email protected]>
Signed-off-by: jerempy <[email protected]>
Signed-off-by: jerempy <[email protected]>
just pass the request.Id instead Signed-off-by: jerempy <[email protected]>
|
@katrogan done. looks better this way. nice call. |
|
Congrats on merging your first pull request! 🎉 |
Return more data if workflow already exists with different structure. Will have another PR in the python flytekit to compare the values. Flytekit PR: flyteorg/flytekit#1235
TL;DR
More data in err message for #2522
Type
Are all requirements met?
Complete description
How did you fix the bug, make the feature etc. Link to any design docs etc
Tracking Issue
Fixes flyteorg/flyte#2522
Follow-up issue
NA
OR
https://github.com/flyteorg/flyte/issues/