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

c-blosc upgrade 1.18.1 -> 1.21.0 #283

Merged

Conversation

olly-writes-code
Copy link

c-blosc submodule upgrade 1.18.1 -> 1.21.0 to fix issue #269

TODO:

  • Unit tests and/or doctests in docstrings
  • tox -e py39 passes locally
  • Docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • tox -e docs passes locally
  • GitHub Actions CI passes
  • Test coverage to 100% (Coveralls passes)

@olly-writes-code
Copy link
Author

When trying to run on my local machine I got the following error

      c-blosc/blosc/blosc.c:31:12: fatal error: 'snappy-c.h' file not found
        #include "snappy-c.h"
                 ^~~~~~~~~~~~ `

@olly-writes-code
Copy link
Author

This fixes the file not found snappy issue when compiling c-blosc 6df653b

IDK - how this will play with other machines.

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

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?

@joshmoore
Copy link
Member

@kindjacket : looks like we have a 👍 from @jakirkham as well (#268), any thoughts on getting this ready to merge?

@pep8speaks
Copy link

pep8speaks commented Aug 18, 2021

Hello @kindjacket! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-08-18 09:31:52 UTC

@joshmoore joshmoore force-pushed the issue_269_c_blosc_upgrade branch from a33735c to cacaf05 Compare August 18, 2021 08:07
@joshmoore joshmoore force-pushed the issue_269_c_blosc_upgrade branch from cacaf05 to fbe4d36 Compare August 18, 2021 08:12
@joshmoore
Copy link
Member

joshmoore commented Aug 18, 2021

whew Starting to look greenish.

Edit: CondaHTTPError: HTTP 502 BAD GATEWAY for url <https://conda.anaconda.org/conda-forge/noarch/current_repodata.json> looks like those that aren't may be unrelated. I'm trying to restart them into greenness.

@jakirkham
Copy link
Member

Actually can we leave the fixture data (instead of deleting it)? We will want this when we add back snappy (somehow 🤔)

@joshmoore
Copy link
Member

Actually can we leave the fixture data (instead of deleting it)? We will want this when we add back snappy

Good point. I didn't realize at first that it was an ordered list etc. etc. so I thought deletion was the appropriate way forward. I'll resurrect.

@joshmoore
Copy link
Member

joshmoore commented Aug 18, 2021

Any thoughts on version number with the drop of snappy? 0.9.0 I assume.

@joshmoore
Copy link
Member

I'm taking that at least as no objections.

@joshmoore joshmoore merged commit 4ea793d into zarr-developers:master Aug 18, 2021
@jakirkham jakirkham mentioned this pull request Aug 18, 2021
@jakirkham
Copy link
Member

Thanks Josh! 😄

Sorry had to drop 😴 Though happy to trust your judgement on versions 🙂

Raised issue ( #285 ) about readding Snappy

@joshmoore
Copy link
Member

np.

A heads up that while testing zarr-developers/zarr-python#773 I'm pretty sure that this PR changed the checksum of chunks in the zarr_implementations test.

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.

4 participants