-
Notifications
You must be signed in to change notification settings - Fork 2.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
setup.py: Support older setuptools (<=20.1.1) #2623
setup.py: Support older setuptools (<=20.1.1) #2623
Conversation
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.
This seems reasonable to me.
In the in-line comment, could you add the specific version of setuptools which added that functionality.
I imagine at some point, we will want to stop supporting old versions of setuptools etc.
6274aae
to
ff095df
Compare
The `python_version` conditional was added fairly recently (<2 years). We've run into a few cases where it would be very convenient for luigi to support older setuptools (e.g. managed environments where it may be difficult or not possible to upgrade).
ff095df
to
094abe3
Compare
@dlstadther 👍 done and thanks for the review! |
Final verification from, does this code work for you in production? |
We haven't used in production (a bit tricky short of depending on the luigi git master), but I repro'd exactly the same error and verified this fix using the lower setuptools version locally. |
Gentle bump here. (It seems the failing codecov check may be spurious) |
Ah, I hadn't seen #2608, thanks for linking! I can see both sides as well. In this case, I see the cost as quite low, which makes it worth it. To provide a bit more detail, the managed environment I'm referring to is the Google Cloud AI/ML environment. Based on their version list they're using the Ubuntu-provided python-setuptools, which for trusty (14.04LTS) is 3.3 and for xenial (16.04LTS) is 20.7. That Google Cloud ML version list is underspecified as they don't specify which Ubuntu release, but based on the behavior we're seeing it must be trusty or older. As for non-managed environments, I might be on the other side of the argument! |
@brianmartin Totally understand. |
Hey @honnix, are you saying you approve? cc @dlstadther |
Yeah. Sorry I was not clear about it. |
The
python_version
conditional was added fairly recently (<2 years).We've run into a few cases where it would be very convenient for luigi
to support older setuptools (e.g. managed environments where it may be
difficult or not possible to upgrade).
The
python_version
conditional was introduced in luigi2.8.x
. For nowwe're pinning to
<2.8.x
as a workaround.