bpo-32321: Add pure Python fallback for functools.reduce by madman-bob · Pull Request #8548 · python/cpython

madman-bob · 2018-07-29T12:37:40Z

Add a Python fallback for functools.reduce, and enable the appropriate tests to run on py_functools.

https://bugs.python.org/issue32321

the-knights-who-say-ni · 2018-07-29T12:37:43Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

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

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

serhiy-storchaka

Please test both implementations.

serhiy-storchaka · 2018-07-29T13:54:52Z

This test tests only C implementation of reduce() if it is available. Please make both implementations be tested. See TestPartial and TestCmpToKey for example.

serhiy-storchaka · 2018-07-29T13:55:27Z

Add a sentence ending period and "Path by yourname."

Sorry, what do you mean?

Usually changes from non-core contributors are attributed by adding "Path by contributorname." In your case it will be "Patch by Robert Wright."

Thanks, the typo confused me.

Oh, sorry. I just noticed it!

bedevere-bot · 2018-07-29T13:56:09Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

madman-bob · 2018-07-29T14:30:41Z

I have made the requested changes; please review again

bedevere-bot · 2018-07-29T14:30:43Z

Thanks for making the requested changes!

@serhiy-storchaka, @zooba: please review the changes made to this pull request.

rhettinger · 2018-07-30T08:11:05Z

Why do you think we might need this? IMO, this just adds unnecessary weight to the code base, resulting in a net degradation of maintainability.

madman-bob · 2018-07-30T08:21:25Z

Consistency. If the c_functools module is not available, why should reduce in particular break?

In my opinion, either all of functools should have pure Python fallbacks, or none of them should.

madman-bob · 2018-07-30T13:35:54Z

Oh, and the CLA's now been received, so the label can be removed.

pppery · 2018-07-30T19:38:38Z

+        try:
+            value = next(it)
+        except StopIteration:
+            raise TypeError("reduce() of empty sequence with no initial value")


Shouldn't this be raised "from None"

…exception context

vstinner · 2018-10-19T22:11:05Z

+### reduce() sequence to a single item
+################################################################################
+
+_initial_missing = object()


functools.py already has a sentinel singleton, _NOT_FOUND used by cached_property. Would it be possible to reuse it, instead of adding a second sentinel value?

Not sure how I feel about this suggestion. On the one hand, there would be a (slight) overhead reduction by reusing the same sentinel. On the other, semantically, this isn't really a _NOT_FOUND, so much as a "not given".

As these sentinels aren't part of the public interface, we could rename them into an object that semantically makes sense - but I'm loathe to touch another function, if I can help it.

Ok, keep _initial_missing.

vstinner · 2018-10-19T22:13:19Z

-    if c_functools:
-        func = c_functools.reduce

+class TestReduce:


Can you please rename it to BaseTestReduce to make it more obvious that it's not a regular test case?

Done this way to be consistent with the other tests.

I have no objection to this suggestion, but as it would also need to be done for TestPartial and TestCmpToKey, I feel it is out of scope for this change.

vstinner · 2018-10-19T22:14:05Z

+
+
+@unittest.skipUnless(c_functools, 'requires the C _functools module')
+class TestReduceC(TestReduce, unittest.TestCase):


nitpick: I suggest to move the C implementation after the Python implementation, since the Python impl is always present, whereas the other one is optional :-)

As with my other comment, I have no objection to this suggestion, but as it would also need to be done for TestPartial and TestCmpToKey, I feel it is out of scope for this change.

vstinner

LGTM.

vstinner · 2018-10-25T14:01:53Z

Why do you think we might need this? IMO, this just adds unnecessary weight to the code base, resulting in a net degradation of maintainability.

Yes. For me, it helps me to understand more easily the behavior and subtle cases, without having to read C code which is harder to decipher. It's also common that we maintain a Python a C implementation of the same function. It helps other Python implementations to quickly get new features without having to reimplement them in their own "faster language" (like RPython for PyPy or .NET for IronPython). The datetime module has been fully rewritten in Python recently :-)

For this function, it's just 12 lines of Python (if you ignore doc and empty lines). It wouldn't call it a huge maintenance burden :-)

miss-islington · 2018-10-25T14:02:17Z

Thanks @madman-bob for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-10-25T14:02:35Z

GH-10092 is a backport of this pull request to the 3.7 branch.

(cherry picked from commit e25d5fc) Co-authored-by: madman-bob <madman.bob@hotmail.co.uk>

vstinner · 2018-10-25T14:09:09Z

@madman-bob: Hi, I merged your PR. You wrote that you care about consistency in the test. I agree that it matters: would you mind to propose a PR to implement my proposed changes?

madman-bob · 2018-10-31T11:33:29Z

@vstinner Raised https://bugs.python.org/issue35123 and https://bugs.python.org/issue35124

madman-bob requested review from ncoghlan and rhettinger as code owners July 29, 2018 12:37

the-knights-who-say-ni added the CLA not signed label Jul 29, 2018

bedevere-bot added the awaiting review label Jul 29, 2018

zooba approved these changes Jul 29, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jul 29, 2018

zooba added the needs backport to 3.7 label Jul 29, 2018

serhiy-storchaka requested changes Jul 29, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Jul 29, 2018

serhiy-storchaka removed the CLA not signed label Jul 29, 2018

the-knights-who-say-ni added the CLA not signed label Jul 29, 2018

bpo-32321: Add pure Python fallback for functools.reduce

b00ddde

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 29, 2018

serhiy-storchaka approved these changes Jul 29, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 29, 2018

ncoghlan removed the CLA not signed label Jul 30, 2018

the-knights-who-say-ni added the CLA signed label Jul 30, 2018

pppery reviewed Jul 30, 2018

View reviewed changes

bpo-32321: Raise functools.reduce empty sequence type error in fresh …

b67584e

…exception context

vstinner reviewed Oct 19, 2018

View reviewed changes

vstinner approved these changes Oct 25, 2018

View reviewed changes

serhiy-storchaka assigned rhettinger Oct 25, 2018

vstinner merged commit e25d5fc into python:master Oct 25, 2018

bedevere-bot removed the awaiting merge label Oct 25, 2018

bedevere-bot removed the needs backport to 3.7 label Oct 25, 2018

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 25, 2018

bpo-32321: Add pure Python fallback for functools.reduce (pythonGH-8548)

3c3f333

(cherry picked from commit e25d5fc) Co-authored-by: madman-bob <madman.bob@hotmail.co.uk>

vstinner mentioned this pull request Oct 25, 2018

bpo-32321: Add pure Python fallback for functools.reduce #9884

Closed



		@unittest.skipUnless(c_functools, 'requires the C _functools module')
		class TestReduceC(TestReduce, unittest.TestCase):

Uh oh!

Conversation

madman-bob commented Jul 29, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Jul 29, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 29, 2018

Uh oh!

madman-bob commented Jul 29, 2018

Uh oh!

bedevere-bot commented Jul 29, 2018

Uh oh!

rhettinger commented Jul 30, 2018

Uh oh!

madman-bob commented Jul 30, 2018

Uh oh!

madman-bob commented Jul 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 25, 2018

Uh oh!

miss-islington commented Oct 25, 2018

Uh oh!

bedevere-bot commented Oct 25, 2018

Uh oh!

vstinner commented Oct 25, 2018

Uh oh!

madman-bob commented Oct 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

madman-bob commented Jul 29, 2018 •

edited by bedevere-bot

Loading