-
Notifications
You must be signed in to change notification settings - Fork 680
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
[FlyteCopilot] Binary IDL Attribute Access Primitive Input #5850
Conversation
Signed-off-by: Future-Outlier <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5850 +/- ##
==========================================
+ Coverage 36.33% 36.36% +0.02%
==========================================
Files 1304 1304
Lines 110137 110154 +17
==========================================
+ Hits 40023 40054 +31
+ Misses 65946 65929 -17
- Partials 4168 4171 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Future-Outlier <[email protected]>
// TODO: Try to support Primitive_Datetime, Primitive_Duration, Flyte File, and Flyte Directory. | ||
return fmt.Sprintf("%v", currVal), nil | ||
} | ||
return "", fmt.Errorf("unsupported binary tag [%v]", o.Binary.Tag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR, datetime, timedelta, flyte directory, and flyte file are not supported, and these are not supported before, so let's ignore them now.
The hard part of this is that we have only scalar
value, but not expected literal type or expected python type as our hint, that's why I can't support these in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty cool.
Can you make sure to create a gh issue to track the TODO?
Yes, will do it tmr |
Tracking issue
#5318
Why are the changes needed?
We need to support attribute access for
dataclass
when it is a msgpack binary IDL object to a raw container task.Example:
What does this PR change?
It adds logic to deserialize the msgpack binary IDL object and convert it into a string for the container task input, similar to how we handle primitive types.
Related Code: template.go#L175-L192
How was this tested?
Example:
Setup for remote execution:
cd docker/sandbox-bundled make build flytectl demo start --image flyte-sandbox:latest --force
Note: See related PR for image
futureoutlier/rawcontainer:0320
: flyteorg/flytekit#2258Checklist
Screenshots
Check all the applicable boxes