Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-132983: Introduce_zstd bindings module#133027
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
_zstd module_zstd bindings moduleemmatyping commentedApr 26, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
E: now fixed |
ZeroIntensity left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice! Some comments at a glance.
Please remove the module state macros. They make this really difficult to review, and don't really add much (isMS_MEMBER(whatever)really that much easier to type thanstate->whatever?)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
emmatyping commentedApr 26, 2025
Thank you for the review@ZeroIntensity! Some of the decisions are just how things were in |
ZeroIntensity commentedApr 26, 2025
There's probably not too much to read about it other than somewhere inPEP-703. You'll just want to replace the locks withcritical sections, which have some magic to prevent deadlocks. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
picnixz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Few preliminary comments, although I'm not sure it can be modified under the given license.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
picnixz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Some other comments I managed to find. I didn't review the decompressor but I think most of my comments that I said for the compressor would apply. The most important ones are essentially function signatures where I want you to usePyObject *self and do a manual cast to the expected type inside the function:
#defineMyType_CAST(op)((MyType *)(op))staticvoidmy_method(PyObject*op) {MyType*self=MyType_CAST(op); ...}
The use of a macro (or a static inline function, up to you) is to allow future runtime type checks.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This commit introduces the `_zstd` module, with bindings to libzstd fromthe pyzstd project. It also includes the unix build system configuration.Windows build system support will be integrated independently as itdepends on integration with cpython-source-deps.
Also removes module state references from the classes in the _zstdmodule and instead uses PyType_GetModuleState()
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
This should avoid races and deadlocks.
The `compress`/`decompress` functions will be moved to Python code for simplicity.C implementations can always be re-added in the future.Also, mark _zstd as not requiring the GIL.
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
bedevere-bot commentedMay 3, 2025
🤖 New build scheduled with the buildbot fleet by@emmatyping for commit30b3934 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133027%2Fmerge If you want to schedule another build, you need to add the🔨 test-with-buildbots label again. |
emmatyping commentedMay 3, 2025
Okay, latest commit should actually work correctly on RHEL 8, I tested it in a container and verified |
bedevere-bot commentedMay 3, 2025
🤖 New build scheduled with the buildbot fleet by@emmatyping for commit10e2e80 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133027%2Fmerge If you want to schedule another build, you need to add the🔨 test-with-buildbots label again. |
emmatyping commentedMay 3, 2025
Okay, configure should now work across all platforms, that was a bit more involved than I was expecting. I wish the configure feedback loop wasn't so long :( |
This comment was marked as abuse.
This comment was marked as abuse.
emmatyping commentedMay 3, 2025
The RHEL8 s390x LTO+PGO failure is unrelated, should be#133261 (comment) (other RHEL 8 s390x may fail too for the same reason?) But the changes have worked on RHEL8:https://buildbot.python.org/#/builders/420/builds/1735
|
hugovk commentedMay 3, 2025
I normally pass |
emmatyping commentedMay 3, 2025
TIL! Thanks that is really handy. |
3b43335 intopython:mainUh oh!
There was an error while loading.Please reload this page.
ZeroIntensity commentedMay 4, 2025
Yay! Congrats for getting this accepted and into 3.14 on time@emmatyping |
emmatyping commentedMay 4, 2025
Thank you, as well as everyone who reviewed! The feedback was incredibly valuable. Now on to merging the Python code:#133365 |
* origin/main: (111 commits)pythongh-91048: Add filename and line number to external inspection routines (pythonGH-133385)pythongh-131178: Add tests for `ast` command-line interface (python#133329) Regenerate pcbuild.sln in Visual Studio 2022 (python#133394)pythongh-133042: disable HACL* HMAC on Emscripten (python#133064)pythongh-133351: Fix remote PDB's multi-line block tab completion (python#133387)pythongh-109700: Improve stress tests for interpreter creation (pythonGH-109946)pythongh-81793: Skip tests for os.link() to symlink on Android (pythonGH-133388)pythongh-126835: Rename `ast_opt.c` to `ast_preprocess.c` and related stuff after moving const folding to the peephole optimizier (python#131830)pythongh-91048: Relax test_async_global_awaited_by to fix flakyness (python#133368)pythongh-132457: make staticmethod and classmethod generic (python#132460)pythongh-132805: annotationlib: Fix handling of non-constant values in FORWARDREF (python#132812)pythongh-132426: Add get_annotate_from_class_namespace replacing get_annotate_function (python#132490)pythongh-81793: Always call linkat() from os.link(), if available (pythonGH-132517)pythongh-122559: Synchronize C and Python implementation of the io module about pickling (pythonGH-122628)pythongh-69605: Add PyREPL import autocomplete feature to 'What's New' (python#133358) bpo-44172: Keep reference to original window in curses subwindow objects (pythonGH-26226)pythonGH-133231: Changes to executor management to support proposed `sys._jit` module (pythonGH-133287)pythongh-133363: Fix Cmd completion for lines beginning with `! ` (python#133364)pythongh-132983: Introduce `_zstd` bindings module (pythonGH-133027)pythonGH-91048: Add utils for printing the call stack for asyncio tasks (python#133284) ...
ned-deily left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@emmatyping,@gpshead,@hugovk Some showstopper problems with trying to build_zstd.
| have_libzstd=no | ||
| AC_DEFUN([TEST_ZSTD_VERSION],[ | ||
| AC_MSG_CHECKING([if libzstd is new enough]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There seem to be two problems with thenew enough check:
- There should be
AC_MSG_RESULT([yes])andAC_MSG_RESULT([no])calls to complete the messages:
checking for libzstd... yeschecking if libzstd is new enough... checking for hstrerror... yes| have_libzstd=yes | ||
| ]) | ||
| PKG_CHECK_MODULES([LIBZSTD],[libzstd],[TEST_ZSTD_VERSION()],[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
and 2. There's something funky going on here with the two calls toTEST_ZSTD_VERSION() such that version number test fails and the failure is sort of hidden inconfig.log:
configure:22400: checking for libzstdconfigure:22407: $PKG_CONFIG --exists --print-errors "libzstd"configure:22410: $? = 0configure:22424: $PKG_CONFIG --exists --print-errors "libzstd"configure:22427: $? = 0configure:22727: result: yesconfigure:22730: checking if libzstd is new enoughconfigure:22756: gcc -o conftest conftest.c -ldl >&5conftest.c:325:10: fatal error: 'zstd.h' file not found 325 | #include "zstd.h" | ^~~~~~~~1 error generated.configure:22756: $? = 1configure: program exited with status 1This later results in:checking for stdlib extension module _zstd... disabled
I suspect the problem is the call toTEST_ZSTD_VERSION() on line 5437 fails because at that pointCPPFLAGS is no longer set to include$LIBZSTD_CFLAGS and the test compile would succeed only if the header files are installed in a default system location, like/usr/include, which won't be the case, for example, on builds on macOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Postscript: a likely confirmation of my suspicion above. For our CI runs on macOS, we use Homebrew to supply third-party libs (likelibzstd) not supplied by Apple. If you look at CI logs for current PRs (i.e. after this PR was merged),_zstd is disabled (as above) on theghcr.io/cirruslabs/macos-runner:sonomarunner but succeeds on themacos-13 runner. The difference is that the former is Apple Silicon (arm64) hardware where Homebrew installs to a non-standard/opt/homebrew prefix, whereas themacos-13 runner is Intel hardware where Homebrew continues to install to the legacy/usr/local prefix which, no doubt, is included in the compiler's default search path.
emmatyping commentedMay 6, 2025
To follow up on autoconf issues Ned raised, those were addressed in#133479 |
-python/cpython#133027-python/cpython#133366-python/cpython#133284-python/cpython#133398-python/cpython#131298-python/cpython#132438-python/cpython#133012---------Co-authored-by: Wingy <git@wingysam.xyz>Co-authored-by: Geoffrey Thomas <geofft@ldpreload.com>
* Add _zstd module forhttps://peps.python.org/pep-0784/This commit introduces the `_zstd` module, with bindings to libzstd fromthe pyzstd project. It also includes the unix build system configuration.Windows build system support will be integrated independently as itdepends on integration with cpython-source-deps.* Add _zstd to modules* Fix path for compression.zstd module* Ignore _zstd module like _io* Expand module state macros to improve code qualityAlso removes module state references from the classes in the _zstdmodule and instead uses PyType_GetModuleState()* Remove backticks suggested in reviewCo-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>* Use critical sections to lock object stateThis should avoid races and deadlocks.* Remove compress/decompress and mark module as not reliant on the GILThe `compress`/`decompress` functions will be moved to Python code for simplicity.C implementations can always be re-added in the future.Also, mark _zstd as not requiring the GIL.* Lift critical section to avoid clang warning* Respond to comments by picnixz* Call out pyzstd explicitly in license descriptionCo-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>* Use a much more robust implementation...... for `get_zstd_state_from_type`Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>* Use PyList_GetItemRef for thread safety purposes* Use a macro for the minimum supported version* remove const from primivite types* Use PyMem_New in another spot* Simplify error handling in _get_frame_size* Another simplification of error handling in get_frame_info* Rename _module_state to mod_state* Rewrite comment explaining the context of the code* Add link to pyzstd* Add TODO about refactoring dict training code* Use PyModule_AddObjectRef over PyModule_AddObjectPyModule_AddObject is soft-deprecated, so we should use PyModule_AddObjectRef* Check result of OutputBufferGrow* Simplify return logic in `add_constant_to_type`Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>* Ignore return value of _zstd_clear()Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>* Remove redundant comments* Remove __reduce__ from ZstdDictWe should instead document that to pickle a dictionary a user should usethe `.dict_content` attribute.* Use PyUnicode_FromFormat instead of a buffer* Don't use C constants/types in error messages* Make error messages easier to understand for Python users* Lower minimum required version 1.4.0* Use casts and make slot function signatures correct* Be consistent with CPython on const usage* Make else clauses in line with PEP 7* Fix over-indented blocks in argument clinic* Add critical section around ZSTD_DCtx_setParameter* Add a TODO about refactoring critical sections* Use Py_UNREACHABLE* Move bytes operations out of Py_BEGIN_ALLOW_THREADS* Add TODO about ensuring a lock is held* Remove asserts that may not be correct* Add TODO to make ZstdDict and others GC objects* Make objects GC tracked* Remove unused include* Fix some memory issues* Fix refleaks on module and in ZstdDict* Update configure to check for ZDICT_finalizeDictionary* Properly check version in configure* exit(1) if check fails* Use AC_RUN_IFELSE* Use a define() to re-use version check* Actually properly set _zstd module status based on version---------Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
This is part 2 (but parallel to part 1) for implementing PEP 784. This is probably the meat of the implementation making up ~half of the LOC in the overall diff.
This change:
_zstdmodule inModules/_zstd. Tests will be included when the Pythoncompression.zstdmodule is added. If people think the tests should be included, I can merge thecompression.zstdchanges into the PR, but I wanted to separate them to make the PRs a bit more manageable.I added skip news as I'd like to write a holistic NEWS/What's New entry once the entire implementation has landed. If people think each PR should have NEWS I can write something up.
TODO before merge:
_zstdgets installed📚 Documentation preview 📚:https://cpython-previews--133027.org.readthedocs.build/