Add zstd and xz support - #2893
Conversation
|
This PR introduces the ability to use external software, i.e. zstd and xz but it makes no attempt to check whether the program is present. I have not tried to handle that situation gracefully but personally I don't think think that it's necessary as I haven't made any changes to the default settings, if indeed there are any, as rose detects which compression utility to called based on file extension or explicit use of the compress keyword - either of which would expect the user to make a deliberate decision to use zstd or xz. I note that zstd is increasingly widely used now and even if it isn't installed by default on a particular linux distro (or mac os), it is almost certain to be present in the package manager and is apparently pretty likely to be installed as a dependency of another package (i.e. pygraphviz (via libtiff and others) in the case of rose) such is its ubiquity. Furthermore it has been approved for addition to the standard library in future versions of python so it is only likely to become more integrated in to python: https://discuss.python.org/t/pep-784-adding-zstandard-to-the-standard-library/87377/138 The same arguments can be made for xz (which uses liblzma) but additionally it does appear to be installed as standard, at least for RHEL 9.4. I'm less wedded to this though and would happily remove it if required. I just added it to check it was that easy... |
|
I also note that there's obviously a lot of very-near-duplication of code here and it would likely be better to implement something more generic (especially given the reliance on external utilities which effectively have identical command line arguments (by design, I think?)). But I am keen to get this in in some form initially and potentially revisit when I have more time (and more skills!) if it has the potential to be beneficial. |
oliver-sanders
left a comment
oliver-sanders
left a comment
There was a problem hiding this comment.
Thanks for this PR (and special thanks for sorting out the tests and documentation too 👏)!
We'll try to get this in soon, but be aware, we've got a very heavy review workload right now, so apologies if it takes longer.
| self.app_runner.popen.run_simple(command, shell=True) | ||
| self.app_runner.fs_util.delete(tar_name) | ||
|
|
||
| if target.compress_scheme in self.ZSTD_EXTS: |
There was a problem hiding this comment.
| if target.compress_scheme in self.ZSTD_EXTS: | |
| elif target.compress_scheme in self.ZSTD_EXTS: |
| self.app_runner.popen.run_simple(command, shell=True) | ||
| self.app_runner.fs_util.delete(tar_name) | ||
|
|
||
| if target.compress_scheme in self.XZ_EXTS: |
There was a problem hiding this comment.
| if target.compress_scheme in self.XZ_EXTS: | |
| elif target.compress_scheme in self.XZ_EXTS: |
| ) | ||
| os.close(fdsec) | ||
| target.work_source_path = zst_name | ||
| command = f"zstd --rm -T{cores} -c '{tar_name}' >'{zst_name}'" |
There was a problem hiding this comment.
Due to the high likelyhood of this command being run directly on Cylc servers, we will have to be careful with this.
There was a problem hiding this comment.
Thanks @oliver-sanders . I agree and I hope that the default being to not use this is a good trade-off between exposing useful functionality and maintaining order!
I have updated the documentation to note that this should be used with caution on shared resources.
9a73855 to
fc9cca8
Compare
|
The "test / docs" test seems to be failing for reasons unrelated to this change, please ignore. The wonderfully named "test / test" tests, however, seem to be failing for legitimate reasons. It looks like |
| handler = compress_manager.get_handler(target.compress_scheme) | ||
| handler.compress_sources(target, work_dir) | ||
| compress_args = {"threads": target.compress_threads} | ||
| handler.compress_sources(target, work_dir, **compress_args) |
There was a problem hiding this comment.
Tried a very simple check
mode=rose_arch
[arch]
# rose-app.conf
command-format=cp %(sources)s %(target)s
target-prefix=/home/users/tim.pillinger/cylc-src/rose-apps/arch/archive/
source-prefix=/home/users/tim.pillinger/cylc-src/rose-apps/arch/source/
[arch:world.out]
source='world.out'
[arch:gunzipme.gz]
source='gunzipme.out'
[arch:targunzipme.tar.gz]
source='targunzipme.out'export CYLC_WORKFLOW_ID='hippo'
export CYLC_TASK_ID='task-run'
export CYLC_TASK_NAME='task-run'
export CYLC_TASK_CYCLE_POINT='task-run'
export CYLC_TASK_LOG_ROOT="${HERE}/log"
echo Running app from "${HERE}/app"
rose task-run --config="${HERE}/app"and got an error:
[FAIL] RoseArchGzip.compress_sources() got an unexpected keyword argument 'threads'
I think that you need to add the threads argument to rose_arch_gzip.py and possibly other items in that folder. You may want to consider emitting a warning if threads != 1 and program_is_single_thread:
This test works for me locally? Is that the most recent version of the code? |
|
Hmm, testing locally (cazldf...), this test passes on upstream/master but fails on david-rundle:feature/add-zstd-xz, so this does look like a genuine error. Do you have zstd and xz installed on your box / in your environment? Does the test need them to be installed? If the test does need these utils installed, let us know and we can work out where to install them in CI. The error can be found from this line onwards: https://github.com/metomi/rose/actions/runs/14859530104/job/41720858720#step:13:14 I've pulled out a couple of error messages that might be pertinent: |
| self.app_runner = app_runner | ||
|
|
||
| def compress_sources(self, target, work_dir): | ||
| def compress_sources(self, target, work_dir, threads="1"): |
There was a problem hiding this comment.
IMO threads should be cast to int as soon as we parse the config.
|
I've written you a couple of integration tests at david-rundle#1 Tests running at https://github.com/wxtim/rose/actions/runs/14864492636 |
|
@david-rundle - Is there anything I can do to help you move this forward? |
|
Just looking at this again now having merged your changes and am getting loads of errors like: and it's been a while so I don't really remember any of the workflow for running the tests so may need some help dusting this off! |
|
@david-rundle - I've fixed the conflict for you, and retriggered the tests. I have replicated the test failure locally, but I'm not clear on the causes. Anyway - to run the test locally I've tried running the tests and I'm getting the same failure - it looks like the workflow is failing because the task is failing, and you can look at the details of the failure at |
|
New developments... Python has now added support for zstd in version 3.14, this makes it much more attractive for use in Rose as we would no longer be relying on an external dependency (which may or may not be installed). Although this functionality would only be present when Python >= 3.14.0 is installed. |
|
(unassigning this to re-focus my review queue, feel free to re-assign when undrafted) |
a68c44e to
a83f957
Compare
Add "zstd" and "xz" as compression schemes for the rose_arch built-in application, alongside the existing "gzip", and a "compress-threads" setting that lets zstd compress using more than one CPU core. Compression is now driven through a single generic compress() function shared by all three compressors and the tar handler, instead of copy-and-pasted per-compressor code (as attempted in the last version of this code): adding a compressor is now a matter of writing a small handler subclass rather than another near-identical chunk of shell-out logic. For xz and zstd, compression is done with a native Python library where one is available (stdlib "lzma" for xz; stdlib "compression.zstd" on Python 3.14+, falling back to the "zstandard" PyPI package on older Pythons, for zstd), falling back to the command line tool otherwise. Both the library and command line paths stream the data through in chunks rather than reading a whole source into memory, so archiving sources of hundreds of gigabytes is not a problem. Multi-threaded compression is the main motivation for this change, and only zstd supports it for a single stream. This is validated when the rose_arch configuration is parsed, and is covered thoroughly in the tests: unit tests drive every scheme's compress_sources() with a bad thread count and confirm only zstd schemes accept more than one thread, functional tests exercise multi-threaded zstd compression end-to-end through a real cylc task, and a dedicated test forces the chunked in-process compression loop to run over many chunks and confirms the result decompresses correctly.
a83f957 to
ffa9ab5
Compare
… native python 3.14 implementation so I have added the ability to optionally drop out to the CLI which seems unaffected. It's hard to say whether the issue is just IO-boundness of spice (the only place where 3.14 is readily available) or whether something else is going on. Either way it's a useful feature.
3 participants
This commit adds support in rose for zstd and xz (with default options) using the same syntax as for gzip, including for compressing tarballs of various flavours.
It adds tests for zstd (46) and xz (47) that are clones of the gzip rose_arch test (32) and a slightly beefier test for zstd (48) that is a clone of (07).