[flyteadmin] Show diff structure when re-registration two different task with same ids #4924

austin362667 · 2024-02-21T07:14:42Z

Tracking issue

#4762

Why are the changes needed?

To highlight diff, the delta part, of error message for NewTaskExistsDifferentStructureError(), NewWorkflowExistsDifferentStructureError(), NewLaunchPlanExistsDifferentStructureError().
Usually when user submit two different task, workflow, launchplan with the same identifier.

What changes were proposed in this pull request?

By a language agnostic implementation located at flyteadmin.
Add "wI2L/jsondiff" go package for computing structural diff.

How was this patch tested?

Setup process

register two different code, but with the same project, domain, name, version.

Screenshots

Please check the comments below.
I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

Docs link

welcome · 2024-02-21T07:14:45Z

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
Sign off your commits (Reference: DCO Guide).

austin362667 · 2024-02-21T07:19:39Z

successful register task
re-register same task with same id (pass)
re-register different(modified) task with same id (failed)
re-register different(modified) task with same id (failed), with diff error message

austin362667 · 2024-02-21T07:22:17Z

What about Workflow, LaunchPlan?

e.g.,
Modify a task name(function name) used by another workflow

austin362667 · 2024-02-21T07:49:18Z

2. extend the error message to include structured data about what in the spec has changed

What if we just show diff as string, in order to prevent modify too much task proto buff?

Future-Outlier

It looks well to me.

first time registration

second time registration

flyteadmin/pkg/errors/errors.go

flyteadmin/pkg/manager/impl/task_manager.go

Future-Outlier · 2024-02-21T14:27:01Z

I think overall looks well, however I am not familiar with flyteadmin's codebase.
cc @katrogan, can you help review? thank you.

codecov · 2024-02-23T19:05:23Z

Codecov Report

Attention: Patch coverage is 69.56522% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 59.03%. Comparing base (ec0bc4c) to head (04c34ba).

Files	Patch %	Lines
flyteadmin/pkg/manager/impl/task_manager.go	11.11%	8 Missing ⚠️
flyteadmin/pkg/manager/impl/launch_plan_manager.go	0.00%	7 Missing ⚠️
flyteadmin/pkg/manager/impl/workflow_manager.go	33.33%	3 Missing and 1 partial ⚠️
flyteadmin/pkg/errors/errors.go	95.74%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4924      +/-   ##
==========================================
+ Coverage   59.00%   59.03%   +0.02%     
==========================================
  Files         645      645              
  Lines       55672    55726      +54     
==========================================
+ Hits        32850    32896      +46     
- Misses      20226    20233       +7     
- Partials     2596     2597       +1

Flag	Coverage Δ
unittests	`59.03% <69.56%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

austin362667 · 2024-02-23T20:24:08Z

from flytekit import task, workflow
import time

@task(container_image="image1")
def say_hello() -> str:
    time.sleep(1)
    return "Hello, World!"

@workflow
def wf() -> str:
    res = say_hello()
    return res

if __name__ == "__main__":
    print(f"Running hello_world_wf() {wf()}")

from flytekit import task, workflow
import time

@task(container_image="image2")
def say_hello() -> str:
    time.sleep(1)
    return "Hello, World!"

@workflow
def wf() -> str:
    res = say_hello()
    return res

if __name__ == "__main__":
    print(f"Running hello_world_wf() {wf()}")

pyflyte register workflow.py --image basics:v2 --version v1

katrogan

this looks so great! thank you for updating workflow validation as part of this task too

katrogan · 2024-02-23T21:29:26Z

flyteadmin/pkg/errors/errors.go

+
+func NewTaskExistsDifferentStructureError(ctx context.Context, request *admin.TaskCreateRequest, oldSpec *core.TaskTemplate, newSpec *core.TaskTemplate) FlyteAdminError {
+	errorMsg := "task with different structure already exists:\n"
+	// omit source code file object storage path


args/2 is still used for the digest computation, right? any reason to omit it here?

Yes, we still hash the whole task for digest.

The reason to omit file path shown in error message is that it already trapped in "different structure with same id" error, saying code file must be different. So it's trivial to hide the path changed information. cc @pingsutw

Another concern of mine is what if the file path value ("s3://my-s3-bucket/flytesnacks/...") being stored with different key path in the future, for example, args/3.

The code will be broken and hard to locate bugs. Any suggestion?

katrogan · 2024-02-23T21:30:00Z

flyteadmin/pkg/errors/errors.go

+func NewTaskExistsDifferentStructureError(ctx context.Context, request *admin.TaskCreateRequest, oldSpec *core.TaskTemplate, newSpec *core.TaskTemplate) FlyteAdminError {
+	errorMsg := "task with different structure already exists:\n"
+	// omit source code file object storage path
+	diff, _ := jsondiff.Compare(oldSpec, newSpec, jsondiff.Ignores("/Target/Container/args/2"))


why do we need two Compare calls here?

it looks like according to https://pkg.go.dev/github.com/wI2L/jsondiff#Differ.Compare this should output the diff between src & tgt

Because jsondiff.Compare returns a json patch file meaning a series of operations of that json.

It only show the modified target json other than source json. Since we're talking about diff message. It must show the difference between changes from source to target.

diff = jsondiff.Compare(oldTemplate, newTemplate)
rdiff = jsondiff.Compare(newTemplate, oldTemplate
which prefix r means reverse.

Finally compareJsons() zip the diff and its complements rdiff.

flyteadmin/pkg/errors/errors.go

flyteadmin/pkg/manager/impl/task_manager.go

pingsutw · 2024-02-27T05:11:18Z

flyteidl/protos/flyteidl/admin/task.proto

+message TaskErrorExistsDifferentStructure {
+    core.Identifier id = 1;


I think we can remove it

austin362667 · 2024-02-27T11:59:06Z

Task: macos to ubuntu

from flytekit import task, workflow
import time
from flytekit.image_spec import ImageSpec

image_spec = "macos"

@task(container_image=image_spec)
def say_hi() -> str:
    time.sleep(1)
    return "Hello, World!"


@workflow
def wf() -> str:
    res = say_hi()
    return res

if __name__ == "__main__":
    print(f"Running hello_world_wf() {wf()}")

from flytekit import task, workflow
import time
from flytekit.image_spec import ImageSpec

image_spec = "ubuntu"

@task(container_image=image_spec)
def say_hi() -> str:
    time.sleep(1)
    return "Hello, World!"
    

@workflow
def wf() -> str:
    res = say_hi()
    return res

if __name__ == "__main__":
    print(f"Running hello_world_wf() {wf()}")

Workflow: say_hi() to say_hey()

from flytekit import task, workflow
import time
from flytekit.image_spec import ImageSpec

image_spec = "ubuntu"

@task(container_image=image_spec)
def say_hi() -> str:
    time.sleep(1)
    return "Hello, World!"


@workflow
def wf() -> str:
    res = say_hi()
    return res

if __name__ == "__main__":
    print(f"Running hello_world_wf() {wf()}")

from flytekit import task, workflow
import time
from flytekit.image_spec import ImageSpec

image_spec = "ubuntu"

@task(container_image=image_spec)
def say_hey() -> str:
    time.sleep(1)
    return "Hello, World!"
    

@workflow
def wf() -> str:
    res = say_hey()
    return res

if __name__ == "__main__":
    print(f"Running hello_world_wf() {wf()}")

katrogan

looks great, just one question about map comparison determinism

katrogan · 2024-03-05T17:23:45Z

flyteadmin/pkg/errors/errors.go

+
+func NewTaskExistsDifferentStructureError(ctx context.Context, request *admin.TaskCreateRequest, oldSpec *core.CompiledTask, newSpec *core.CompiledTask) FlyteAdminError {
+	errorMsg := "task with different structure already exists:\n"
+	diff, _ := jsondiff.Compare(oldSpec, newSpec)


since in golang map ordering is random/never guaranteed, I'm worried that converting the specs to bytes here might lead to non deterministic byte arrays

when we compute the digest, we use pbhash to marshal the protos to json

should we do the same here before calling Compare()?

alternatively, do you mind double checking that jsondiff.Compare() returns no diff for multiple calls comparing 2 identical pb objects that contain maps in the message?

can!
I've carefully double checked that jsondiff.Compare() is an identical operation w.r.t determinism. Also added the test for this function.

thanks @austin362667 for the super fast turnaround!

in this case, i was more concerned about identical maps not showing a false diff - would you mind double checking that? otherwise PR looks great!!

flyteadmin/pkg/manager/impl/launch_plan_manager.go

flyteadmin/pkg/manager/impl/task_manager.go

katrogan

this so awesome, thank you!

Signed-off-by: Austin Liu <austin362667@gmail.com>

Signed-off-by: Austin Liu <austin362667@gmail.com> format Signed-off-by: Austin Liu <austin362667@gmail.com>

Signed-off-by: Austin Liu <austin362667@gmail.com>

Signed-off-by: Austin Liu <austin362667@gmail.com> rollback proto change Signed-off-by: Austin Liu <austin362667@gmail.com>

Signed-off-by: Austin Liu <austin362667@gmail.com>

Future-Outlier

LGTM, Let's merge it when all tests passed

welcome · 2024-03-27T07:31:45Z

Congrats on merging your first pull request! 🎉

dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. enhancement New feature or request labels Feb 21, 2024

Future-Outlier reviewed Feb 21, 2024

View reviewed changes

katrogan reviewed Feb 23, 2024

View reviewed changes

austin362667 force-pushed the draft/flytekit4762 branch from 2dd7cc0 to 321b2e7 Compare February 25, 2024 10:44

pingsutw reviewed Feb 27, 2024

View reviewed changes

dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Feb 27, 2024

austin362667 force-pushed the draft/flytekit4762 branch from 72ce445 to 2b8e6a1 Compare February 27, 2024 11:49

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Feb 27, 2024

austin362667 requested review from katrogan, pingsutw and Future-Outlier February 27, 2024 11:52

austin362667 mentioned this pull request Mar 1, 2024

Remove verbose debug message flyteorg/flytekit#2227

Merged

3 tasks

katrogan reviewed Mar 5, 2024

View reviewed changes

katrogan previously approved these changes Mar 8, 2024

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 8, 2024

austin362667 added 3 commits March 27, 2024 15:01

add json diff module

812b0c1

Signed-off-by: Austin Liu <austin362667@gmail.com>

add CreateTaskFailureReason

c424646

Signed-off-by: Austin Liu <austin362667@gmail.com>

update task proto buff gen code

fb7de5f

Signed-off-by: Austin Liu <austin362667@gmail.com>

austin362667 added 10 commits March 27, 2024 15:02

add task diff DifferentStructureError msg

f37a90e

Signed-off-by: Austin Liu <austin362667@gmail.com>

prettify json error message & rollback idl change

713f93a

Signed-off-by: Austin Liu <austin362667@gmail.com>

refactor to compiledWorkflow

2718106

Signed-off-by: Austin Liu <austin362667@gmail.com>

clean up

39a8c39

Signed-off-by: Austin Liu <austin362667@gmail.com>

add create task errors test

7347706

Signed-off-by: Austin Liu <austin362667@gmail.com> format Signed-off-by: Austin Liu <austin362667@gmail.com>

cleanup error & test

38a8357

Signed-off-by: Austin Liu <austin362667@gmail.com>

rollback proto change

1663a7b

Signed-off-by: Austin Liu <austin362667@gmail.com> rollback proto change Signed-off-by: Austin Liu <austin362667@gmail.com>

add launchplan

804179e

Signed-off-by: Austin Liu <austin362667@gmail.com>

cleanup

cff01cf

Signed-off-by: Austin Liu <austin362667@gmail.com>

test: add no diff case

04c34ba

Signed-off-by: Austin Liu <austin362667@gmail.com>

austin362667 dismissed katrogan’s stale review via 04c34ba March 27, 2024 07:10

austin362667 force-pushed the draft/flytekit4762 branch from c6f732d to 04c34ba Compare March 27, 2024 07:10

Future-Outlier approved these changes Mar 27, 2024

View reviewed changes

Future-Outlier enabled auto-merge (squash) March 27, 2024 07:22

Future-Outlier disabled auto-merge March 27, 2024 07:22

Future-Outlier enabled auto-merge (squash) March 27, 2024 07:25

Future-Outlier merged commit a5e2733 into flyteorg:master Mar 27, 2024
47 of 48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flyteadmin] Show diff structure when re-registration two different task with same ids #4924

[flyteadmin] Show diff structure when re-registration two different task with same ids #4924

austin362667 commented Feb 21, 2024 •

edited

Loading

welcome bot commented Feb 21, 2024

austin362667 commented Feb 21, 2024 •

edited

Loading

austin362667 commented Feb 21, 2024 •

edited

Loading

austin362667 commented Feb 21, 2024

Future-Outlier left a comment

Future-Outlier commented Feb 21, 2024

codecov bot commented Feb 23, 2024 •

edited

Loading

austin362667 commented Feb 23, 2024 •

edited

Loading

katrogan left a comment

katrogan Feb 23, 2024

austin362667 Feb 25, 2024 •

edited

Loading

austin362667 Feb 25, 2024 •

edited

Loading

katrogan Feb 23, 2024

austin362667 Feb 25, 2024

pingsutw Feb 27, 2024

austin362667 Feb 27, 2024

austin362667 commented Feb 27, 2024

katrogan left a comment

katrogan Mar 5, 2024

austin362667 Mar 5, 2024 •

edited

Loading

katrogan Mar 5, 2024

austin362667 Mar 8, 2024

katrogan left a comment

Future-Outlier left a comment

welcome bot commented Mar 27, 2024

		message TaskErrorExistsDifferentStructure {
		core.Identifier id = 1;

[flyteadmin] Show diff structure when re-registration two different task with same ids #4924

[flyteadmin] Show diff structure when re-registration two different task with same ids #4924

Conversation

austin362667 commented Feb 21, 2024 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Related PRs

Docs link

welcome bot commented Feb 21, 2024

austin362667 commented Feb 21, 2024 • edited Loading

austin362667 commented Feb 21, 2024 • edited Loading

austin362667 commented Feb 21, 2024

Future-Outlier left a comment

Choose a reason for hiding this comment

first time registration

second time registration

Future-Outlier commented Feb 21, 2024

codecov bot commented Feb 23, 2024 • edited Loading

Codecov Report

austin362667 commented Feb 23, 2024 • edited Loading

katrogan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin362667 Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

austin362667 Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin362667 commented Feb 27, 2024

katrogan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin362667 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katrogan left a comment

Choose a reason for hiding this comment

Future-Outlier left a comment

Choose a reason for hiding this comment

welcome bot commented Mar 27, 2024

austin362667 commented Feb 21, 2024 •

edited

Loading

austin362667 commented Feb 21, 2024 •

edited

Loading

austin362667 commented Feb 21, 2024 •

edited

Loading

codecov bot commented Feb 23, 2024 •

edited

Loading

austin362667 commented Feb 23, 2024 •

edited

Loading

austin362667 Feb 25, 2024 •

edited

Loading

austin362667 Feb 25, 2024 •

edited

Loading

austin362667 Mar 5, 2024 •

edited

Loading