Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace piz with rc-zip #3012

Draft
wants to merge 4 commits into
base: latest
Choose a base branch
from
Draft

replace piz with rc-zip #3012

wants to merge 4 commits into from

Conversation

luizirber
Copy link
Member

@luizirber luizirber commented Feb 18, 2024

rc-zip can work on top of regular files (not memmap-ed files like piz).

It is also much easier to interact with.

Pending: more perf tests, but mem-wise I ran the command from #1909 (comment) and was seeing ~110MB max memory consumption.


Sadly, I think this will be draft for quite some time, it needs Rust 1.74 to compile =/
(it also broke wasm, need to do some splitting in src/core/src/storage.rs to be able to compile a subset...)

Copy link

codecov bot commented Feb 18, 2024

Codecov Report

Attention: Patch coverage is 55.00000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 86.87%. Comparing base (24ab89c) to head (37343ec).

Files Patch % Lines
src/core/src/storage.rs 55.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           latest    #3012      +/-   ##
==========================================
- Coverage   86.88%   86.87%   -0.02%     
==========================================
  Files         136      136              
  Lines       15543    15550       +7     
  Branches     2637     2637              
==========================================
+ Hits        13505    13509       +4     
- Misses       1736     1739       +3     
  Partials      302      302              
Flag Coverage Δ
hypothesis-py 25.44% <ø> (ø)
python 92.87% <ø> (ø)
rust 61.44% <55.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bluegenes
Copy link
Contributor

Sadly, I think this will be draft for quite some time, it needs Rust 1.74 to compile =/

What do we need in order to switch to 1.74? Is it just time, other packages, etc?

@luizirber
Copy link
Member Author

What do we need in order to switch to 1.74? Is it just time, other packages, etc?

We don't really have any rules, but I usually check

1.74 was released Nov 16th, which is fairly recent for distro standards/release cycles. 1.74 is also a pretty big release due to projections in opaque return types, which is exactly the feature that rc-zip uses and makes async programming much easier. Not that we do async, but many, many crates are going to MSRV to 1.74 because of that...

Maybe a good rule of thumb is checking what rust is available on the places we do packaging:

  • pypi - we can choose
  • conda-forge - 1.76
  • pyodide - nightly-2024-01-29?!?!?!?
  • spack - 1.75, about to add 1.76
  • nixpkgs (1.73 on latest release, 1.76 on rolling)

But these are fairly fast moving places =]

More links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants