[RFC] Binary IDL With MessagePack Bytes #5742

Future-Outlier · 2024-09-12T09:39:23Z

Tracking issue

#5318

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

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-09-12T09:46:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.30%. Comparing base (e67aae0) to head (bf2a8af).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5742      +/-   ##
==========================================
+ Coverage   36.21%   36.30%   +0.08%     
==========================================
  Files        1303     1305       +2     
  Lines      109644   109991     +347     
==========================================
+ Hits        39710    39927     +217     
- Misses      65810    65909      +99     
- Partials     4124     4155      +31

Flag	Coverage Δ
unittests-datacatalog	`51.37% <ø> (ø)`
unittests-flyteadmin	`55.62% <ø> (+0.01%)`	⬆️
unittests-flytecopilot	`12.17% <ø> (ø)`
unittests-flytectl	`62.26% <ø> (+0.04%)`	⬆️
unittests-flyteidl	`7.12% <ø> (ø)`
unittests-flyteplugins	`53.35% <ø> (ø)`
unittests-flytepropeller	`41.87% <ø> (+0.12%)`	⬆️
unittests-flytestdlib	`55.21% <ø> (ø)`

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.

wild-endeavor · 2024-09-16T00:15:13Z

rfc/system/5741-binary-idl-with-message-pack.md

+|-----------------------------------|----------------------------------------------|
+| Protobuf Struct -> JSON String -> Python Val | Binary (value: MessagePack Bytes, tag: msgpack) IDL Object -> Bytes -> (Dict ->) -> Python Val |
+
+Note: if a python value can't directly be converted to `MessagePack Bytes`, we can convert it to `Dict`, and then convert it to `MessagePack Bytes`.


Add a note here to say very clearly that there is no JSON in the new type at all. JSON plays zero part of the new spec (except for the schema).

eapolinario

I left a few comments. Overall, I like the direction.

rfc/system/5741-binary-idl-with-message-pack.md

eapolinario · 2024-09-16T19:47:52Z

rfc/system/5741-binary-idl-with-message-pack.md

+        msgpack_bytes = lv.scalar.json.value
+    else:
+        raise ValueError(f"{tag} is not supported to decode this Binary Literal: {lv.scalar.binary}.")
+    return msgpack.loads(msgpack_bytes)


Can you add the full signature of to_python_value? Specifically, we can use typing.cast(expected_python_type, msgpack.loads(msgpack_bytes)) to get type-checkers to agree with this, right?

We don't need this, because we will use from mashumaro.codecs.msgpack import MessagePackDecoder, MessagePackEncoder to encode and decode.
It will make sure we convert it back to a type we 100% want.

msgpack.dumps will only be used when dealing with untyped dict.

rfc/system/5741-binary-idl-with-message-pack.md

eapolinario · 2024-09-17T00:41:32Z

rfc/system/5741-binary-idl-with-message-pack.md

+    }
+}
+// Use Message Pack as Default Tag for deserialization.
+func MakeBinaryLiteral(v []byte) *core.Literal {


This is not called anywhere yet (besides a few tests). We can make the tag part of the signature (and assume a default value).

rfc/system/5741-binary-idl-with-message-pack.md

eapolinario · 2024-09-17T00:51:40Z

rfc/system/5741-binary-idl-with-message-pack.md

+MsgPack is a good choice because it's more smaller and faster than UTF-8 Encoded JSON String.
+
+You can see the performance comparison here: https://github.com/flyteorg/flyte/pull/5607#issuecomment-2333174325
+We will use `msgpack` to do it.


This comparison should move to the Alternatives section. The conclusion should be about how this design ticks the two problems we set out to solve (1. a better representation for json objects in Flyte, and 2. Fix Attribute Access once and for all).

wild-endeavor · 2024-09-18T00:09:00Z

rfc/system/5741-binary-idl-with-message-pack.md

+
+1. No JSON Schema provided:
+
+Input is expected as an `Object` (e.g., `{"a": 1}`).


This is a Javascript Object right? Let's add that.

Can you also add a section about pasting? How do users paste in something? Can add to unresolved questions if you want. If this is a Javascript object, we should allow json pasting, but what about yaml? What about msgpack bytes if they were copied from a binary file?

wild-endeavor · 2024-09-18T00:19:09Z

rfc/system/5741-binary-idl-with-message-pack.md

+
+Input is expected as an `Object` (e.g., `{"a": 1}`).
+
+2. JSON Schema provided:


In both cases, with or without JSON schema... what happens after the user enters data? Can you add a short description of what happens? I assume it's some JS msgpack library that will turn the object into bytes.

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

eapolinario · 2024-09-18T14:31:58Z

Thank you for all your work on this, @Future-Outlier ! This feature is going to solve a massive a pain in the Flyte ecosystem.

fg91 · 2024-09-26T19:12:18Z

rfc/system/5741-binary-idl-with-message-pack.md

+            return true
+        }
+
+        return isSameTypeInJSON(upstreamMetadata, downstreamMetadata) ||\


Thank you @Future-Outlier for identifying a solution to this problem 🙇

Future-Outlier changed the title ~~[wip] Binary IDL With MessagePack Bytes~~ [wip] [RFC] Binary IDL With MessagePack Bytes Sep 12, 2024

Future-Outlier changed the title ~~[wip] [RFC] Binary IDL With MessagePack Bytes~~ [RFC] Binary IDL With MessagePack Bytes Sep 12, 2024

Future-Outlier marked this pull request as ready for review September 12, 2024 12:56

This was referenced Sep 14, 2024

JSON IDL flyteorg/flytekit#2600

Closed

[RFC] JSON IDL #5607

Closed

JSON IDL #5542

Closed

wild-endeavor reviewed Sep 16, 2024

View reviewed changes

eapolinario reviewed Sep 17, 2024

View reviewed changes

wild-endeavor reviewed Sep 18, 2024

View reviewed changes

wild-endeavor previously approved these changes Sep 18, 2024

View reviewed changes

eapolinario previously approved these changes Sep 18, 2024

View reviewed changes

Future-Outlier dismissed stale reviews from eapolinario and wild-endeavor via ff8c99a September 18, 2024 07:01

Future-Outlier requested review from eapolinario and wild-endeavor September 18, 2024 07:13

wild-endeavor approved these changes Sep 18, 2024

View reviewed changes

Future-Outlier enabled auto-merge (squash) September 18, 2024 07:19

Future-Outlier disabled auto-merge September 18, 2024 07:58

Future-Outlier enabled auto-merge (squash) September 18, 2024 07:58

Future-Outlier added 10 commits September 18, 2024 18:59

Binary IDL With MessagePack Bytes

f60854a

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

Finish part 1 and part 2

d97bb2a

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

Finish the flyteidl part

96a948b

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

Start from flyteclt

ae16b44

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

TBD: flyteconsole

fd4aa00

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

TBD: flyteconsole

933bc38

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

RFC Done

7809338

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

nit

90bdcff

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

Update Yee's advice

16b741e

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

Update msgpack golang library

b93569a

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

Future-Outlier added 16 commits September 18, 2024 18:59

Update msgpack golang library

343402c

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

better url reference

affc182

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

add conditional problem in msgpack

8c6b57c

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

update yee's advice

eda65ef

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

use SimpleType.STRUCT for literal type

8a822ff

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

update yee's advice

f33ef39

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

update flyteidl part with permenant link

5c8604f

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

update flytectl part by Yee's advice

8f01c8a

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

update comments for the flytekit part

6de3b3d

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

use msgpack as default tag in func MakeBinaryLiteral

434855a

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

Add FlyteCopilot section

0e8ed03

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

update all Eduardo's advice

71744f3

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

update Yee's advice

3d122dd

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

FlyteConsole update

123232e

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

update Yee's advice

b6dfb33

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

update Yee's advice

3716255

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

Future-Outlier force-pushed the MessagePack-IDL branch from bf2a8af to 3716255 Compare September 18, 2024 10:59

Future-Outlier self-assigned this Sep 18, 2024

Future-Outlier added the rfc A label for RFC issues label Sep 18, 2024

Future-Outlier merged commit 312910d into master Sep 18, 2024
47 of 48 checks passed

Future-Outlier deleted the MessagePack-IDL branch September 18, 2024 11:15

fg91 reviewed Sep 26, 2024

View reviewed changes

flyteorg deleted a comment from Future-Outlier Oct 7, 2024

fg91 mentioned this pull request Oct 23, 2024

[Core feature] Introduce a zero_value field in LiteralType to capture DataClasses default fields #5887

Open

2 tasks

This was referenced Nov 7, 2024

Allowing dataclasses which require list protobuf deserialization flyteorg/flytekit#2614

Closed

[Core feature] Enhanced promise attribute support for Dataclasses in Python Flytekit #5667

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Binary IDL With MessagePack Bytes #5742

[RFC] Binary IDL With MessagePack Bytes #5742

Future-Outlier commented Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

wild-endeavor Sep 16, 2024 •

edited

Loading

Future-Outlier Sep 16, 2024

eapolinario left a comment

eapolinario Sep 16, 2024

Future-Outlier Sep 17, 2024

eapolinario Sep 17, 2024

eapolinario Sep 17, 2024

wild-endeavor Sep 18, 2024

wild-endeavor Sep 18, 2024

wild-endeavor Sep 18, 2024

eapolinario commented Sep 18, 2024

fg91 Sep 26, 2024


		1. No JSON Schema provided:

		Input is expected as an `Object` (e.g., `{"a": 1}`).


		Input is expected as an `Object` (e.g., `{"a": 1}`).

		2. JSON Schema provided:

[RFC] Binary IDL With MessagePack Bytes #5742

[RFC] Binary IDL With MessagePack Bytes #5742

Conversation

Future-Outlier commented Sep 12, 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

Check all the applicable boxes

Related PRs

Docs link

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

wild-endeavor Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eapolinario commented Sep 18, 2024

Choose a reason for hiding this comment

Future-Outlier commented Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

wild-endeavor Sep 16, 2024 •

edited

Loading