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

bpo-27873: Update docstring for multiprocessing.Pool.map #17367

Closed
wants to merge 1 commit into from

Conversation

@aisk
Copy link
Contributor

aisk commented Nov 24, 2019

Co-authored-by: naught101 .

This PR is transfered from https://bugs.python.org/issue27873

https://bugs.python.org/issue27873

@the-knights-who-say-ni

This comment has been minimized.

Copy link

the-knights-who-say-ni commented Nov 24, 2019

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@aisk

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@aisk

This comment has been minimized.

Copy link
Contributor Author

aisk commented Nov 24, 2019

@corona10

This comment has been minimized.

Copy link
Member

corona10 commented Nov 24, 2019

@aisk

Thanks for the contribution to CPython project :)
Glad to meet you again on GitHub.
Would you like to sign up for the CLA, please?

@corona10 corona10 added the skip news label Nov 24, 2019
@corona10 corona10 requested a review from berkerpeksag Nov 24, 2019
Copy link
Member

corona10 left a comment

@berkerpeksag
Dear core developers,

@aisk is the first time contributor to this project.
This PR looks good to me except the CLA is not done yet.

Please take a look @aisk PR kindly.

Thank you for understanding :)

@aisk

This comment has been minimized.

Copy link
Contributor Author

aisk commented Nov 24, 2019

@corona10 Hi thanks for your welcome! Happy to see you again!

The CLA is signed, and I think it's taking time to complete? The bot said it's taking a work day usually.

@corona10

This comment has been minimized.

Copy link
Member

corona10 commented Nov 24, 2019

The CLA is signed, and I think it's taking time to complete? The bot said it's taking a workday usually.

Yes, it is expected to take some time. Let's wait until then. :)
I request the final review to the core developer.

If you need my help please ping me any time

Thanks for your contribution!

@aisk

This comment has been minimized.

Copy link
Contributor Author

aisk commented Nov 24, 2019

😃 Thank you again!

Copy link
Member

aeros left a comment

Thanks for the PR @aisk and welcome!

I approve the general goal of the PR, but I would recommend a slight change in wording:

      A parallel equivalent of the built-in function :func:`map`. It blocks
      until the result is ready.

      Unlike :func:`map`, this method supports only one *iterable*
      argument. For functions that require multiple arguments, see
      :meth:`starmap`.

(Note: adjusted the word wrapping because it exceeded 80 chars per line with the changes I recommended)

Co-authored-by: naught101 .
@aisk aisk force-pushed the aisk:transfer-issue-27873 branch from b157e8f to ca2630d Nov 26, 2019
@aisk

This comment has been minimized.

Copy link
Contributor Author

aisk commented Nov 26, 2019

Updated, and the CLA check is passed.

@aeros
aeros approved these changes Nov 26, 2019
Copy link
Member

aeros left a comment

Thanks for making the recommended changes @aisk, LGTM.

Copy link
Member

corona10 left a comment

lgtm

@corona10 corona10 requested a review from rhettinger Nov 26, 2019
@corona10

This comment has been minimized.

Copy link
Member

corona10 commented Nov 26, 2019

Dear @rhettinger

This PR is the @aisk 's first time contributing to this project.
Can you please take a look at this PR as the core developer?
Triagers reviewed it already, only left is the core developer's decision. :)
Thank you for your understanding.

@rhettinger

This comment has been minimized.

Copy link
Contributor

rhettinger commented Nov 26, 2019

Sorry, I'm going to recommend rejecting this edit as being unnecesary. The argument signature already shows that only one iterable is accepted.

AFAICT, no one has ever been confused by the current docs. There has been interest in changing the signature to accommodate multiple inputs, but that is a different matter than whether the current docs are accurate.

@aisk

This comment has been minimized.

Copy link
Contributor Author

aisk commented Nov 27, 2019

@rhettinger Hi I'm fine with this, and have another question. If we change the signature to allow it accept multiple iterable as input, how to compat with old codes? Some developer could call this with chunksize without keyword argument like pool.map(func, iterable, 10).

Can we just check the chunksize's type and make it compatible? or mark it as a breaking change?

@aeros

This comment has been minimized.

Copy link
Member

aeros commented Nov 27, 2019

@rhettinger

Sorry, I'm going to recommend rejecting this edit as being unnecesary. The argument signature already shows that only one iterable is accepted.

While there's already mention of pool.map() only accepting one iterable, I think it's helpful to briefly mention and link to pool.starmap() as an alternative for multiple iterables. @berkerpeksag suggested mentioning this in the bpo issue, which seems to be the primary purpose of the PR:

I think we can add a note for starmap() in the following sentence:

[...] (it supports only one iterable argument though).

Perhaps the PR could be edited to leave the existing "supports only one iterable" part intact, and modify it only slightly to mention pool.starmap() (which is more along the lines of @berkerpeksag's suggestion):

      A parallel equivalent of the :func:`map` built-in function (it supports only
      one *iterable* argument though, for multiple iterables see :meth:`starmap`).
      It blocks until the result is ready.

Also, it might be helpful to obtain additional input on this from @pitrou, since he's an active expert/maintainer for the multiprocessing module.

@rhettinger rhettinger closed this Nov 27, 2019
@pitrou

This comment has been minimized.

Copy link
Member

pitrou commented Nov 27, 2019

I'd be in favour of @aeros' proposed change. Feel free to open a new PR :-)

@aeros

This comment has been minimized.

Copy link
Member

aeros commented Nov 27, 2019

@pitrou

I'd be in favour of @aeros' proposed change. Feel free to open a new PR :-)

Awesome, sounds good. (:

@aisk Let me know if you'd like to open a PR with the above recommended changes (or simply open it and @ mention me).

@aisk

This comment has been minimized.

Copy link
Contributor Author

aisk commented Nov 28, 2019

@aeros ok, I'll try it later this day, thanks!

miss-islington added a commit to miss-islington/cpython that referenced this pull request Dec 3, 2019
)

Update docstring for `multiprocessing.Pool.map` to mention `pool.starmap()`.

Prev PR: python#17367  @aeros

https://bugs.python.org/issue27873
(cherry picked from commit eb48a45)

Co-authored-by: An Long <aisk@users.noreply.github.com>
miss-islington added a commit that referenced this pull request Dec 3, 2019
Update docstring for `multiprocessing.Pool.map` to mention `pool.starmap()`.

Prev PR: #17367  @aeros


https://bugs.python.org/issue27873
miss-islington added a commit to miss-islington/cpython that referenced this pull request Dec 3, 2019
)

Update docstring for `multiprocessing.Pool.map` to mention `pool.starmap()`.

Prev PR: python#17367  @aeros

https://bugs.python.org/issue27873
(cherry picked from commit eb48a45)

Co-authored-by: An Long <aisk@users.noreply.github.com>
miss-islington added a commit that referenced this pull request Dec 3, 2019
Update docstring for `multiprocessing.Pool.map` to mention `pool.starmap()`.

Prev PR: #17367  @aeros

https://bugs.python.org/issue27873
(cherry picked from commit eb48a45)

Co-authored-by: An Long <aisk@users.noreply.github.com>
miss-islington added a commit that referenced this pull request Dec 3, 2019
Update docstring for `multiprocessing.Pool.map` to mention `pool.starmap()`.

Prev PR: #17367  @aeros

https://bugs.python.org/issue27873
(cherry picked from commit eb48a45)

Co-authored-by: An Long <aisk@users.noreply.github.com>
jacobneiltaylor added a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
)

Update docstring for `multiprocessing.Pool.map` to mention `pool.starmap()`.

Prev PR: python#17367  @aeros


https://bugs.python.org/issue27873
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.