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

fix zstd/zstandard import error #393

Merged
merged 4 commits into from Jan 20, 2021

Conversation

imkumarg
Copy link
Contributor

Description:

The _zmsgpack.py (kartothek.core.cube) uses zstd module for the compressor and decompressor technique. I have updated the requirements.txt with the module name.

Copy link
Collaborator

@fjetter fjetter left a comment

Choose a reason for hiding this comment

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

There are two packages for the zstd compression lib in python

https://github.com/indygreg/python-zstandard

and

https://github.com/sergey-dryabzhinsky/python-zstd

and we're using the first one. Installing both can have non trivial side effects.

The release notes of zstandard==0.15.0 mention already that some modules are no longer available in the zstandard module any longer but rather in zstd therefore I would like to change the import in our module accordingly instead of adding a redundant dependency

See also #392

@imkumarg
Copy link
Contributor Author

So instead, I have to modify the usage of zstd accordingly in the codebase.

@hoffmann
Copy link
Collaborator

@fjetter is it an option to downpin zstandard in this tests until #392 is fixed?

@lr4d
Copy link
Collaborator

lr4d commented Jan 19, 2021

From the zstandard docs

There will be some backwards incompatible changes before 1.0, probably in the 0.9 release. This may involve renaming the main module from zstd to zstandard and renaming various types and methods. Pin the package version to prevent unwanted breakage when this change occurs!

@lr4d
Copy link
Collaborator

lr4d commented Jan 19, 2021

I think the easiest way forward here is using this commit d8f4c22 (#392)
Otherwise, we can also downpin zstandard.

@imkumarg
Copy link
Contributor Author

I think the easiest way forward here is using this commit d8f4c22 (#392)
Otherwise, we can also downpin zstandard.

Yes I think too, we can use this commit [d8f4c22 (#392)]

@fjetter
Copy link
Collaborator

fjetter commented Jan 19, 2021

Haven't tried downpinning and I do not know why tests do not detect this. However, I do not feel comfortable with installing both zstandard libs. I think reusing d8f4c22 is the proper way to go. The other PR is also supposed to deal with the doc build issues since the doc builds are also failing for some other reason

@lr4d
Copy link
Collaborator

lr4d commented Jan 19, 2021

I do not know why tests do not detect this

@imkumarg Could you include a test for this so that the test would fail before applying d8f4c22 (#392) and would pass afterwards?

@lr4d
Copy link
Collaborator

lr4d commented Jan 20, 2021

Looks good. Please add a changelog entry

@imkumarg
Copy link
Contributor Author

Looks good. Please add a changelog entry

Sure I will make the changes.

Copy link

@dhumil-agarwal-by dhumil-agarwal-by left a comment

Choose a reason for hiding this comment

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

Looks good.

@lr4d
Copy link
Collaborator

lr4d commented Jan 20, 2021

We can merge this now to fix the issue downstream, but I would still like to see a test for this so that we don't run into this or a similar issue next time.

@lr4d
Copy link
Collaborator

lr4d commented Jan 20, 2021

Ah, I just figured out why we weren't seeing this in our CI: zstandard is available up to 0.15.1 on PyPI but only up to 0.14.1 on conda-forge (what we're using in our CI).
So, the only way we could "test" this would have been to have a pip env in our tests

@lr4d lr4d changed the title Updated requirements.txt with zstd module fix zstd/zstandard import error Jan 20, 2021
@lr4d lr4d merged commit 6fa807c into JDASoftwareGroup:master Jan 20, 2021
@xhochy
Copy link
Contributor

xhochy commented Jan 22, 2021

The conda package has been updated, would be nice to get people fix this when I don't have the time ;)

@xhochy
Copy link
Contributor

xhochy commented Jan 25, 2021

Can we have a release with this fix?

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

Successfully merging this pull request may close these issues.

None yet

6 participants