Repository navigation
Prefer native Zstandard compression workers - #185
zhou-jianwen wants to merge 1 commit into
Conversation
|
@rhpvorderman Any opinion on entirely AI-generated PRs with overly verbose PR descriptions? |
|
The verbose PR descriptions are annoying, but reviewing fixes is always less time consuming than making them ourselves. Provided the fixes are of good quality. So I'd judge the merit of the PR based on that. Currently python-isal is undergoing a veritable bombardment of high quality AI assisted improves by a user who likes the project. It is really improving the project, so I am not complaining. Also, given that all the AIs are probably trained on our code without permission it is about time they give something back ;-). |
There was a problem hiding this comment.
This AI generated PR is quite poor. If the native library does not accept threads, there is no reason why a fallback to the application should not be used. The change is not properly integrate in the logic flow. The tests are also testing all sorts of things about the code flow, but not if the returned object is the desired one. Which is what matters.
| assert compresslevel != 0 | ||
| if compresslevel is None: | ||
| compresslevel = XOPEN_DEFAULT_ZST_COMPRESSION | ||
| if ( |
There was a problem hiding this comment.
I don't like how this extensive if clause is placed at the start of the logic. I feel that could be handled quite a bit better. There are already several tests for zstd and the number of threads. Adding extra makes the function harder to maintain. Please refactor the function so it flows more logically.
| (that is, no subprocess is spawned). | ||
|
|
||
| Set threads to 0 to force opening the file without using a subprocess. | ||
| Zstandard writing and appending prefer native compression threads when the |
There was a problem hiding this comment.
This is true for multiple backends. No need to add a specific paragraph for zstandard here.
| def test_zst_native_roundtrip_append(tmp_path, monkeypatch, threads, file_like): | ||
| if threads != 0 and zstd.CompressionParameter.nb_workers.bounds()[1] == 0: | ||
| pytest.skip("Native Zstandard multithreading not supported") | ||
| import xopen as xopen_module |
There was a problem hiding this comment.
Why would the import be in the function? It is at the top of the test file.
| def unexpected_process(*args, **kwargs): | ||
| pytest.fail("Native Zstandard writing must not spawn a process") | ||
|
|
||
| monkeypatch.setattr(xopen_module, "_PipedCompressionProgram", unexpected_process) |
There was a problem hiding this comment.
What a convoluted way to test this. Simply test if the return object is what you want rather than inserting all sorts of nonsense throughout the library.
| "threads,cpus,workers", [(None, 2, 2), (None, 8, 4), (1, 8, 1), (7, 2, 7)] | ||
| ) | ||
| @pytest.mark.parametrize("compresslevel", [None, 1, 9]) | ||
| def test_zst_native_options( |
There was a problem hiding this comment.
This whole test seems nonsense to me. It inserts code to capture a dictionary, then tests that dictionary. But we are not really testing the result here.
Zstandard writes and appends currently prefer an external
zstdprocess wheneverthreadsis nonzero or omitted. Prefercompression.zstd/backports.zstdcompression workers when the linked library supports them, keeping the existing automatic limit of four workers and honoring explicit counts.Pass the compression level and worker count together through
options, and enable the content checksum used by the external CLI. Check threading capability before opening output; unsupported builds retain the external-then-single-thread fallback. Native errors propagate without retrying another backend after output has been opened. Reading andthreads=0behavior are unchanged.Fixes #180.
Validation on macOS arm64:
backports.zstd) and Python 3.14.10 (compression.zstd): full suites each passed 404 tests, with 4 skips; unmodified baseline passed 379 tests, with 4 skips.git diff --checkpass. Local mypy reports the same 9 errors on pristinemainand this branch: missing optionalisal/zlib_ngimports and associated redefinitions, plus macOSos.sched_getaffinitytyping. All 13 hosted test/lint jobs now pass, including Linux mypy and the Linux, macOS and Windows test matrix.README backend selection and the changelog are updated. No throughput benchmark is claimed.
AI assistance: OpenAI Codex was used for implementation, tests and automated review.
Coverage status: Codecov project is still red. Its comparison uses
89bfc5d(June 3) rather than the actual PR baseca60aa1, for which Codecov has no report. The comparison includes intervening upstream changes. All added executable production lines are covered; some new test skip/fail guards are not, and this project includes tests in coverage. This does not establish how much of the reported drop belongs to this PR. No coverage thresholds or exclusions were changed.Related follow-up: #186 restores the previously unreachable native LZ4 CI step on an independent branch. Its hosted tests and Codecov pass; it has not been merged into this branch.