Skip to content
This repository has been archived by the owner on Feb 21, 2023. It is now read-only.

Make hiredis an optional(extra) requirement #663

Closed
hallazzang opened this issue Nov 15, 2019 · 9 comments
Closed

Make hiredis an optional(extra) requirement #663

hallazzang opened this issue Nov 15, 2019 · 9 comments
Labels

Comments

@hallazzang
Copy link

hallazzang commented Nov 15, 2019

I know that there were already some discussions about it in #197, #317 and #394.

But please take a look at pypa/pip#7339. Because aioredis has forced hiredis dependency, I should rely on workaround methods.
If aioredis had hiredis as an optional(extra) requirement, then my requirements.txt would be like this:

aioredis==1.3.0

and others who want better performance, would write their requirements.txt like this:

aioredis[hiredis]==1.3.0

I think it's very clear solution, and is already adopted by another aio-libs family, aiohttp.
Make hiredis an optional requirement, and recommend to use pip install aioredis[hiredis] for users who want speedups, how do you think about it?

@popravich
Copy link
Contributor

Your containers use case sounds reasonable, I will consider this.

@popravich popravich added the task label Nov 15, 2019
@hallazzang
Copy link
Author

Thanks. Just let me know what you think. :)

@asvetlov
Copy link
Contributor

asvetlov commented Nov 19, 2019

I guess hiredis should be opt-out, not opt-in (sorry for the reverse in the unedited version of the comment).
Otherwise, people will use a slower aioredis version without good reason.
pip has no setting for disable something. Master versions of aiohttp/yarl/multidict adopts environment variables to disable C extensions, e.g. AIOHTTP_NO_EXTENSIONS.

See corresponding setup.py to revel this magic.

@popravich
Copy link
Contributor

Thanks, Andrew, I have forgotten this trick. Will look into this.

@hallazzang
Copy link
Author

@asvetlov You mean hiredis should be opt-out, not opt-in(seems flipped), so the current behavior is the preferred way. Did I understand you correctly?

I found those *_NO_EXTENSIONS settings in aiohttp/yarl/multidict setup.py but it seems that those settings control whether to compile their own C extension, not their dependencies'.
How does the trick apply to aioredis in my case?

@asvetlov
Copy link
Contributor

Sorry for flipping, I did mean that hiredis is opt-out.

Let's go to the roots: why do you want to skip hiredis installation?
Do you think that the vast majority want to use the slower redis protocol parser?

@hallazzang
Copy link
Author

Nope, I completely agree with your opinion, faster implementation should be preferred most of the time. But in some cases, pure python parser would be just enough. I want to have hiredis as an opt-in in my container use case, because installing build-base package to build hiredis wheel makes my resulting docker image too big.

Here's the comparison table:

REPOSITORY                      TAG                 IMAGE ID            CREATED             SIZE
alpine-test                     latest              b04b5721c738        2 seconds ago       274MB
python                          3.8-alpine          204216b3821e        4 weeks ago         111MB

Dockerfile for that alpine-test looks like:

FROM python:3.8-alpine
RUN apk add --update build-base

So just to build hiredis wheel, I get >150MB bigger image. To disable building hiredis, I have to deal with workarounds since pip doesn't have an option to skip dependency installation in requirements.txt. If hiredis was an optional requirement, I would achieve my goal by just doing so:

aioredis==1.3.0

Instead of:

aioredis[hiredis]==1.3.0

in my requirements.txt.

Additionally, here's how aiohttp does now: https://aiohttp.readthedocs.io/en/stable/#library-installation
If you need speedup, then specify [speedup] extra requirement. Why can't it be applied to aioredis?

@asvetlov
Copy link
Contributor

I understand your need, alpine linux is a problem for C Extensions building.
pip doesn't support optional excluding, only optional including.
aioredis[nohiredis]==1.3.0 cannot be implemented.

Perhaps you are confused by aiohttp[speedup] meaning. It is for aiodns, brotlipy and cchardet -- all are not the major speedups actually. Moreover, aiodns can show a liitle different behavior than builtin DNS resolver. That's why these requirements are optional.
In contrast, fast http parser and other real speedups are opt-out. E.g., to disable http-parser you need to use AIOHTTP_NO_EXTENSIONS=1 pip install aiohttp for the master version.

Previously, if aiohttp was unable to get the compilation toolset it falls to pure-python version without speedups silently. This doesn't work well: people get the aiohttp slowdown without any notice.
Making the compilation opt-out avoids this misunderstanding.
I believe the same logic is legitimate for aioredis as well.

@hallazzang
Copy link
Author

Yes, I misunderstood the meaning of speedup.

I get your point here, closing this issue. I'll keep looking forward to changes in pip.

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

No branches or pull requests

3 participants