Regain Python 2.6 support #206

adamjstewart · 2017-12-29T17:25:14Z

The Spack package manager uses distro to determine which Linux distribution we run on. Spack still supports Python 2.6. I noticed that official 2.6 support was dropped in #195. This pull request should fix the only problems we noticed with Python 2.6.

adamjstewart · 2017-12-29T18:23:31Z

I'm having trouble getting _lsb_release_info working. Let me know if you have any ideas.

sethmlarson

If we're adding this you would want to run tests against 2.6 in Travis as well. Should probably modify .travis.yml. Also not sure how I feel about adding support for an EOL language but that's the call of @nir0s.

sethmlarson · 2017-12-29T23:20:44Z

distro.py

+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE)
+        stdout, stderr = process.communicate()
+        stdout, stderr = stdout.decode('utf-8'), stderr.decode('utf-8')


Let's continue to use sys.getfilesystemencoding() instead of assuming utf-8 here. Also remove the tuple pack and unpack. Doesn't make the code any more readable IMO.

sethmlarson · 2017-12-29T23:22:20Z

distro.py

-        content = stdout.decode(sys.getfilesystemencoding()).splitlines()
-        return self._parse_lsb_release_content(content)
+        else:
+            if sys.version_info[:2] >= (3, 5):


Grab version_info = sys.version_info[:2] once and compare it instead of grabbing it fresh each time. Also now this function raises errors where it would previously swallow them? That probably will have unintended side-effects?

sethmlarson · 2017-12-29T23:25:20Z

distro.py

@@ -925,16 +925,26 @@ def _lsb_release_info(self):
        Returns:
            A dictionary containing all information items.
        """
-        if not self.include_lsb:
+        cmd = ['lsb_release', '-a']
+        process = subprocess.Popen(


This doesn't seem to pipe to devnull anymore and I can't find anywhere where stderr is used meaningfully unless we get an error. Can we move the processing of stderr until that one error branch? (Or better yet: pipe it to /dev/null and not process it at all!)

sethmlarson · 2017-12-29T23:26:00Z

distro.py

@@ -518,7 +518,7 @@ def __init__(self, f):
        self._f = f

    def __get__(self, obj, owner):
-        assert obj is not None, 'call {} on an instance'.format(self._fname)
+        assert obj is not None, 'call {0} on an instance'.format(self._fname)


Genuinely curious, does 2.6 not support assuming format indexes?

It doesn't. It's a 2.7 and up thing.

Correct. See https://docs.python.org/2/library/string.html#format-string-syntax

Changed in version 2.7: The positional argument specifiers can be omitted, so '{} {}' is equivalent to '{0} {1}'.

Thanks for verifying this!

adamjstewart · 2017-12-30T19:50:57Z

@SethMichaelLarson Thanks for the review! I'll get started on implementing these suggestions. For the record, I tried a lot of different tactics that were completely unrelated, so if you scroll back through previous commits and find one you like better, just let me know. The last commit I settled on was a simple revert of @asottile's #197; perhaps he can comment on why #197 was necessary and if there is a better way of doing this that doesn't involve subprocess.check_output. I'm just surprised that reverting that commit completely breaks the unit tests.

@nir0s Let me know if you want to fully revert #195 or just silently support 2.6 but not advertise/guarantee it. Obviously, we would certainly appreciate 2.6 support, but I can understand if you don't want to support it.

asottile · 2017-12-30T20:00:29Z

I switched to check_output because I knew 2.6 wasn't supported. The support was pretty explicitly removed in #195

I was knocking off performance issues and the shell overhead was a quick and easy win.

While I don't have any say on this project, I'd like to voice my 👎 on this PR. python2.6 has been end of lifed for over four years -- continuing to maintain python2.6 support (even python 2 support) is quite stifling to progress. The entire packaging infrastructure has also moved on (pip, setuptools, wheel).

Instead of burdening upstreams with support of long-since-dead python versions, perhaps vendoring is a better solution? distro is after all a single file without dependencies (well, unless you're on python2.6 and you need argparse!).

asottile · 2017-12-30T20:04:55Z

As for check_output, there's a python2.6 equivalent way, but it's slightly more verbose. Here's an (untested but probably works) example:

def check_output(cmd, **kwargs):
    proc = subproces.Popen(
        cmd,
        stdout=subprocess.PIPE,
        **kwargs
    )
    out, _ = proc.communicate()[0]
    if proc.returncode:
       raise AssertionError('{0} exited {1}'.format(cmd, proc.returncode))
    return out

asottile · 2017-12-30T20:06:32Z

Actually, check_output is implemented in pure python in python2.7 -- if you really wanted identical implementation you could drop that in: https://github.com/python/cpython/blob/3ceaed0dce81fd881bbaf2dbdbe827d9681887da/Lib/subprocess.py#L194-L224

adamjstewart · 2017-12-30T20:16:26Z

@asottile I understand your concerns. We are actually vendoring distro at the moment (as well as argparse), however we don't want to be forever stuck with an older version of distro that doesn't receive bug fixes.

I was hoping that this PR would be fairly simple: that the difference between supporting 2.6 and not supporting 2.6 would be so minimal as to be accepted. In this way, you wouldn't have to officially maintain 2.6 support as long as you were willing to accept occasional small changes like this one from people like me who want to maintain 2.6 support.

I may need to start a conversation with the other Spack developers about how much longer we want to maintain 2.6 support ourselves. Our situation is a little bit different. We are developing a package manager targeted towards users of supercomputers which often come with outdated OSes. For reference, we just retired a cluster running CentOS 5 (Python 2.4) last year, and still have a cluster running CentOS 6 (Python 2.6) that we won't be retiring for some time. We could of course ask our users to install a more modern version of Python in order to run Spack, but a package manager that tells you to install your own software isn't too useful.

As for check_output, there's a python2.6 equivalent way, but it's slightly more verbose.

I actually tried something like that but it didn't work: b5b6a14
Maybe I was just missing the explicit AssertionError handling?

Actually, check_output is implemented in pure python in python2.7

I actually tried this too: 4b9c61c
No idea why it didn't work. You can see the past Travis logs if you want to try to debug it, but they meant nothing to me.

asottile · 2017-12-30T20:24:39Z

however we don't want to be forever stuck with an older version of distro that doesn't receive bug fixes.

well you're already stuck on an older version of python that hasn't received bug fixes in literal years so I question the validity of that concern 😆

Your failures in reimplmenting check_output is a misunderstanding between a function and a method (which receives self as its first argument) -- you'd want to decorate your check_output with @staticmethod most likely -- or since it doesn't really make sense as a method at all -- make it a module scoped function. With your patch you were passing self as the command and the command as the positional bufsize parameter.

Is it not an option for your consumers to use an older version of spack? they're already content with using older versions of basically every other package :)

sethmlarson · 2017-12-30T20:31:06Z

I'm actually 👍 on adding this and not advertising support. If this is all that's holding us from 2.6 I think it'd be nice to have around. There's nothing really that special about this library that keeps it from 2.6 as long as it implements 2.7.

Also as far as check_output goes I don't think it should be on the class. Just do something to the effect of:

import subprocess

try:
    _check_output = subprocess.check_output
except ImportError:
    def _check_output(cmd, **kwargs):
        proc = subproces.Popen(
            cmd,
            stdout=subprocess.PIPE,
            **kwargs
        )
        out = proc.communicate()[0]
        if proc.returncode:
            raise AssertionError('{0} exited {1}'.format(cmd, proc.returncode))
        return out

and then use _check_output where it needs to be used.

asottile · 2017-12-30T20:37:06Z

From prior experience, it'll regress again if you're not testing it -- it's also quite the complication to test it (and getting more and more difficult). At the least you'll need to:

reintroduce the argparse bits in setup.py -- I'd suggest instead doing that with conditional requirements, you can read mode about why here: https://github.com/anthonywritescode/episodes-py3/blob/master/03-backports/slides.md#writing-setuppy-5
pin an older version of tox: drop Python 2.6 and 3.3 support tox-dev/tox#689
pin an older version of pip, setuptools, wheel
pin an older version of pytest: Nomore py26 pytest-dev/pytest#2822
pin an older version of flake8

sethmlarson · 2017-12-30T20:40:25Z

asottile · 2017-12-30T20:45:22Z

2.6 won't be officially supported so we don't need to test it.

But then you're getting all the costs of supporting it (arguably more costs as reverts and re-releases after the fact are more costly -- not to mention the cognitive overhead while authoring and reviewing code with pseudo-support in the back of the mind) without supporting it.

I can't see many changes to distro in the future that would hurt 2.6 support?

And again it won't seem like you'll get changes that will hurt support but even little things like string formatting will re-break your compatibility.

I really believe it's in your best interest to cut ties with the past and continue on into the future with the rest of the python community :)

sethmlarson · 2017-12-30T20:48:42Z

@adamjstewart What's best for the "community" isn't always feasible for corporations and real-life workflows unfortunately. I think this request should be considered as it poses distro maintenance minimal trouble. Anyone else shoulders the other burdens themselves. :)

adamjstewart · 2017-12-30T20:52:04Z

From prior experience, it'll regress again if you're not testing it

Completely agree.

As for the dependencies, yes, you would have to pin older versions. That's what we do to test. Instead of pinning flake8 we only run flake8 tests on a single version of Python.

I really believe it's in your best interest to cut ties with the past and continue on into the future with the rest of the python community :)

Trust me, I really want to. But we don't get a choice in what platforms we support. 6 months ago, the newest supercomputer at ANL was running CentOS 6. If we dropped Python 2.6 support, the newest clusters at many national labs would no longer be supported. But I'll try to start that conversation and see what other developers think.

P.S. What is the newest version of distro that still had Python 2.6 support? (just in case we decide to stick with vendoring an older version)

sethmlarson · 2017-12-30T20:53:35Z

@adamjstewart Last version that supported 2.6 was 1.0.4 :)
I feel your pain with having to support EOL software, carry on the good work friend!

nir0s · 2017-12-30T22:27:50Z

@adamjstewart, I'm actually with @asottile on this one. The way I see distro is such that would be the "official" what-should-have-been platform.linux_distribution in the first place. This also means that it follows Python itself.

While I agree it's relatively easy to support Python 2.6 (we have up until just a while ago), I don't want to officially deal with addressing potential 2.6 issues going forward. It's not just that we'll have to accept this PR, but rather that any development going forward (into 3.7, and eventually, 4), will require that we keep 2.6 in mind. Things like string formatting, dict comprehensions, subprocess methods, test dependencies and the likes will have to be kept in mind.

Given that distro is relatively widely used, I don't feel like I'm the only one who should decide what happens. I'd rather have a wider discussion to decide this.

Assuming that we don't re-add support for 2.6, I simply suggest that we keep a branch of distro which includes 2.6 support and merge master into it every time. This branch can be vendored by anyone, anytime, and I (and hopefully whomever wants to contribute) will try to make sure it's up-to-date and working (should be travis tested, of course). If the 2.6 tests fail on that branch, we'll know that we need to fix them specifically for that branch (since they were obviously broken in master).

WDYT?

adamjstewart · 2017-12-30T22:45:52Z

@nir0s I understand that it can be difficult to support EOL versions of Python. Unfortunately, for complicated packages with many dependencies, this often means going with the lowest common denominator when it comes to Python support. We may have to stick with vendoring distro 1.0.4 until sites upgrade to RHEL 7. Unfortunately, RHEL 6 isn't EOL until 2020, so I expect we will need to support 2.6 for quite some time.

Assuming that we don't re-add support for 2.6, I simply suggest that we keep a branch of distro which includes 2.6 support and merge master into it every time. This branch can be vendored by anyone, anytime, and I (and hopefully whomever wants to contribute) will try to make sure it's up-to-date and working (should be travis tested, of course). If the 2.6 tests fail on that branch, we'll know that we need to fix them specifically for that branch (since they were obviously broken in master).

Wouldn't this be even more work to maintain than simply supporting 2.6 in the default branch?

sethmlarson · 2017-12-30T23:05:51Z

Not necessarily more work just duplicating work.

tgamblin · 2017-12-30T23:42:37Z

I'll just chime in here. distro is great -- we use it to label installs and binary packages for OS compatibility in Spack. One of the distros we pretty much have to support (and one that distro detects 😄 ) is RHEL6, and the default Python there is 2.6.

The prevalence of RHEL6 and CentOS6 (basically the same thing) is why we want this. If those two distros went away and nobody used them, we really wouldn't care and would be happy to drop support. But, Spack is a package manager and it's actually the way some people install Python 2.7 on older machines. We want it to work out of the box so that people don't have to bootstrap the thing they use to bootstrap their environment.

Most HPC sites that run RHEL6 probably have some mechanism (environment modules, etc.) to get Python 2.7, but there isn't a consistent way for us to detect that (all the sites are different). The Linux desktops at LLNL that still run RHEL6 don't really even have a good way to get Python 2.7.

@adamjstewart mentioned that RHEL6 is EOL in 2020. I think most places will upgrade to RHEL7 (which has Python 2.7 as the default) before then, and we might think about dropping 2.6 support in a year. But there are big but still very useful EOL systems (like the Blue Gene/Q's at LLNL and Argonne) that will likely not upgrade from RHEL6 for a while.

So, we'd love it if distro still supported Python 2.6. We'll end up backporting it if it's not in the mainline, and I don't think it's that much work. But if we backport it, it's work for us and not for you, so there's that.

tgamblin · 2017-12-31T05:21:54Z

Also here is an unscientific sample (sad though it may be) of what folks are using in HPC: https://twitter.com/tgamblin/status/947254477160988672

sethmlarson · 2017-12-31T05:33:01Z

If what we need to approve "unofficial" Python 2.6 support for distro is someone willing to put up with the work it requires I'm willing to put my name down. I have to deal with 2.6 for a couple of OSS modules I maintain anyways.

asottile · 2017-12-31T05:39:50Z

imo the easiest way forward is spack vendors 1.0.4 and we press onwards and upwards -- personally I'm turned off to contributing to projects if I have to code to the py26-lowest-common-denominator.

spack already seems content with vendoring old versions of libraries as evidenced by spack/spack#6791

tgamblin · 2017-12-31T06:05:36Z

spack already seems content with vendoring old versions of libraries as evidenced by spack/spack#6791

Jsonschema is a bit different. Our configure file format isn’t going to change rapidly, but people do want to use spack on newer and newer (as well as older and older!) distros. Keeping up with distro is more important.

Given what distro does, it seems like the sort of package that should work across the default python versions in distributions that are currently widely used. I do think RHEL6 is still pretty widespread. It’s only one RHEL release ago.

asottile · 2017-12-31T15:39:18Z

would this work?

given: old distro properly parses old operating systems today

vendor separate copies for python2.6 @ 1.0.4
vendor the latest for py27+ @ latest

at import of distro, choose based on the version

sethmlarson · 2017-12-31T15:42:27Z

@asottile We're adding support for new and old operating systems in new versions of distro though. See my recent PR for all BSD variants.

asottile · 2017-12-31T15:53:46Z

🤷‍♂️ it was worth a try. does BSD run ancient pythons too? does spack support BSD?

sethmlarson · 2017-12-31T16:08:19Z

Looking at OpenBSD's packages for Python I can see Python 2.7 and 3.6 on the most recent release of OpenBSD. Python isn't installed by default.

adamjstewart · 2017-12-31T23:37:08Z

does spack support BSD?

Spack supports everything except Windows (for now). Linux is definitely the most common for our users, but macOS and BSD are also supported.

nir0s · 2018-01-09T10:29:01Z

@adamjstewart, I truly understand what you're saying. I worked on an open-source orchestrator for enterprises and most of them ran Python 2.6. Even so, I stand for the reasons I posted earlier not to support 2.6.

I still think that supporting it unofficially with a concentrated effort would be a better way to go. You asked if that would require more maintenance. I believe that the overhead will be minimal but the main benefit is that we won't be officially bound to it. We won't have to fix bugs right away and we won't have to submit new versions quickly just because there's some ultra-minor bug specific to an EOL Python version.

That being said, this isn't my decision to make alone. If the majority's opinion is that we should re-support 2.6, we will.

If you're still interested, in the meanwhile, I can create a branch which we can maintain and merge all changes to it. I'll add you as a contributor (and whomever else who would like to contribute) and you can either vendor from that branch or use the branch directly.

WDYT?

adamjstewart · 2018-01-09T14:45:36Z

@nir0s I'm not sure if I can personally contribute (grad school commitments) but if anyone else from Spack wants to step up that might work.

tgamblin · 2018-01-09T14:48:59Z

I don’t mind doing it. I can’t say that we’ll be able to keep it completely current, but we can keep it current enough for our vendored copy. Would that work?

tgamblin · 2018-01-09T14:49:49Z

@adamjstewart: if you want to start by merging this PR into it and committing the resulting backport, that would be great.

tgamblin · 2018-01-09T14:52:00Z

Also for anyone interested, the poll I linked above got 76 votes and seems to have converged on ~30% of HPC sites still using a distro with Python 2.6.

sethmlarson · 2018-01-09T14:52:34Z

@tgamblin That's very compelling! I'm willing to work on the 2.6 support branch.

adamjstewart · 2018-01-09T14:56:04Z

@tgamblin I wouldn't merge this branch in as it doesn't pass the unit tests for some reason.

sethmlarson · 2018-01-09T14:57:47Z

@adamjstewart It looks like the tests aren't respecting PATH when calling lsb_release. That would be my guess.

hugovk · 2018-01-12T09:13:33Z

Here's the pip installs for distro from PyPI for the last month (via pypinfo --percent --pip --markdown distro pyversion).

python_version	percent	download_count
2.7	47.7%	30,093
3.6	23.3%	14,739
3.5	22.1%	13,972
3.4	6.1%	3,876
2.6	0.5%	347
3.3	0.2%	107
3.7	0.0%	10

tgamblin · 2018-01-12T10:51:08Z

Makes sense. I doubt many people are using pip to install things for their system python, and system installs are by and large where we see 2.6.

In our case (Spack) we vendor distro, so no pip. Spack is written in Python, and we want Spack to work out of the box b/c people use it to get a newer Python. We want that to work without having to use another package manager to bootstrap a newer python.

nir0s · 2018-01-21T07:20:35Z

https://github.com/nir0s/distro/tree/python2.6-support is now a protected branch branched from the latest master. Please use it for this PR.

adamjstewart · 2018-03-16T16:23:55Z

Closing as I never got this working and grad school prevents me from working on the things I love 😢

abadger · 2018-12-15T01:59:54Z

I've added a pair of PRs that get the HEAD version of distro working with Python-2.6.

Rebase python2.6 support #231 Rebase against current HEAD of master
Fix python2.6 support #232 Changes to restore python-2.6 support

#232 is based on the work here with some changes due to there being more usages of check_output now, not needing to use the compat version on Python-2.7+, and getting unittests to pass. I'd be happy if that got merged and I was perhaps made a contributor to keep it up to date with the current master branch. If there's other people who are interested in it as well, I'd be happy to see more contributors as I'll probably mostly just be syncing from master to the python2.6-support branch when Ansible is doing development towards a new release.

Regain Python 2.6 support

b5b6a14

adamjstewart mentioned this pull request Dec 29, 2017

Update to distro 1.0.4 spack/spack#6788

Merged

adamjstewart added 4 commits December 29, 2017 12:31

Flake8 fix

cb5c331

Don't use auxiliary function

85c30b8

Steal check_output code from Python 2.7

4b9c61c

Revert #197

dee3194

sethmlarson suggested changes Dec 29, 2017

View reviewed changes

adamjstewart mentioned this pull request Dec 30, 2017

Update to the latest version of jsonschema spack/spack#6791

Closed

adamjstewart closed this Mar 16, 2018

abadger mentioned this pull request Dec 13, 2018

Rebase python2.6 support #231

Merged

adamjstewart deleted the features/python2.6 branch December 15, 2018 02:05

Regain Python 2.6 support #206

Regain Python 2.6 support #206

Conversation

adamjstewart commented Dec 29, 2017

adamjstewart commented Dec 29, 2017

sethmlarson left a comment • edited Loading

Choose a reason for hiding this comment

sethmlarson Dec 29, 2017

Choose a reason for hiding this comment

sethmlarson Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

sethmlarson Dec 29, 2017

Choose a reason for hiding this comment

sethmlarson Dec 29, 2017

Choose a reason for hiding this comment

nir0s Dec 30, 2017

Choose a reason for hiding this comment

adamjstewart Dec 30, 2017

Choose a reason for hiding this comment

sethmlarson Dec 30, 2017

Choose a reason for hiding this comment

adamjstewart commented Dec 30, 2017

asottile commented Dec 30, 2017

asottile commented Dec 30, 2017

asottile commented Dec 30, 2017

adamjstewart commented Dec 30, 2017

asottile commented Dec 30, 2017

sethmlarson commented Dec 30, 2017 • edited Loading

asottile commented Dec 30, 2017

sethmlarson commented Dec 30, 2017 • edited Loading

asottile commented Dec 30, 2017 • edited Loading

sethmlarson commented Dec 30, 2017

adamjstewart commented Dec 30, 2017

sethmlarson commented Dec 30, 2017 • edited Loading

nir0s commented Dec 30, 2017

adamjstewart commented Dec 30, 2017

sethmlarson commented Dec 30, 2017

tgamblin commented Dec 30, 2017 • edited Loading

tgamblin commented Dec 31, 2017

sethmlarson commented Dec 31, 2017

asottile commented Dec 31, 2017

tgamblin commented Dec 31, 2017 • edited Loading

asottile commented Dec 31, 2017

sethmlarson commented Dec 31, 2017

asottile commented Dec 31, 2017

sethmlarson commented Dec 31, 2017

adamjstewart commented Dec 31, 2017

nir0s commented Jan 9, 2018

adamjstewart commented Jan 9, 2018

tgamblin commented Jan 9, 2018

tgamblin commented Jan 9, 2018

tgamblin commented Jan 9, 2018

sethmlarson commented Jan 9, 2018

adamjstewart commented Jan 9, 2018

sethmlarson commented Jan 9, 2018

hugovk commented Jan 12, 2018

tgamblin commented Jan 12, 2018

nir0s commented Jan 21, 2018

adamjstewart commented Mar 16, 2018

abadger commented Dec 15, 2018

sethmlarson left a comment •

edited

Loading

sethmlarson Dec 29, 2017 •

edited

Loading

sethmlarson commented Dec 30, 2017 •

edited

Loading

sethmlarson commented Dec 30, 2017 •

edited

Loading

asottile commented Dec 30, 2017 •

edited

Loading

sethmlarson commented Dec 30, 2017 •

edited

Loading

tgamblin commented Dec 30, 2017 •

edited

Loading

tgamblin commented Dec 31, 2017 •

edited

Loading