Add zstandard - #4792
Conversation
Sorry, something went wrong.
|
Looks like the build timed out before it could build zstandard. @ryanking13 or @hoodmane, could you please re-trigger the relevant CircleCI runs? Thanks! zstandard builds in ~16 seconds locally, FYI. |
Sorry, something went wrong.
|
context deadline exceeded means that too much time passed without anyone writing to stdout. We could add to pyodide build-recipes in CI a pulse that occasionally just writes a keepalive message so that this won't happen. |
Sorry, something went wrong.
|
I pushed fixes in 71c7e72 to resolve the two test failures, and all tests are now passing. Please let me know if I should add any more tests. Otherwise, I think this is ready for review/merging. |
Sorry, something went wrong.
|
I sent you an email, did you receive it? (No reply needed, just want confirmation that I got the right email for you.) |
Sorry, something went wrong.
|
Yes, received it, @hoodmane (I was going to reply later in the day today!). |
Sorry, something went wrong.
hoodmane
left a comment
There was a problem hiding this comment.
Thanks @agriyakhetarpal. Tests look good. One comment.
Sorry, something went wrong.
Co-Authored-By: Hood Chatham <roberthoodchatham@gmail.com>
|
Thanks for the feedback! Addressed in a7487a8 by adding a pytest fixture and I additionally used autouse=True so that I do not need to apply it across all the functions. |
Sorry, something went wrong.
Co-Authored-By: Hood Chatham <roberthoodchatham@gmail.com>
|
In 8ce3314, I added zstd.backend so that we can avoid using the environment variable and instead set the backend programmatically. This should run the tests with both backends. |
Sorry, something went wrong.
|
In c66aaaf, I succumbed to parameterising each function directly... this shouldn't fail on anything and I thought it would be okay since there are not a lot of tests. |
Sorry, something went wrong.
|
Builds with both cext and cffi are passing on CircleCI in the no-numpy-dependents jobs, @hoodmane. Please let me know if anything else is needed to be done. Ready for review/merging, again. |
Sorry, something went wrong.
There was a problem hiding this comment.
@agriyakhetarpal Yeah this commit definitely wasn't going to work. The request fixture is not pickleable. You'd still need the pair of functions so that you can access request.param in the host runtime and then pickle just that string.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh, but I see it was wrong before this commit too because you put @run_in_pyodide on the fixture. If you want to avoid manually parametrizing each test, the correct code is:
@run_in_pyodide(packages=["zstandard"])
def set_zstd_backend(selenium, zstd_backend):
import zstandard as zstd
zstd.backend = request.param
@pytest.fixture(params=["cext", "cffi"], autouse=True)
def zstd_backend(selenium, request):
# Runs in host, request not pickleable so we can't send it to Pyodide.
# Look up `request.param` which is a string hence pickleable and send that to Pyodide
set_zstd_backend(selenium, request.param)
Sorry, something went wrong.
There was a problem hiding this comment.
Oh, I didn't realise that – I thought the reason why it didn't work before was because I didn't add a yield statement for set_zstd_backend. Do we need to revert this PR? I can add the fix you suggested.
Sorry, something went wrong.
There was a problem hiding this comment.
Nah you can make a follow-up that does that. Label it nfc for nonfunctional change.
Thanks!
Sorry, something went wrong.
Description
This PR adds the zstandard Python package with the C and CFFI backends (the Rust backend is not compiled as of now, but can be a future improvement). zstandard is a required dependency in the upcoming version 3 release for Zarr.
Closes #4788
Checklists
Additional context
xref: zarr-developers/zarr-python#1903