GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-132983: Introduce `_zstd` bindings module by emmatyping · Pull Request #133027 · python/cpython Conversation
|
|
E: now fixed |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 (is MS_MEMBER(whatever) really that much easier to type than state->whatever?)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Thank you for the review @ZeroIntensity! Some of the decisions are just how things were in |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
There's probably not too much to read about it other than somewhere in PEP-703. You'll just want to replace the locks with critical sections, which have some magic to prevent deadlocks. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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 use PyObject *self and do a manual cast to the expected type inside the function:
#define MyType_CAST(op) ((MyType *)(op)) static void my_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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
This commit introduces the `_zstd` module, with bindings to libzstd from the pyzstd project. It also includes the unix build system configuration. Windows build system support will be integrated independently as it depends on integration with cpython-source-deps.
Also removes module state references from the classes in the _zstd module and instead uses PyType_GetModuleState()
Co-authored-by: Stan Ulbrych <[email protected]>
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 <[email protected]>
|
🤖 New build scheduled with the buildbot fleet by @emmatyping for commit 30b3934 🤖 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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Okay, latest commit should actually work correctly on RHEL 8, I tested it in a container and verified |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🤖 New build scheduled with the buildbot fleet by @emmatyping for commit 10e2e80 🤖 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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 :( |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I normally pass |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
TIL! Thanks that is really handy. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yay! Congrats for getting this accepted and into 3.14 on time @emmatyping |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thank you, as well as everyone who reviewed! The feedback was incredibly valuable. Now on to merging the Python code: #133365 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
* 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) ...
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| have_libzstd=no | ||
| AC_DEFUN([TEST_ZSTD_VERSION],[ | ||
| AC_MSG_CHECKING([if libzstd is new enough]) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
There seem to be two problems with the new enough check:
AC_MSG_RESULT([yes]) and AC_MSG_RESULT([no]) calls to complete the messages:checking for libzstd... yes
checking if libzstd is new enough... checking for hstrerror... yes
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| have_libzstd=yes | ||
| ]) | ||
|
|
||
| PKG_CHECK_MODULES([LIBZSTD], [libzstd], [TEST_ZSTD_VERSION()], [ |
There was an error while loading. Please reload this page.
There was a problem 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 to TEST_ZSTD_VERSION() such that version number test fails and the failure is sort of hidden in config.log:
configure:22400: checking for libzstd
configure:22407: $PKG_CONFIG --exists --print-errors "libzstd"
configure:22410: $? = 0
configure:22424: $PKG_CONFIG --exists --print-errors "libzstd"
configure:22427: $? = 0
configure:22727: result: yes
configure:22730: checking if libzstd is new enough
configure:22756: gcc -o conftest conftest.c -ldl >&5
conftest.c:325:10: fatal error: 'zstd.h' file not found
325 | #include "zstd.h"
| ^~~~~~~~
1 error generated.
configure:22756: $? = 1
configure: program exited with status 1
This later results in:
checking for stdlib extension module _zstd... disabled
I suspect the problem is the call to TEST_ZSTD_VERSION() on line 5437 fails because at that point CPPFLAGS 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 (like libzstd) 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 the ghcr.io/cirruslabs/macos-runner:sonoma runner but succeeds on the macos-13 runner. The difference is that the former is Apple Silicon (arm64) hardware where Homebrew installs to a non-standard /opt/homebrew prefix, whereas the macos-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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
To follow up on autoconf issues Ned raised, those were addressed in #133479 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
- 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 <[email protected]> Co-authored-by: Geoffrey Thomas <[email protected]>
* Add _zstd module for https://peps.python.org/pep-0784/ This commit introduces the `_zstd` module, with bindings to libzstd from the pyzstd project. It also includes the unix build system configuration. Windows build system support will be integrated independently as it depends 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 quality Also removes module state references from the classes in the _zstd module and instead uses PyType_GetModuleState() * Remove backticks suggested in review Co-authored-by: Stan Ulbrych <[email protected]> * Use critical sections to lock object state This should avoid races and deadlocks. * Remove compress/decompress and mark module as not reliant on the GIL 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. * Lift critical section to avoid clang warning * Respond to comments by picnixz * Call out pyzstd explicitly in license description Co-authored-by: Adam Turner <[email protected]> * Use a much more robust implementation... ... for `get_zstd_state_from_type` Co-authored-by: Bénédikt Tran <[email protected]> * 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_AddObject PyModule_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 <[email protected]> * Ignore return value of _zstd_clear() Co-authored-by: Bénédikt Tran <[email protected]> * Remove redundant comments * Remove __reduce__ from ZstdDict We should instead document that to pickle a dictionary a user should use the `.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 <[email protected]> Co-authored-by: Adam Turner <[email protected]> Co-authored-by: Bénédikt Tran <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.