-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add the optional compression libs to extras_require #2123
Conversation
As noted in https://kafka-python.readthedocs.io/en/master/install.html these are some additional optional libs that we should be specifying in `extras_require` so that callers can specify them if desired.
extras_require={ | ||
"crc32c": ["crc32c"], | ||
"lz4": ["lz4"], | ||
"snappy": ["python-snappy"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if we should take the "snappy"
namespace or use "python-snappy"
so as to disambiguate between different implementations if we later have support for a pure-python vs c-based lib... this applies to the "zstd"
entry as well...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dpkp @tvoinarovskyi any opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think dropping the python-
prefix is fine here.
extras_require={ | ||
"crc32c": ["crc32c"], | ||
"lz4": ["lz4"], | ||
"snappy": ["python-snappy"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think dropping the python-
prefix is fine here.
"crc32c": ["crc32c"], | ||
"lz4": ["lz4"], | ||
"snappy": ["python-snappy"], | ||
"zstd": ["python-zstandard"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually correct? It looks like the PyPI package is just zstandard
. When testing installing only this package locally, I get an error:
$ pip install python-zstandard
ERROR: Could not find a version that satisfies the requirement python-zstandard (from versions: none)
ERROR: No matching distribution found for python-zstandard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing the same error after cloning repository and trying to install with the extra locally.
$ pip install -e .[zstd]
Obtaining file:///Users/anton/projects/kafka-python
Preparing metadata (setup.py) ... done
ERROR: Could not find a version that satisfies the requirement python-zstandard; extra == "zstd" (from kafka-python[zstd]) (from versions: none)
ERROR: No matching distribution found for python-zstandard; extra == "zstd"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd be better off opening a new issue, as comments on old PR's tend to get lost.
As noted in https://kafka-python.readthedocs.io/en/master/install.html these are some additional optional libs that we should be specifying in
extras_require
so that callers can specify them if desired.Fix #2122.
This change is