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

Fix map_task sensitive to argument order #1888

Merged

Conversation

chaohengstudent
Copy link
Contributor

@chaohengstudent chaohengstudent commented Oct 12, 2023

TL;DR

fixing bug about map_task collecting value length with first key which may be the bound input causing error.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Iterate the key and get the one which is not bound.
Get the length of that key's value for mapping.

Tracking Issue

Fixes flyteorg/flyte#4161

Follow-up issue

NA

Copy link
Collaborator

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

Can you add a few tests?

@chaohengstudent
Copy link
Contributor Author

chaohengstudent commented Oct 12, 2023

Can you add a few tests?

sure. will update later.

Signed-off-by: Chao-Heng Lee <[email protected]>
@chaohengstudent
Copy link
Contributor Author

I have added a test about testing task parameter order.
If there are any suggestion about it. Please let me know, thanks!

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (d89cb21) 55.06% compared to head (c5c1926) 55.21%.
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1888      +/-   ##
==========================================
+ Coverage   55.06%   55.21%   +0.14%     
==========================================
  Files         296      300       +4     
  Lines       22250    22393     +143     
  Branches     3357     3365       +8     
==========================================
+ Hits        12252    12364     +112     
- Misses       9835     9867      +32     
+ Partials      163      162       -1     
Files Coverage Δ
flytekit/core/array_node_map_task.py 34.88% <0.00%> (-1.65%) ⬇️
flytekit/core/map_task.py 31.70% <0.00%> (-1.21%) ⬇️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

Can you also make this change to array node map tasks?

Signed-off-by: Chao-Heng Lee <[email protected]>
@chaohengstudent
Copy link
Contributor Author

Can you also make this change to array node map tasks?

have updated it! thanks for review.

@eapolinario eapolinario merged commit b651833 into flyteorg:master Oct 17, 2023
69 of 70 checks passed
@eapolinario
Copy link
Collaborator

thank you, @chaohengstudent !

Future-Outlier pushed a commit to Future-Outlier/flytekit that referenced this pull request Oct 20, 2023
* Fix _raw_execute for getting correct len of input value.

Signed-off-by: Chao-Heng Lee <[email protected]>

* Add test.

Signed-off-by: Chao-Heng Lee <[email protected]>

* also update with array_node_map_task.

Signed-off-by: Chao-Heng Lee <[email protected]>

* rename test.

Signed-off-by: Chao-Heng Lee <[email protected]>

---------

Signed-off-by: Chao-Heng Lee <[email protected]>
ringohoffman pushed a commit to ringohoffman/flytekit that referenced this pull request Nov 24, 2023
* Fix _raw_execute for getting correct len of input value.

Signed-off-by: Chao-Heng Lee <[email protected]>

* Add test.

Signed-off-by: Chao-Heng Lee <[email protected]>

* also update with array_node_map_task.

Signed-off-by: Chao-Heng Lee <[email protected]>

* rename test.

Signed-off-by: Chao-Heng Lee <[email protected]>

---------

Signed-off-by: Chao-Heng Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] map_task sensitive to argument order
4 participants