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 aisk commented Nov 24, 2019

Co-authored-by: naught101 .

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

https://bugs.python.org/issue27873

@bedevere-bot bedevere-bot added the docs Documentation in the Doc dir label Nov 24, 2019
@the-knights-who-say-ni
Copy link

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
Copy link
Contributor Author

aisk commented Nov 24, 2019

@aeros

@corona10
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?

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

@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
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
Copy link
Member

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
Copy link
Contributor Author

aisk commented Nov 24, 2019

😃 Thank you again!

Copy link
Member

@aeros aeros left a comment

Choose a reason for hiding this comment

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

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)

@aisk
Copy link
Contributor Author

aisk commented Nov 26, 2019

Updated, and the CLA check is passed.

Copy link
Member

@aeros aeros left a comment

Choose a reason for hiding this comment

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

Thanks for making the recommended changes @aisk, LGTM.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm

@corona10
Copy link
Member

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
Copy link
Contributor

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
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
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
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
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
Copy link
Contributor Author

aisk commented Nov 28, 2019

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

miss-islington pushed 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 pushed 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 pushed 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 pushed 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
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
)

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

Prev PR: python#17367  @aeros


https://bugs.python.org/issue27873
@aisk aisk deleted the transfer-issue-27873 branch March 6, 2022 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants