Opened 5 years ago
Last modified 4 years ago
#3815 new defect
compression: libzstd should be a default config and a requirement if ZSTD is default compression method
| Reported by: | mlennert | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | 7.6.2 |
| Component: | Raster | Version: | svn-trunk |
| Keywords: | compression zstd zlib | Cc: | |
| CPU: | Unspecified | Platform: | Unspecified |
Description
I just came upon the problem that in my grass installation ZSTD is configured and I thus worked with raster data which was automatically compressed using ZSTD, but when I transferred the data to our HPC system, I realized that grass there is not configure with ZSTD and the latter is currently not available on the system.
As the r.compress man page says, ZSTD is the default compressor if available. However, when I check in current trunk's configure, it says:
--with-zstd support Zstandard functionality (default: no)"
Does that mean that even if libzstd is available on a system, GRASS is not compiled with support for it, unless --with-zstd is set ?
I find this situation a bit uncomfortable as it can lead to situations where the user creates data without actively thinking about the compression method and this data is the unusable elsewhere.
I would plead, thus, for libzstd to become a compulsory requirement for GRASS GIS, and for the configure script to default to --with-zstd=yes.
Or the default compression method should be put back to ZLIB.
Change History (19)
follow-up: 2 comment:1 by , 5 years ago
follow-up: 3 comment:2 by , 5 years ago
Replying to mmetz:
Replying to mlennert:
I just came upon the problem that in my grass installation ZSTD is configured and I thus worked with raster data which was automatically compressed using ZSTD, but when I transferred the data to our HPC system, I realized that grass there is not configure with ZSTD and the latter is currently not available on the system.
As the r.compress man page says, ZSTD is the default compressor if available. However, when I check in current trunk's configure, it says:
--with-zstd support Zstandard functionality (default: no)"Does that mean that even if libzstd is available on a system, GRASS is not compiled with support for it, unless --with-zstd is set ?
yes
I find this situation a bit uncomfortable as it can lead to situations where the user creates data without actively thinking about the compression method and this data is the unusable elsewhere.
I would plead, thus, for libzstd to become a compulsory requirement for GRASS GIS, and for the configure script to default to --with-zstd=yes.
Done in trunk r74391. The default is now
--with-zstd=yes, you can disable ZSTD support with--with-zstd=no. To be backported I guess.
+1 for backporting.
Should libzstd be added to REQUIREMENTS.html ?
follow-up: 5 comment:3 by , 5 years ago
Replying to mlennert:
Replying to mmetz:
Replying to mlennert:
[...]
I would plead [...] for libzstd to become a compulsory requirement for GRASS GIS, and for the configure script to default to --with-zstd=yes.
Done in trunk r74391. The default is now
--with-zstd=yes, you can disable ZSTD support with--with-zstd=no. To be backported I guess.+1 for backporting.
Done in relbr76 r74398
Should libzstd be added to REQUIREMENTS.html ?
Yes, done in r74396,9 (trunk, relbr76)
comment:4 by , 5 years ago
| Milestone: | 7.8.0 → 7.6.2 |
|---|
follow-up: 6 comment:5 by , 5 years ago
Replying to mmetz:
Replying to mlennert:
+1 for backporting.
Done in relbr76 r74398
Just FYI: This broke the manual-building cronjobs on the grass.osgeo.org server but I fixed it by now actively disabling zstd in the configure step (Debian Jessie doesn't have zstd yet and #3486 = compile in a docker container is still open).
In any case, the manuals on the server are now back.
follow-up: 7 comment:6 by , 5 years ago
Replying to neteler:
Replying to mmetz:
Replying to mlennert:
+1 for backporting.
Done in relbr76 r74398
Just FYI: This broke the manual-building cronjobs on the grass.osgeo.org server but I fixed it by now actively disabling zstd in the configure step (Debian Jessie doesn't have zstd yet and #3486 = compile in a docker container is still open).
A good reason to not make zstd a compulsory requirement. Debian Jessie has long-term support until 30 June 2020, and GRASS 7.x should IMHO run on this system (with appropriate configure options).
follow-ups: 8 9 comment:7 by , 5 years ago
Replying to mmetz:
Replying to neteler:
Replying to mmetz:
Replying to mlennert:
+1 for backporting.
Done in relbr76 r74398
Just FYI: This broke the manual-building cronjobs on the grass.osgeo.org server but I fixed it by now actively disabling zstd in the configure step (Debian Jessie doesn't have zstd yet and #3486 = compile in a docker container is still open).
A good reason to not make zstd a compulsory requirement. Debian Jessie has long-term support until 30 June 2020, and GRASS 7.x should IMHO run on this system (with appropriate configure options).
I think there was some misunderstanding from my side. For maximum compatibility of GRASS raster data, the default compression should indeed be ZLIB, even if ZSTD is available. I made ZSTD the default if available in order to get it tested.
There was one issue reported where raster data compressed with ZSTD 1.3.6 could not be read with ZSTD 1.3.1, which remains a mystery.
What are the opinions on changing the default compression back to ZLIB even if ZSTD is available?
+1 from Moritz Lennert as I understand
0 from me
comment:9 by , 5 years ago
Replying to mmetz:
Replying to mmetz:
Replying to neteler:
Replying to mmetz:
Replying to mlennert:
+1 for backporting.
Done in relbr76 r74398
Just FYI: This broke the manual-building cronjobs on the grass.osgeo.org server but I fixed it by now actively disabling zstd in the configure step (Debian Jessie doesn't have zstd yet and #3486 = compile in a docker container is still open).
A good reason to not make zstd a compulsory requirement. Debian Jessie has long-term support until 30 June 2020, and GRASS 7.x should IMHO run on this system (with appropriate configure options).
I think there was some misunderstanding from my side. For maximum compatibility of GRASS raster data, the default compression should indeed be ZLIB, even if ZSTD is available. I made ZSTD the default if available in order to get it tested.
There was one issue reported where raster data compressed with ZSTD 1.3.6 could not be read with ZSTD 1.3.1, which remains a mystery.
What are the opinions on changing the default compression back to ZLIB even if ZSTD is available?
+1 from Moritz Lennert as I understand
Yes.
comment:11 by , 5 years ago
Replying to mmetz:
In 74426:
This change means that newly created raster maps are by default compressed with ZLIB, unless the environment variable GRASS_COMPRESSOR is set. Previously created raster maps compressed with ZSTD are readable as long as GRASS 7.6 is compiled with ZSTD.
Trunk still uses ZSTD as default if available.
follow-up: 13 comment:12 by , 5 years ago
Please note that this change in 7.6 contradicts our release announcement:
https://trac.osgeo.org/grass/wiki/Release/7.6.0-News#Librarychanges
"libraster: ZSTD is now the default compression if available"
follow-up: 14 comment:13 by , 5 years ago
Replying to neteler:
Please note that this change in 7.6 contradicts our release announcement:
https://trac.osgeo.org/grass/wiki/Release/7.6.0-News#Librarychanges
"libraster: ZSTD is now the default compression if available"
The release announcement for 7.6.2 should then include something like
The default compression has been changed back to ZLIB, even if ZSTD compression is available, because not all currently supported systems provide ZSTD with standard package management tools, and because problems were encountered regarding compatibility with different ZSTD 1.3.x versions.
comment:14 by , 5 years ago
Replying to mmetz:
Replying to neteler:
Please note that this change in 7.6 contradicts our release announcement:
https://trac.osgeo.org/grass/wiki/Release/7.6.0-News#Librarychanges
"libraster: ZSTD is now the default compression if available"
The release announcement for 7.6.2 should then include something like
The default compression has been changed back to ZLIB, even if ZSTD compression is available, because not all currently supported systems provide ZSTD with standard package management tools, and because problems were encountered regarding compatibility with different ZSTD 1.3.x versions.
Added in new draft: https://trac.osgeo.org/grass/wiki/Release/7.6.2-News
comment:15 by , 4 years ago
As a reference, the related relbranch76 change in GH: https://github.com/OSGeo/grass/commit/d3e60da04e0e5f7be151ee3a51deb8c6135e0527
comment:16 by , 4 years ago
For the record: in G7.8 and master ZSTD is the default compression:
- https://github.com/OSGeo/grass/blob/releasebranch_7_8/lib/gis/compress.c#L126
- https://github.com/OSGeo/grass/blob/master/lib/gis/compress.c#L126
As we didn't get any complaints with that, I suggest to keep it as is.
comment:17 by , 4 years ago
Problems reported between Oct 31, 2019 and now:
Generally speaking, changing the default in minor version became challenge for users sharing data (locations, packs) with other users (...choice of raster compression method...) since the requirement was version 7.6 compiled with newly introduced ZSTD. Expected issues between major versions, but not minor ones.
Even more problems were created by compatibility issues between different ZSTD versions.
follow-up: 19 comment:18 by , 4 years ago
comment:19 by , 4 years ago
Replying to annakrat:
Another reported problem: https://gis.stackexchange.com/questions/368823/r-grow-distance-with-r-mask-returns-errors-on-a-global-scale-but-not-with-a-sma
Yes, but reported with GRASS GIS 7.6 and ZSTD 1.3.2.0.
Currently ZSTD 1.4.4-1 is shipped in OSGeo4W).
Replying to mlennert:
yes
Done in trunk r74391. The default is now
--with-zstd=yes, you can disable ZSTD support with--with-zstd=no. To be backported I guess.