-
Notifications
You must be signed in to change notification settings - Fork 310
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
Read offloaded literals #2685
Read offloaded literals #2685
Conversation
Signed-off-by: Eduardo Apolinario <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2685 +/- ##
==========================================
- Coverage 66.44% 58.40% -8.05%
==========================================
Files 9 302 +293
Lines 453 24748 +24295
Branches 0 2867 +2867
==========================================
+ Hits 301 14453 +14152
- Misses 152 9777 +9625
- Partials 0 518 +518 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
@@ -852,6 +852,52 @@ def from_flyte_idl(cls, pb2_object): | |||
) | |||
|
|||
|
|||
class LiteralOffloadedMetadata(_common.FlyteIdlEntity): |
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.
Not your change, but a generator class for this could be useful
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.
do you mean like a utility function to help in tests?
Signed-off-by: Eduardo Apolinario <[email protected]>
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.
Minor nit on test, otherwise LGTM
) | ||
|
||
# Write offloaded_lv as bytes to a temp file | ||
with open(f"{tmp_path}/offloaded_proto.pb", "wb") as f: |
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.
I'm surprised using /
works with windows. I usually go with:
with open(f"{tmp_path}/offloaded_proto.pb", "wb") as f: | |
with (tmp_path / "offloaded_proto.pb").open("wb") as f: |
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.
that's a good call. I'm going to fix this in a separate PR.
As for what happens on windows, my guess is that the /
becomes part of the file name.
Tracking issue
Linking to the RFC: flyteorg/flyte#5103
Why are the changes needed?
As discussed in flyteorg/flyte#5103, flytekit needs to be able to read offloaded literals.
What changes were proposed in this pull request?
The type engine "unrolls" literals that contain offloaded literals prior to turning those into python values.
How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link