Skip to content
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

[Flyte][3][flytepropeller][Attribute Access][flytectl] Binary IDL With MessagePack #5763

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Future-Outlier
Copy link
Member

@Future-Outlier Future-Outlier commented Sep 20, 2024

Tracking Issue

Issue #5318

Why are these changes needed?

  1. Support attribute access with 100% type correctness.
  2. Add support for flytectl to create and get executions.

Note: This PR includes handling of primitive types, Flyte types, and dataclasses.

What changes are proposed in this pull request?

Attribute Access

  1. Use MsgPack to deserialize msgpack bytes in Binary Literal.
  2. Retrieve attributes from the deserialized object.
  3. Serialize the retrieved attributes back to Binary Literal.

flytectl

  • When receiving non-string input in flytectl, serialize it to msgpack bytes and create a binary scalar directly.

This version is more concise, avoids redundant words, and clarifies the structure a bit.

How was this patch tested?

Attribute Access

unit tests and remote execution

flytectl

command line

  • create execution
flytectl create execution --execFile build/PR/JSON/stacked_PRs/flytectl/create_task.yaml -p flytesnacks -d development
image image
iamRoleARN: ""
inputs:
    dc:
        a: 1
        b: 3.14
        c: example_string
envs: {}
kubeServiceAcct: ""
targetDomain: ""
targetProject: ""
task: build.PR.JSON.stacked_PRs.flytectl.dataclass_simple.t_simple
version: kNjxOflWVudmuIRR4tLHeQ
  • get execution
flytectl get task -d development -p flytesnacks build.PR.JSON.stacked_PRs.flytectl.dataclass_simple.t_simple --execFile build/PR/JSON/stacked_PRs/flytectl/get_task.yaml --version kNjxOflWVudmuIRR4tLHeQ
image

get_task.yaml

iamRoleARN: ""
inputs:
    dc: {}
envs: {}
kubeServiceAcct: ""
targetDomain: ""
targetProject: ""
task: build.PR.JSON.stacked_PRs.flytectl.dataclass_simple.t_simple
version: kNjxOflWVudmuIRR4tLHeQ

Setup process

single binary

Screenshots

  • Unit Tests
image
  • Local Execution
    image

  • Remote Execution
    image

Check all the applicable boxes

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

Signed-off-by: Future-Outlier <eric901201@gmail.com>
Copy link

codecov bot commented Sep 20, 2024

Codecov Report

Attention: Patch coverage is 85.33333% with 11 lines in your changes missing coverage. Please review.

Project coverage is 36.35%. Comparing base (28f65b3) to head (f68f75c).
Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
...opeller/pkg/controller/nodes/attr_path_resolver.go 80.70% 9 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5763      +/-   ##
==========================================
+ Coverage   36.30%   36.35%   +0.05%     
==========================================
  Files        1305     1304       -1     
  Lines      110000   110112     +112     
==========================================
+ Hits        39932    40032     +100     
- Misses      65912    65921       +9     
- Partials     4156     4159       +3     
Flag Coverage Δ
unittests-datacatalog 51.37% <ø> (ø)
unittests-flyteadmin 55.61% <ø> (-0.01%) ⬇️
unittests-flytecopilot 12.17% <ø> (ø)
unittests-flytectl 62.21% <ø> (ø)
unittests-flyteidl 7.16% <100.00%> (+0.04%) ⬆️
unittests-flyteplugins 53.35% <ø> (ø)
unittests-flytepropeller 42.03% <81.03%> (+0.13%) ⬆️
unittests-flytestdlib 55.37% <ø> (+0.15%) ⬆️

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: Future-Outlier <eric901201@gmail.com>
Future-Outlier and others added 3 commits September 25, 2024 09:38
Signed-off-by: Future-Outlier <eric901201@gmail.com>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Future-Outlier <eric901201@gmail.com>
@Future-Outlier Future-Outlier changed the title [Flyte][3][Attribute Access] Binary IDL With MessagePack [Flyte][3][flytepropeller][Attribute Access][flytectl] Binary IDL With MessagePack Sep 26, 2024
Comment on lines -127 to -133
t.Run("Generic", func(t *testing.T) {
literalVal := map[string]interface{}{
"x": 1,
"y": "ystringvalue",
}
var literalType = &core.LiteralType{Type: &core.LiteralType_Simple{Simple: core.SimpleType_STRUCT}}
lit, err := MakeLiteralForType(literalType, literalVal)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: Future-Outlier <eric901201@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants