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.

Sign up for GitHub

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

Jump to bottom

fix: Support multiple function arguments in couler.map() #215

Merged
merged 1 commit into from Jun 28, 2021
Merged

fix: Support multiple function arguments in couler.map() #215

merged 1 commit into from Jun 28, 2021

Conversation

nooraangelva
Copy link
Contributor

@nooraangelva nooraangelva commented Jun 15, 2021 •
edited

What changes were proposed in this pull request?

Added changes to accept multiple function arguments in couler.map()
#169 continuing the fix adding the test function for the modifications.

Had to start again from scratch.
Reason:
The previous idea just pretty much loops the arguments in the map().

return map(map(function, input_list), *other)

But the return value is a Step Class not a function so the first map() will succeed but not the second one or the third... because the map() checks the function first.

inner_step = Step(name=inner_dict["id"], template=template_name)

return inner_step
#32 issue

Why are the changes needed?

#32 issue

Does this PR introduce any user-facing change?

No.

How was this patch tested?

A test was created for it. similar to the one-argument test.
Test

@nooraangelva nooraangelva changed the title fix: Support multiple function arguments in couler.map() [WIP] fix: Support multiple function arguments in couler.map() Jun 17, 2021
@nooraangelva nooraangelva force-pushed the master-map branch 2 times, most recently from 3c35100 to afb1149 Compare June 18, 2021 12:30
terrytangyuan
terrytangyuan reviewed Jun 18, 2021
View reviewed changes
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

I am converting this to draft since there seems to be many debug statements and unnecessary code. Please ping me when ready for review.

@terrytangyuan terrytangyuan changed the title fix: Support multiple function arguments in couler.map() [WIP] fix: Support multiple function arguments in couler.map() Jun 18, 2021
@nooraangelva
Copy link
Contributor Author

I am converting this to draft since there seems to be many debug statements and unnecessary code. Please ping me when ready for review.

Hi, Thanks for the feedback. @terrytangyuan May I ask what do you mean by debugging statements? Do you mean the comments in the code? Other than those there shouldn't be any debugging.

Thanks in advance for your answer.

  • Noora

terrytangyuan
terrytangyuan reviewed Jun 22, 2021
View reviewed changes
couler/tests/map_test.py Outdated Show resolved Hide resolved
couler/tests/map_test.py Outdated Show resolved Hide resolved
@nooraangelva
Copy link
Contributor Author

Hi @terrytangyuan ,
The fixes you proposed are now implemented.

  • Noora

@nooraangelva nooraangelva changed the title [WIP] fix: Support multiple function arguments in couler.map() fix: Support multiple function arguments in couler.map() Jun 23, 2021
terrytangyuan
terrytangyuan reviewed Jun 23, 2021
View reviewed changes
couler/core/syntax/loop.py Outdated Show resolved Hide resolved
couler/core/syntax/loop.py Outdated Show resolved Hide resolved
couler/core/syntax/loop.py Outdated Show resolved Hide resolved
couler/core/syntax/loop.py Outdated Show resolved Hide resolved
couler/tests/map_test.py Outdated Show resolved Hide resolved
couler/tests/map_test.py Outdated Show resolved Hide resolved
@nooraangelva nooraangelva changed the title fix: Support multiple function arguments in couler.map() [WIP]fix: Support multiple function arguments in couler.map() Jun 24, 2021
@nooraangelva
Copy link
Contributor Author

I changed it to WIP.
I want to go through the code ones more before saying it's ready to be merged.

  • Noora

@terrytangyuan
Copy link
Member

#212 (comment)

@nooraangelva nooraangelva force-pushed the master-map branch 2 times, most recently from e5c9c1b to eea73eb Compare June 24, 2021 08:10
@nooraangelva nooraangelva changed the title [WIP]fix: Support multiple function arguments in couler.map() fix: Support multiple function arguments in couler.map() Jun 24, 2021
moshewe
moshewe reviewed Jun 26, 2021
View reviewed changes
Copy link
Contributor

@moshewe moshewe left a comment

Choose a reason for hiding this comment

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

Right now the code works only with with_items, but in a PR I will submit soon we'll be able to use with_param and parallelism, and I can see it's going to be easy to make map work with both additions. Nicely done!

couler/core/syntax/loop.py Outdated Show resolved Hide resolved
couler/core/syntax/loop.py Outdated Show resolved Hide resolved
@nooraangelva
Copy link
Contributor Author

Right now the code works only with with_items, but in a PR I will submit soon we'll be able to use with_param and parallelism, and I can see it's going to be easy to make map work with both additions. Nicely done!

Hi @moshewe!
Thanks for advising me. This is only my second PR to a project that is not mine. So all help and advice are greatly appreciated.

  • Noora

@nooraangelva
Clean up part 3
79b98ee 
Improved comments and naming in loop.py

Clean up part 2.

Testing

Clean up.

Update map_test.py

deleted debuging.

Delete integration_tests.Unix.sh

sanity check

Map() fix and test

map() that accepts several arguments (*arg) whit test for it.)

map() arguments

modifications and a test function in the works.

Create integration_tests.Unix.sh
@terrytangyuan terrytangyuan merged commit 4b3a2b0 into couler-proj:master Jun 28, 2021
@nooraangelva nooraangelva deleted the master-map branch June 28, 2021 10:17
@moshewe moshewe mentioned this pull request Jun 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrytangyuan terrytangyuan terrytangyuan approved these changes

@moshewe moshewe moshewe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nooraangelva @terrytangyuan @moshewe