-
Notifications
You must be signed in to change notification settings - Fork 671
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
[flytepropeller] Support attribute access on promise #4232
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4232 +/- ##
==========================================
+ Coverage 59.06% 60.02% +0.96%
==========================================
Files 621 571 -50
Lines 53105 41337 -11768
==========================================
- Hits 31365 24812 -6553
+ Misses 19240 14123 -5117
+ Partials 2500 2402 -98
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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.
A few nit comments and failing checks on lint and generate. Otherwise this looks great! This is going to be so powerful.
Signed-off-by: byhsu <[email protected]>
Signed-off-by: byhsu <[email protected]>
Signed-off-by: byhsu <[email protected]>
Signed-off-by: byhsu <[email protected]>
fa23167
to
1b5fe18
Compare
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.
Looks great! Will wait for one more approve before merging to try and stick with old idl requirements.
|
||
switch obj := obj.(type) { | ||
case map[string]interface{}: | ||
newSt, err := structpb.NewStruct(obj) |
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.
just one question. if you just have a typing.Dict[str, DataClass], and you bind to one of the dataclasses. you end up in this condition right?
What is this line for? newSt, err := structpb.NewStruct(obj)
In fact in this case - line 22 above:
tmpVal, exist = currVal.GetMap().GetLiterals()[attr.GetStringValue()]
won't tmpVal already be a Literal? can't you just return it? it won't get returned right now, it'll fall into the condition on line 41.
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 guess you are talking about the case that where bindAttrPath[count:]
is 0 and the currVal
on line41 is a datatclass. Currently we convert convert struct to map, and convert it back to struct without doing other things because line 63 the loop will be skipped.
adding a check about len(bindAttrPath[count:]) == 0
on line 41 can circumvent the unnecessary work, but I think it's ok to keep it as-is because it should not be a large overhead.
Tracking issue
#3864
Describe your changes
See flyteorg/flyteidl#439 (comment)
Check all the applicable boxes
Screenshots
Note to reviewers