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.

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

c-blosc upgrade 1.18.1 -> 1.21.0 #283

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 3 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
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