Skip to content

gh-118830: Bump DEFAULT_PROTOCOL to 5 (#118830) #119340

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

Merged
merged 4 commits into from
Jul 19, 2024
Merged

Conversation

mdkcore0
Copy link
Contributor

@mdkcore0 mdkcore0 commented May 21, 2024

I didn't have add a news entry, as it is not a user facing change. Should do I add it?


📚 Documentation preview 📚: https://cpython-previews--119340.org.readthedocs.build/

@bedevere-app
Copy link

bedevere-app bot commented May 21, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@mdkcore0
Copy link
Contributor Author

mdkcore0 commented May 21, 2024

On a second thought it makes sense to add it, but in which section this change falls in?

@carljm
Copy link
Member

carljm commented May 21, 2024

Yes, this deserves a NEWS entry, and it should go in Library section, I think.

@ambv
Copy link
Contributor

ambv commented May 21, 2024

This not only deserves a change log entry but also a "What's New in Python 3.14" entry.

@mdkcore0
Copy link
Contributor Author

@carljm, @ambv: I've added requested entries

For the "WhatsNew in 3.14" I looked on how it was added for python 3.8 (the rst file), it includes some wording about the impact on performance (under Optimizations section):

  • The default protocol in the :mod:pickle module is now Protocol 4,
    first introduced in Python 3.4. It offers better performance and smaller
    size compared to Protocol 3 available since Python 3.0.

Is it a good idea to add something like that here? Since I'm starting contributing now, I will need some help to fulfill it :)

@mdkcore0 mdkcore0 force-pushed the gh-118830 branch 3 times, most recently from b65e9fb to 70676b2 Compare June 12, 2024 17:31
@picnixz
Copy link
Member

picnixz commented Jun 13, 2024

Before bumping it to version 5, I would like to address #120380 since it could introduce breakage. At least, there was something that was not correctly handled for pickle's buffers.

@mdkcore0
Copy link
Contributor Author

mdkcore0 commented Jun 18, 2024

Before bumping it to version 5, I would like to address #120380 since it could introduce breakage. At least, there was something that was not correctly handled for pickle's buffers.

no problem, thanks!!

@picnixz
Copy link
Member

picnixz commented Jun 27, 2024

Now that #120380 is fixed, I think this can be done, unless someone finds something that is broken for protocol 5 or something that should be supported for protocol 5 and that is not. There are #84895 and #89467 which could be something that is desired before bumping the default protocol to protocol 5 since they could dramatically improve performances (especially the second one).

@encukou
Copy link
Member

encukou commented Jul 3, 2024

I don't think either of those needs to block this PR.

I've resolved the conflict in the What's New document, and in the process moved the entry from Optimizations to Improved Modules. Unlike version 4, version 5 isn't primarily a performance improvement.

@hugovk
Copy link
Member

hugovk commented Jul 3, 2024

See also the failed CI check:

Generated files not up to date.
Perhaps you forgot to run make regen-all or build.bat --regen. ;)

@mdkcore0
Copy link
Contributor Author

mdkcore0 commented Jul 3, 2024

See also the failed CI check:

Generated files not up to date.
Perhaps you forgot to run make regen-all or build.bat --regen. ;)

oh, cool
I run the mentioned command, should I commit the modified files them (Modules/_pickle.c and Modules/clinic/_pickle.c.h)?

@hugovk
Copy link
Member

hugovk commented Jul 3, 2024

Yes please!

@encukou encukou merged commit d66b061 into python:main Jul 19, 2024
36 checks passed
@encukou
Copy link
Member

encukou commented Jul 19, 2024

Thank you!

@mdkcore0 mdkcore0 deleted the gh-118830 branch July 19, 2024 16:44
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.

8 participants