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

gh-121676: Raise a DeprecationWarning if the Python implementation of functools.reduce is called with a keyword args #121677

Merged
merged 14 commits into from
Jan 1, 2025

Conversation

Eclips4
Copy link
Member

@Eclips4 Eclips4 commented Jul 13, 2024

Lib/functools.py Outdated Show resolved Hide resolved
vstinner
vstinner previously approved these changes Jul 13, 2024
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

In general, the C implementation is always used so functools.reduce() doesn't accept keyword arguments. This change fix an inconsistency when the C extension (_functools) cannot be loaded.

@rhettinger
Copy link
Contributor

ISTM that adding an argument restriction to an already published function is a breaking change.

@ambv
Copy link
Contributor

ambv commented Jul 15, 2024

I see how @Eclips4 would conclude that the likelihood of users running the Python version of functools.reduce is low. However, I have to agree with Nikita and Raymond here. It is unknowable whether users had or had not depended on keyword arguments here, especially after the function existing for so long. A lot of code isn't open.

In general, it might be tempting to treat a case like this one as a special case and relax our usual standards, but I believe it's not worth doing: we're not fixing a crippling problem, and we're not unblocking users to perform a legitimate operation. This change is a cleanup to improve consistency, it's not urgent.

Moreover, the function isn't actually documented as positional-only, so we cannot claim that the current behavior of the pure Python implementation was against stated intent.

Therefore, the responsible thing to do here would be to deprecate keyword argument use (through an awkward *args, **kwargs wrapper that would discover keyword usage and warn). Best case scenario, we might ask Thomas if he'd be okay with this new deprecation would be okay for 3.13 still, even though it's officially too late for that. But as Kirill says, the blast radius is limited as the function is shadowed by the C implementation for the vast majority of users.

@Eclips4
Copy link
Member Author

Eclips4 commented Jul 15, 2024

Thanks Łukasz for sharing your thoughts!

Moreover, the function isn't actually documented as positional-only, so we cannot claim that the current behavior of the pure Python implementation was against stated intent.

Actually it's documented as positional-only, but only in 3.13+ branches:
https://docs.python.org/3.13/library/functools.html#functools.reduce

I absolutely agree with you that if we can deprecate it for 3.13, that would be fine. If not, that would also be fine (should we then revert the change to the docs mentioned above?).
cc @Yhg1s

@Eclips4 Eclips4 removed the needs backport to 3.12 bug and security fixes label Jul 15, 2024
@Yhg1s
Copy link
Member

Yhg1s commented Jul 15, 2024

No, I don't think this should be done in 3.13. The potential impact is low, but so is the benefit. It's fine to leave it documented as positional-only, but changes in behaviour (if warranted at all, and following the normal deprecation process) should go into 3.14 instead

@Eclips4 Eclips4 removed the needs backport to 3.13 bugs and security fixes label Jul 15, 2024
@Eclips4 Eclips4 changed the title gh-121676: Fix the signature of the Python implementation of functools.reduce gh-121676: Raise a DeprecationWarning if the Python implementation of functools.reduce is called with a keyword args Jul 15, 2024
@Eclips4 Eclips4 requested review from vstinner, sobolevn and ambv July 15, 2024 20:20
@Eclips4 Eclips4 marked this pull request as draft July 15, 2024 20:58
@Eclips4 Eclips4 marked this pull request as ready for review July 15, 2024 21:26
@vstinner vstinner dismissed their stale review July 16, 2024 09:58

PR was rewritten since I approved it

@vstinner
Copy link
Member

What's the status of this PR? I'm now confused :-(

@Eclips4
Copy link
Member Author

Eclips4 commented Oct 29, 2024

What's the status of this PR? I'm now confused :-(

I'm waiting for PR #125917 to be merged, so the initial argument would be pos-and-keyword. Therefore, I will change this PR to raise a DeprecationWarning if the Python version of functools.reduce is called with a function or sequence as a keyword argument. What do you think?

@skirpichev skirpichev self-requested a review November 14, 2024 12:16
@skirpichev
Copy link
Member

Now #125917 is merged. I think that @vstinner review should be dismissed as this pr reworked to allow initial kwarg.

Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Shouldn't we also add something in the "Deprecated" section of 3.14?

Lib/functools.py Outdated Show resolved Hide resolved
@Eclips4 Eclips4 requested a review from picnixz December 28, 2024 17:00
Copy link
Member

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

LGTM, except for missing comment on moved import.

Lib/test/test_functools.py Show resolved Hide resolved
@Eclips4 Eclips4 requested a review from picnixz December 29, 2024 14:48
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I'm suggesting renaming _warn_kwargs to something more explicit for anyone stumbling upon the code. I'm also suggesting deleting the decorator from the module's global names.

I'm not sure if all my suggestions will pass the CI so just double-check before hitting the "commit suggestion" button if you want.

Lib/functools.py Outdated Show resolved Hide resolved
Lib/functools.py Outdated Show resolved Hide resolved
Lib/functools.py Outdated Show resolved Hide resolved
Lib/functools.py Outdated Show resolved Hide resolved
Lib/functools.py Outdated Show resolved Hide resolved
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@picnixz picnixz self-requested a review January 1, 2025 10:54
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

LGTM.

@Eclips4 Eclips4 merged commit d903b17 into python:main Jan 1, 2025
38 checks passed
@Eclips4
Copy link
Member Author

Eclips4 commented Jan 1, 2025

Thanks to all who participated! ❤️

@Eclips4 Eclips4 deleted the issue-121676 branch January 1, 2025 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.