redis-py version attribute should be decoupled from the redis module

[ashwani99]

Following the conversation from #1625 (comment) looks like importing redis module prior to installation in setup.py for version attribute is not ideal.

Currently there are two places where module version is required.

• setup.py for module installation
• redis/__init__.py for module level __version__ attribute

One way to fix this is to maintain a version.py file in top level directory and using that as source of truth in both the above places.

@chayim @hartwork What do you think? I can create a PR for this :)

[hartwork]

looks like importing redis module prior to installation in setup.py for version attribute is not ideal.

When setup.py is being run, the current working directory is the first entry in sys.path so that Python will find the local redis package, installed or not. To see it in action, this diff may help:

diff --git a/setup.py b/setup.py
index 58d753f..ac8fc79 100644
--- a/setup.py
+++ b/setup.py
@@ -1,6 +1,10 @@
 #!/usr/bin/env python
 from setuptools import find_packages, setup
 
+import sys
+print('\nPYTHONPATH:', file=sys.stderr)
+print('\n'.join(f'- {p!r}' for p in sys.path), file=sys.stderr)
+
 import redis
 
 setup(

One way to fix this is to maintain a version.py file in top level directory and using that as source of truth in both the above places.

version.py sounds troublesome, since (1) we must not install it and (2) redis.__version__ would need to come somewhere without that file being present.

Personally, I think touching this may make things worse. redis.__init__ is likely doing to much but it cannot do less now, because of keeping API backwards compatability. Reading from a plain text file would mean that file needs to be read on every import, as well. Would that really help? I'm not sure.

[chayim]

These are all of the things that using poetry (#1658) solves, for our package build and management - though I'm sure that will introduce new items to track down.

Reading versions from a file will at the very least make things slower and should be avoided.

As we're not (currently) going to solve these items with poetry - my goal would still be to move to a pyproject.toml like solution, and move the ball forward on PEP 516 / PEP 517 changes.

@ashwani99 Does tackling that interest you?

[WisdomPill]

We are also seeing this issue in django-redis CI, when using master.
Here are the logs.
Is there something that I can do to help?

[chayim]

Nothing here - @ashwani99 did a fantastic job and his PR triggers highlights part of this issue, as discussed. Assigned to myself.

[ashwani99]

Thanks @chayim for the encouragement. 👍🏼

Based on my understanding, we are trying to migrate from setup.py to a pyproject.toml + setup.cfg based solution as a first step while moving to PEP 516 / PEP 517 changes. But with that too, I believe we will face this issue while persist redis module will be imported. Please correct me if I am wrong here.

Reading versions from a file will at the very least make things slower and should be avoided.

Also, I am curious to learn about this limitation of reading version from a file. Would be great if you share some thoughts on this.
I found this note that discusses different approaches to this problem and their trade-offs.

[chayim]

I think this bug should be split into two disctinct items. First off - there's the decoupling issue - then there's the desire to move to a pyproject.toml (#1658) solution.

Opening a file is expensive. It would happen on a per-import of the module, which in turn adds unnecessary IO just to fetch a module version. Splitting data into a metadata.py file and loading - while fine, is adding more separation, that violates pyproject.toml's all in one approach.

Using importliib.metadata works well - except when the package isn't installed, yielding the same problem. Now - we can solve this by forcing a default version (say the common '99.99.99' or '0.0.0') but that's really just asking for a different issue. Personally, I think this is the best move, and the one I have ready.

In this scenario:

1. The package version is in a single place: setup.py -> future pyproject.toml
2. Anyone doing a pip install directly from the repo, will receive the proper version
3. Anyone doing a pip install via pypi receives the correct version
4. Someone working locally has a clear way to differentiate, should they need to in that the version is 99.99.99/0.0.0

The downside to the approach above is:

1. It requires installing importlib-metadata on python < 3.8. To be fair, other solutions require setuptools, so I view this as a relative non-issue.
2. For someone working locally, their version is a constant 99.99.99 rather than the currently deployed version. But, I would argue for the extremely rare occasion where the version value itself matters, this can be removed.
3. In the future, we have to remove importlib-metadata

[chayim]

@hartwork if you could tell me what you think about the approach outlined above and PR #1791 I'd love the feedback! For the purposes of extended testing,

@ashwani99 I'm going to merge your PR and mine into another branch - just to 100% validate this catches that. EIther way, these issues are separate, and this issue would only enter master after your #1791 PR.

[WisdomPill]

@chayim I saw in django-redis from the people contributing before my step in,
that there is an elegant way to handle the issue.

Using setup tools which everybody should (already ?) have installed,
that it is using setup.cfg where you can read attributes of files.

In fact here you can see how to configure package metadata.
And here you can see how it is used in django-redis

[chayim]

So - you're not guaranteed to have setuptools installed as opposed to importlib - case in point I'm one of the lucky people who doesn't in all of my environments, given my tools and machines. But let's ignore me.

I am 100% in favour of moving to pyproject.toml - this isn't the PR that'll do that. Any objections (please voice here).

[WisdomPill]

I admit, I am not an expert in this field,
mine was just an example as a maintainer of a library based on redis.

I started reading about pyproject.toml it seems like a replacement of setup.py and so on.

So for me pyproject.toml is more than okay.
I will read further and try to keep up with the developments over here, as I see there is a lot to learn.

[chayim]

You're not the only one, there's always a tonne to learn :). I love it!

[chayim]

FYI, I'm proposing merging this and getting a version change for 4.1.0 ready as we're almost there. This passed in both the merged test PR I gummied together (thanks @ashwani99 ) and standalone in the current world (which doesn't test anything). I'd prefer to have the CI PR (#1790) merged first as it would provide even better testing - but it'll at least unblock your issue.

@WisdomPill sound fair?

[WisdomPill]

yep, I am always in favor of more tests

[chayim]

Amazeballs. Okay - this merges first, then #1790. I validated their interaction in a PR I'm wiping (#1795). Thank you everyone. This was awesome collaboration!