JSON IDL #5542

Future-Outlier · 2024-07-05T19:19:38Z

Tracking issue

#5318

Why are the changes needed?

solve more than 10+ issues due to the protobuf struct doesn't support int.
using msgpack bytes, which are smaller than JSON bytes, we can reduce storage size and improve I/O performance when interacting with database.

What changes were proposed in this pull request?

flyteidl

add JSON IDL type

flytepropeller

attribute access for remote execution
backward compatibility (compiler)
upstreamType (Struct) -> downstreamType (Json)

flytectl

flytectl create execution with inputs
flytectl get execution

How was this patch tested?

unit tests and remote execution.
Example are from here

Setup process

Screenshots

[flytepropeller] attribute access

dataclass example

[flytepropeller] backward compatible

reference task
upstream (struct) -> downstream (json)

cache
upstream (struct) -> downstream (json)

[flytepropeller] others

dict example

[flytectl] create execution with inputs

(dev) future@outlier ~ % flytectl create execution --execFile build/PR/JSON/demo/flytectl/create_json_dataclass.yaml -p flytesnacks -d development

execution identifier project:"flytesnacks" domain:"development" name:"f6dee626da95445a8894"

iamRoleARN: ""
inputs:
    input:
        a: 1
        b: 3.14
        c: example_string
        d:
            "1": 100
            "2": 200
        e:
            a: 1
            b: 3.14
envs: {}
kubeServiceAcct: ""
targetDomain: ""
targetProject: ""
task: dataclass_example.dataclass_task
version: x1QUSQo64Vd9FuIo7xBkvg

[flytectl] get task

(dev) future@outlier ~ % flytectl get task -d development -p flytesnacks dataclass_example.dataclass_task --execFile build/PR/JSON/demo/flytectl/get_json_dataclass.yaml --version x1QUSQo64Vd9FuIo7xBkvg
 ------------------------ ---------------------------------- ------------- -------- --------- -------------- ------------------- ----------------------------- 
| VERSION                | NAME                             | TYPE        | INPUTS | OUTPUTS | DISCOVERABLE | DISCOVERY VERSION | CREATED AT                  |
 ------------------------ ---------------------------------- ------------- -------- --------- -------------- ------------------- ----------------------------- 
| x1QUSQo64Vd9FuIo7xBkvg | dataclass_example.dataclass_task | python-task | input  | o0      |              |                   | 2024-08-26T12:27:05.188306Z |
 ------------------------ ---------------------------------- ------------- -------- --------- -------------- ------------------- ----------------------------- 
1 rows

iamRoleARN: ""
inputs:
    input: []
envs: {}
kubeServiceAcct: ""
targetDomain: ""
targetProject: ""
task: dataclass_example.dataclass_task
version: x1QUSQo64Vd9FuIo7xBkvg

Note: For struct and JSON, the input will be an empty {} or []. This is an existing bug, and I am not planning to fix it in this PR. I genuinely believe this is a relatively low-priority issue."

Check all the applicable boxes

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

Related PRs

Docs link

codecov · 2024-07-05T19:23:36Z

Codecov Report

Attention: Patch coverage is 28.76712% with 104 lines in your changes missing coverage. Please review.

Project coverage is 34.61%. Comparing base (0da324b) to head (27dbc63).
Report is 177 commits behind head on master.

Files with missing lines	Patch %	Lines
flyteidl/gen/pb-go/flyteidl/core/literals.pb.go	12.72%	96 Missing ⚠️
flyteidl/clients/go/coreutils/literals.go	76.47%	7 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5542      +/-   ##
==========================================
- Coverage   36.18%   34.61%   -1.57%     
==========================================
  Files        1302      990     -312     
  Lines      109666    85001   -24665     
==========================================
- Hits        39678    29424   -10254     
+ Misses      65841    52667   -13174     
+ Partials     4147     2910    -1237

Flag	Coverage Δ
unittests-datacatalog	`51.37% <ø> (ø)`
unittests-flyteadmin	`55.63% <ø> (+0.30%)`	⬆️
unittests-flytecopilot	`12.17% <ø> (ø)`
unittests-flytectl	`62.17% <ø> (-0.02%)`	⬇️
unittests-flyteidl	`7.21% <28.76%> (+0.08%)`	⬆️
unittests-flyteplugins	`53.35% <ø> (+<0.01%)`	⬆️
unittests-flytepropeller	`?`
unittests-flytestdlib	`55.35% <ø> (ø)`

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.

Signed-off-by: Yee Hing Tong <[email protected]>

Signed-off-by: Yee Hing Tong <[email protected]> Signed-off-by: Future-Outlier <[email protected]>

Signed-off-by: Future-Outlier <[email protected]>

…and support json idl attribute access Signed-off-by: Future-Outlier <[email protected]>

Signed-off-by: Future-Outlier <[email protected]>

Future-Outlier · 2024-08-05T06:10:12Z

flytepropeller/pkg/compiler/validators/typing.go

+	if upstreamTypeCopy.GetSimple() == flyte.SimpleType_STRUCT && downstreamTypeCopy.GetSimple() == flyte.SimpleType_JSON {
+		return true
+	}


This is for case like:

@reference_task( project="flytesnacks", domain="development", name="create_reference_task.creat_reference_task_dict", version="nNDnS6m4_fo-GXPxVUm2TA", ) def t1( ) -> dict: # return STRUCT ... @task(container_image=image) def t2(input: dict) -> dict: # input STRUCT, return JSON print(input) for key, value in input.items(): print(f'Key: {key}, Value: {value}') print(f'Key: {type(key)}, Value: {type(value)}') return input @workflow def wf() -> dict: d1 = t1() d2 = t2(input=d1) return d2

Future-Outlier · 2024-08-08T12:09:39Z

flyteidl/clients/go/coreutils/literals.go

+		if newT.Simple == core.SimpleType_JSON {
+			if _, isValueStringType := v.(string); !isValueStringType {
+				jsonBytes, err := json.Marshal(v)
+				if err != nil {
+					return nil, fmt.Errorf("unable to marshal to json string for json value %v", v)
+				}
+				jsonBytes, err = msgpack.Marshal(jsonBytes)
+				if err != nil {
+					return nil, fmt.Errorf("unable to marshal to msgpack bytes for json value %v", v)


for creating execution by flytectl.

flytectl create execution --execFile build/PR/JSON/demo/flytectl/create_json_dataclass.yaml -p flytesnacks -d development

Signed-off-by: Future-Outlier <[email protected]>

Future-Outlier · 2024-08-28T08:21:39Z

flytepropeller/pkg/controller/nodes/attr_path_resolver_test.go

+func TestResolveAttrPathInJson(t *testing.T) {
+	// Helper function to convert a map to JSON and then to msgpack


Test the map type first, then test the list type.

Within each category, order the tests by data type: integers (int), floats (float), strings (string), and nested maps or lists.

Finally, arrange the exception case tests.

Signed-off-by: Future-Outlier <[email protected]>

flyteidl/clients/go/coreutils/literals_test.go

flytepropeller/pkg/controller/nodes/attr_path_resolver.go

Signed-off-by: Future-Outlier <[email protected]>

flyteidl/protos/flyteidl/core/literals.proto

pingsutw · 2024-08-30T07:54:38Z

flytepropeller/pkg/controller/nodes/attr_path_resolver.go

+	}
+	currVal = convertNumbers(tmpVal)
+
+	// Turn the current value to a map so it can be resolved more easily


hmm, where did you turn the current value to a map?

This is my fault, nice catch!!

pingsutw · 2024-08-30T07:56:12Z

flytepropeller/pkg/controller/nodes/attr_path_resolver.go

+	return literal, nil
+}
+
+// convertJSONToLiteral converts the protobuf struct (e.g. dataclass) to literal


Suggested change

// convertJSONToLiteral converts the protobuf struct (e.g. dataclass) to literal

// convertJSONToLiteral converts the JSON (e.g. dataclass) to literal

Future-Outlier · 2024-08-30T08:08:24Z

@eapolinario Can you review?
Thank you

Signed-off-by: Future-Outlier <[email protected]> Co-authored-by: Kevin Su <[email protected]>

Signed-off-by: Future-Outlier <[email protected]>

eapolinario

Can you comment on the use of github.com/vmihailenco/msgpack to handle msgpack-encoded values? I see that this is not listed in the official msgpack website.

Signed-off-by: Future-Outlier <[email protected]>

…aultLiteralForType Signed-off-by: Future-Outlier <[email protected]>

Signed-off-by: Future-Outlier <[email protected]>

wild-endeavor · 2024-09-09T06:29:24Z

flyteidl/protos/flyteidl/core/literals.proto

+
+    // The format used to serialize the byte array.
+    // This field identifies the specific format of the serialized JSON data,
+    // allowing future flexibility in supporting different JSON variants.


add a comment here saying that serialization formats need to be supported in python, golang, rust, and javascript, in order for the full flyte experience to work. (just throwing rust in there for now, we don't have anything yet).

Yes, you're right, this is a pretty good and important advice, thank you.

wild-endeavor · 2024-09-09T06:30:05Z

flyteidl/protos/flyteidl/core/literals.proto

+
+    // The format used to serialize the byte array.
+    // This field identifies the specific format of the serialized JSON data,
+    // allowing future flexibility in supporting different JSON variants.


remove the json part of this comment, it's no longer true.

yes, thank you.

Future-Outlier · 2024-09-14T00:42:42Z

Move to #5742

Future-Outlier marked this pull request as draft July 6, 2024 04:21

Future-Outlier mentioned this pull request Aug 1, 2024

[flytepropeller][compiler] Error Handling when Type is not found #5612

Merged

3 tasks

wild-endeavor and others added 6 commits August 6, 2024 09:44

proto

d26c228

Signed-off-by: Yee Hing Tong <[email protected]>

make generate

a5d515d

Signed-off-by: Yee Hing Tong <[email protected]> Signed-off-by: Future-Outlier <[email protected]>

new swagger

707a013

Signed-off-by: Future-Outlier <[email protected]>

update test

5d85d16

Signed-off-by: Future-Outlier <[email protected]>

add all json scalar things need to be implemented, need to add tests …

fa8bbd5

…and support json idl attribute access Signed-off-by: Future-Outlier <[email protected]>

support upstream STRUCT downstream JSON case

a503b8b

Signed-off-by: Future-Outlier <[email protected]>

Future-Outlier force-pushed the idl/json-type-bytes-and-attribute-access branch from 108350f to a503b8b Compare August 6, 2024 01:48

Future-Outlier added 7 commits August 6, 2024 09:55

make idl correct back

0b32845

Signed-off-by: Future-Outlier <[email protected]>

support attribute access

1dcfbc4

Signed-off-by: Future-Outlier <[email protected]>

fix attribute access for nested dict/dataclass

d5d7b79

Signed-off-by: Future-Outlier <[email protected]>

Create Execution Success, need to implement get Execution

c7c82ac

Signed-off-by: Future-Outlier <[email protected]>

remove print

00d6e2b

Signed-off-by: Future-Outlier <[email protected]>

Use scalarValue.Json.GetValue()

a005a2d

Signed-off-by: Future-Outlier <[email protected]>

fix flyteadmin tests

7ece699

Signed-off-by: Future-Outlier <[email protected]>

Future-Outlier changed the title ~~[WIP][flyteIDL][flytepropeller] Support Json IDL and Json attribute access in workflow~~ [WIP][flyteIDL][flytepropeller] Json IDL Aug 26, 2024

Future-Outlier commented Aug 26, 2024

View reviewed changes

Future-Outlier added 2 commits August 26, 2024 18:05

Merge branch 'master' into idl/json-type-bytes-and-attribute-access

8c0ab5c

regenerate proto

b3543e5

Signed-off-by: Future-Outlier <[email protected]>

Future-Outlier mentioned this pull request Aug 26, 2024

JSON IDL flyteorg/flytekit#2600

Closed

6 tasks

propeller error msg example

cb2876c

Signed-off-by: Future-Outlier <[email protected]>

Future-Outlier changed the title ~~[WIP][flyteIDL][flytepropeller] Json IDL~~ Json IDL Aug 27, 2024

Future-Outlier changed the title ~~Json IDL~~ JSON IDL Aug 27, 2024

Future-Outlier added 5 commits August 27, 2024 11:42

Merge branch 'master' into idl/json-type-bytes-and-attribute-access

aa68c42

Signed-off-by: Future-Outlier <[email protected]>

solve conlfict

ded1f7c

Signed-off-by: Future-Outlier <[email protected]>

solve lint and depedency error

62b7aa4

Signed-off-by: Future-Outlier <[email protected]>

make go-tidy

e44d309

Signed-off-by: Future-Outlier <[email protected]>

add tests for flyteidl/clients/go/coreutils/literals.go

b099d06

Signed-off-by: Future-Outlier <[email protected]>

lint

77ea3ed

Signed-off-by: Future-Outlier <[email protected]>

Future-Outlier marked this pull request as ready for review August 28, 2024 07:59

Future-Outlier self-assigned this Aug 28, 2024

Future-Outlier commented Aug 28, 2024

View reviewed changes

nit

e5af3a3

Signed-off-by: Future-Outlier <[email protected]>

pingsutw reviewed Aug 29, 2024

View reviewed changes

flyteidl/clients/go/coreutils/literals_test.go Show resolved Hide resolved

flytepropeller/pkg/controller/nodes/attr_path_resolver.go Outdated Show resolved Hide resolved

Future-Outlier added 3 commits August 29, 2024 13:52

split convertLiteral to convertStructToLiteral and convertJSONToLiteral

3f155d6

Signed-off-by: Future-Outlier <[email protected]>

Merge branch 'master' into idl/json-type-bytes-and-attribute-access

19bac6a

add comments for convertNumbers

bade905

Signed-off-by: Future-Outlier <[email protected]>

pingsutw reviewed Aug 30, 2024

View reviewed changes

Future-Outlier and others added 3 commits August 30, 2024 23:28

Update pingsu's advice

4e3c28f

Signed-off-by: Future-Outlier <[email protected]> Co-authored-by: Kevin Su <[email protected]>

Merge branch 'master' into idl/json-type-bytes-and-attribute-access

b523b9d

Signed-off-by: Future-Outlier <[email protected]>

update idl

2cf54c3

Signed-off-by: Future-Outlier <[email protected]>

eapolinario reviewed Sep 4, 2024

View reviewed changes

Future-Outlier added 10 commits September 5, 2024 22:50

update attribute resolver

1b0edc8

Signed-off-by: Future-Outlier <[email protected]>

remove func convertNumbers

fa8cca2

Signed-off-by: Future-Outlier <[email protected]>

specify msgpack as JSON format

a56d56a

Signed-off-by: Future-Outlier <[email protected]>

add tests for attr_path_resolver

5adafcc

Signed-off-by: Future-Outlier <[email protected]>

add coreutils for flytectl

cf95f40

Signed-off-by: Future-Outlier <[email protected]>

update the IDL

1631a94

Signed-off-by: Future-Outlier <[email protected]>

add serialization format in flytepropeller and flytectl

6f07cdd

Signed-off-by: Future-Outlier <[email protected]>

support SerializationFormat: msgpack in coreutils/literals.go MakeDef…

44da0cd

…aultLiteralForType Signed-off-by: Future-Outlier <[email protected]>

fix propeller's test

2a12ce2

Signed-off-by: Future-Outlier <[email protected]>

fix coreutils test

27dbc63

Signed-off-by: Future-Outlier <[email protected]>

wild-endeavor reviewed Sep 9, 2024

View reviewed changes

Future-Outlier closed this Sep 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON IDL #5542

JSON IDL #5542

Future-Outlier commented Jul 5, 2024 •

edited

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading

Future-Outlier Aug 5, 2024

Future-Outlier Aug 8, 2024

Future-Outlier Aug 28, 2024

pingsutw Aug 30, 2024

Future-Outlier Aug 30, 2024

pingsutw Aug 30, 2024

Future-Outlier commented Aug 30, 2024

eapolinario left a comment

wild-endeavor Sep 9, 2024

Future-Outlier Sep 9, 2024

wild-endeavor Sep 9, 2024

Future-Outlier Sep 9, 2024

Future-Outlier commented Sep 14, 2024

		func TestResolveAttrPathInJson(t *testing.T) {
		// Helper function to convert a map to JSON and then to msgpack

	// convertJSONToLiteral converts the protobuf struct (e.g. dataclass) to literal
	// convertJSONToLiteral converts the JSON (e.g. dataclass) to literal

JSON IDL #5542

JSON IDL #5542

Conversation

Future-Outlier commented Jul 5, 2024 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

flyteidl

flytepropeller

flytectl

How was this patch tested?

Setup process

Screenshots

[flytepropeller] attribute access

[flytepropeller] backward compatible

[flytepropeller] others

[flytectl] create execution with inputs

[flytectl] get task

Check all the applicable boxes

Related PRs

Docs link

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Future-Outlier commented Aug 30, 2024

eapolinario left a 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

Choose a reason for hiding this comment

Future-Outlier commented Sep 14, 2024

Future-Outlier commented Jul 5, 2024 •

edited

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading