search

conda-forge / conda-forge-ci-setup-feedstock

A conda-smithy repository for conda-forge-ci-setup.
BSD 3-Clause "New" or "Revised" License
13 stars 51 forks source link

Increase zstd_compression_level to 19 #217

Closed mbargull closed 1 year ago

mbargull commented 1 year ago

Checklist

Copy/paste a comment of mine from https://github.com/conda/conda-package-handling/issues/167 :

I highly recommend going for -19 at max. Not just because of the resources during compression but also for the decompression on the users' side. For decompression you'd need some sub 10M base memory plus the window size (which is capped by the input size, i.e., it's most relevant for big packages), e.g.,

* level 19: window size: `2**23=8M`

* level 20: window size: `2**25=32M`

* level 21: window size: `2**26=64M`

* level 22: window size: `2**27=128M`

For compression the higher levels additionally use bigger search tables which is why the memory increases more. See https://github.com/facebook/zstd/blob/v1.5.2/lib/compress/clevels.h#L44-L50 for the actual (logarithmic) values used for each level and https://manpages.ubuntu.com/manpages/focal/man1/zstd.1.html for a short description of those.

You get slightly higher resource usage for -19 compared to -18 during compression (and the same low max x+8M for decompression). I'd go for one of those and try -19 first and see if we encounter cases that demand lowering it to -18 if needed.

refs:

conda-forge-linter commented 1 year ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

mbargull commented 1 year ago 
zstd.ZstdError: zstd compress error: Allocation error : not enough memory

hehe

jakirkham commented 1 year ago

For the viewers at home, the error above will be fixed by PR ( https://github.com/conda-forge/conda-build-feedstock/pull/190 )

jakirkham commented 1 year ago

Restarting the failed CI builds since conda-build packages are up

beckermr commented 1 year ago

Technically a decrease since due to the conda build bug we were at 22.

github-actions[bot] commented 1 year ago

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

Thus the PR was passing and merged! Have a great day!

jakirkham commented 1 year ago

Failure here is due to this issue ( https://github.com/conda-forge/conda-build-feedstock/pull/190#issuecomment-1332989764 )

Updating boa's conda-build pin will fix it ( https://github.com/conda-forge/conda-build-feedstock/pull/190#issuecomment-1333000486 ). Filed issue ( https://github.com/conda-forge/boa-feedstock/issues/58 )

mbargull commented 1 year ago

Technically a decrease since due to the conda build bug we were at 22.

I didn't mention the effective level :P. (But a valid point ^_^ .)

mbargull commented 1 year ago

@conda-forge-admin, please restart ci

mbargull commented 1 year ago

Over 50 jobs for this PR and not a single one failed due OOM issue -- looks like the newest conda-build+boa builds work fine and this was built with zstd_compression_level: 16 :tada:.

beckermr commented 1 year ago

Actually built with 19. This feedstock installs the local copy of ci setup and builds with that. Helps for testing and bootstrapping.

jakirkham commented 1 year ago

Awesome thanks Marcel! 🙏

This feedstock installs the local copy of ci setup and builds with that. Helps for testing and bootstrapping.

Definitely, this was a motivating design principle early on. Particularly as things broke more often. We needed an easy way to get out of a jam and bootstrapping helps with that

mbargull commented 1 year ago

Actually built with 19. This feedstock installs the local copy of ci setup and builds with that. Helps for testing and bootstrapping.

ooh, I'm always happy to be corrected, plus getting bits and pieces of information around the infra from you in comments like that one :)

beckermr commented 1 year ago

Yes thank you @mbargull!