Skip to content

c-blosc upgrade 1.18.1 -> 1.21.0 #283

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Aug 18, 2021
Merged

c-blosc upgrade 1.18.1 -> 1.21.0 #283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
64dd243
c-blosc upgrade to 1.18.1 -> 1.21.0
Jul 28, 2021
b76a21a
change from master to v1.21.0
Jul 29, 2021
6df653b
remove HAVE_SNAPPY macro to fix c-blosc build
Jul 29, 2021
d425e41
Merge branch 'master' into issue_269_c_blosc_upgrade
joshmoore Aug 18, 2021
e692219
Disable snappy tests
joshmoore Aug 18, 2021
a84a166
Remove snappy fixture
joshmoore Aug 18, 2021
fbe4d36
Enter blank into codecs tests to preserve indexes
joshmoore Aug 18, 2021
5bbb865
Revert "Remove snappy fixture"
joshmoore Aug 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion c-blosc
Open in desktop
Submodule c-blosc updated 164 files
3 changes: 3 additions & 0 deletions numcodecs/tests/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,9 @@ def check_backwards_compatibility(codec_id, arrays, codecs, precision=None, pref

for j, codec in enumerate(codecs):

if codec is None:
pytest.skip("codec has been removed")

# setup a directory to hold encoded data
codec_dir = os.path.join(fixture_dir, 'codec.{:02d}'.format(j))
if not os.path.exists(codec_dir): # pragma: no cover
Expand Down
12 changes: 10 additions & 2 deletions numcodecs/tests/test_blosc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
Blosc(cname='zlib', clevel=1, shuffle=0),
Blosc(cname='zstd', clevel=1, shuffle=1),
Blosc(cname='blosclz', clevel=1, shuffle=2),
Blosc(cname='snappy', clevel=1, shuffle=2),
None, # was snappy
Blosc(shuffle=Blosc.SHUFFLE, blocksize=0),
Blosc(shuffle=Blosc.SHUFFLE, blocksize=2**8),
Blosc(cname='lz4', clevel=1, shuffle=Blosc.NOSHUFFLE, blocksize=2**8),
Expand All @@ -61,6 +61,11 @@
]


def _skip_null(codec):
if codec is None:
pytest.skip("codec has been removed")


@pytest.fixture(scope='module', params=[True, False, None])
def use_threads(request):
return request.param
Expand All @@ -69,6 +74,7 @@ def use_threads(request):
@pytest.mark.parametrize('array', arrays)
@pytest.mark.parametrize('codec', codecs)
def test_encode_decode(array, codec):
_skip_null(codec)
check_encode_decode(array, codec)


Expand All @@ -77,6 +83,7 @@ def test_encode_decode(array, codec):
else pytest.param(x, marks=[pytest.mark.xfail])
for x in arrays])
def test_partial_decode(codec, array):
_skip_null(codec)
check_encode_decode_partial(array, codec)


Expand Down Expand Up @@ -143,7 +150,6 @@ def test_compress_complib(use_threads):
'lz4': 'LZ4',
'lz4hc': 'LZ4',
'blosclz': 'BloscLZ',
'snappy': 'Snappy',
'zlib': 'Zlib',
'zstd': 'Zstd',
}
Expand Down Expand Up @@ -257,6 +263,7 @@ def test_err_encode_object_buffer():

def test_decompression_error_handling():
for codec in codecs:
_skip_null(codec)
with pytest.raises(RuntimeError):
codec.decode(bytearray())
with pytest.raises(RuntimeError):
Expand All @@ -265,5 +272,6 @@ def test_decompression_error_handling():

def test_max_buffer_size():
for codec in codecs:
_skip_null(codec)
assert codec.max_buffer_size == 2**31 - 1
check_max_buffer_size(codec)
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def blosc_extension():
include_dirs += [d for d in glob('c-blosc/internal-complibs/*/*/*')
if os.path.isdir(d)]
define_macros += [('HAVE_LZ4', 1),
('HAVE_SNAPPY', 1),
# ('HAVE_SNAPPY', 1),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don't think we want to skip using Snappy. Did this move somewhere else? Is there a path we need to update?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...looks like they dropped Snappy

Blosc/c-blosc#295

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# ('HAVE_SNAPPY', 1),
('HAVE_SNAPPY', 0), # Blosc 1.19.0+ dropped Snappy

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make this change and see if installation still succeeds

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming that it did not work with the above change #283 (comment) . I get the same error #283 (comment)

Copy link
Member

@jakirkham jakirkham Jul 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, that's with 0 and not 1, correct? If so, that's really weird

Could you please try None as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep set to 0. Yeah I can try None too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep same issue with None. I can dig into how the macro passes information to the c-blosc build process

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh it seems like @joshmoore started on this very same PR but stopped because of a call #259

Do we want to proceed with this change or go down another path?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kindjacket: explaining the #259 / #274 decisions -- we discussed that on that call that spending our time on the pure python implementation was probably a bigger win. However, as you can see in my #274, that turned out to be more work than (I) expected.

At this point, I'm inclined to say we temporarily drop snappy in order to get M1 fixed and then re-evaluate once that's working with the options being continue on the #274 path, or to first work on getting snappy working with c-block 1.21+.

@zarr-developers/core-devs : any objections?

('HAVE_ZLIB', 1),
('HAVE_ZSTD', 1)]
# define_macros += [('CYTHON_TRACE', '1')]
Expand Down