ci: Pin Python Version

[panicstevenson]

Pin the CPython version in Python-oriented GitHub Actions to a micro/patch version.

I have been waffling on this issue for a couple hours now, trying to exactly pinpoint what broke in the upgrade to CPython 3.7.8:

Click for recent Python-lint GitHub Action run error output.

Run pip install pylint==2.2.3 PyYAML==5.1.2 requests==2.22.0
  pip install pylint==2.2.3 PyYAML==5.1.2 requests==2.22.0
  shell: /bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.7.8/x64
Collecting pylint==2.2.3
  Downloading pylint-2.2.3-py3-none-any.whl (750 kB)
Collecting PyYAML==5.1.2
  Downloading PyYAML-5.1.2.tar.gz (265 kB)
Collecting requests==2.22.0
  Downloading requests-2.22.0-py2.py3-none-any.whl (57 kB)
Collecting astroid<2.2.0,>=2.0
  Downloading astroid-2.1.0-py3-none-any.whl (176 kB)
Collecting mccabe
  Downloading mccabe-0.6.1-py2.py3-none-any.whl (8.6 kB)
Collecting isort>=4.2.5
  Downloading isort-5.0.4-py3-none-any.whl (153 kB)
Collecting idna<2.9,>=2.5
  Downloading idna-2.8-py2.py3-none-any.whl (58 kB)
Requirement already satisfied: urllib3!=1.25.0,!=1.25.1,<1.26,>=1.21.1 in /opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/urllib3-1.25.9-py3.7.egg (from requests==2.22.0) (1.25.9)
Requirement already satisfied: chardet<3.1.0,>=3.0.2 in /opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/chardet-3.0.4-py3.7.egg (from requests==2.22.0) (3.0.4)
Requirement already satisfied: certifi>=2017.4.17 in /opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/certifi-2020.6.20-py3.7.egg (from requests==2.22.0) (2020.6.20)
Collecting wrapt
  Downloading wrapt-1.12.1.tar.gz (27 kB)
Collecting lazy-object-proxy
  Downloading lazy_object_proxy-1.5.0-cp37-cp37m-manylinux1_x86_64.whl (57 kB)
Requirement already satisfied: six in /opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/six-1.15.0-py3.7.egg (from astroid<2.2.0,>=2.0->pylint==2.2.3) (1.15.0)
Building wheels for collected packages: PyYAML, wrapt
  Building wheel for PyYAML (setup.py): started
  Building wheel for PyYAML (setup.py): finished with status 'done'
  Created wheel for PyYAML: filename=PyYAML-5.1.2-cp37-cp37m-linux_x86_64.whl size=44104 sha256=ada23ec8ea389863aa4b5f9b886c93d3d01099392a0d5056b08235ae39b4d961
  Stored in directory: /home/runner/.cache/pip/wheels/23/b9/73/57aaccb6957d94ed63f474b51a9f7f992c5eff4635052c0557
  Building wheel for wrapt (setup.py): started
  Building wheel for wrapt (setup.py): finished with status 'done'
  Created wheel for wrapt: filename=wrapt-1.12.1-cp37-cp37m-linux_x86_64.whl size=71068 sha256=3f43a05b73e73190d1b4d32faced1f696b52f7b072f6d1fb6e7410059d0ad5c4
  Stored in directory: /home/runner/.cache/pip/wheels/62/76/4c/aa25851149f3f6d9785f6c869387ad82b3fd37582fa8147ac6
Successfully built PyYAML wrapt
ERROR: aws-sam-cli 0.53.0 has requirement requests==2.23.0, but you'll have requests 2.22.0 which is incompatible.
Installing collected packages: wrapt, lazy-object-proxy, astroid, mccabe, isort, pylint, PyYAML, idna, requests
  Attempting uninstall: PyYAML
    Found existing installation: PyYAML 5.3.1
    Uninstalling PyYAML-5.3.1:
      Successfully uninstalled PyYAML-5.3.1
  Attempting uninstall: idna
    Found existing installation: idna 2.10
    Uninstalling idna-2.10:
      Successfully uninstalled idna-2.10
  Attempting uninstall: requests
    Found existing installation: requests 2.23.0
    Uninstalling requests-2.23.0:
      Successfully uninstalled requests-2.23.0
Successfully installed PyYAML-5.1.2 astroid-2.1.0 idna-2.8 isort-5.0.4 lazy-object-proxy-1.5.0 mccabe-0.6.1 pylint-2.2.3 requests-2.22.0 wrapt-1.12.1
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.7.8/x64/bin/pip", line 8, in <module>
    sys.exit(main())
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_internal/cli/main.py", line 75, in main
    return command.main(cmd_args)
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_internal/cli/base_command.py", line 114, in main
    return self._main(args)
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_internal/cli/base_command.py", line 226, in _main
    self.handle_pip_version_check(options)
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_internal/cli/req_command.py", line 155, in handle_pip_version_check
    timeout=min(5, options.timeout)
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_internal/cli/req_command.py", line 100, in _build_session
    index_urls=self._get_index_urls(options),
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_internal/network/session.py", line 249, in __init__
    self.headers["User-Agent"] = user_agent()
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_internal/network/session.py", line 159, in user_agent
    setuptools_version = get_installed_version("setuptools")
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_internal/utils/misc.py", line 634, in get_installed_version
    working_set = pkg_resources.WorkingSet()
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_vendor/pkg_resources/__init__.py", line 567, in __init__
    self.add_entry(entry)
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_vendor/pkg_resources/__init__.py", line 623, in add_entry
    for dist in find_distributions(entry, True):
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_vendor/pkg_resources/__init__.py", line 1983, in find_eggs_in_zip
    if metadata.has_metadata('PKG-INFO'):
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_vendor/pkg_resources/__init__.py", line 1414, in has_metadata
    return self._has(path)
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_vendor/pkg_resources/__init__.py", line 1854, in _has
    return zip_path in self.zipinfo or zip_path in self._index()
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_vendor/pkg_resources/__init__.py", line 1731, in zipinfo
    return self._zip_manifests.load(self.loader.archive)
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/pip/_vendor/pkg_resources/__init__.py", line 1688, in load
    mtime = os.stat(path).st_mtime
FileNotFoundError: [Errno 2] No such file or directory: '/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/site-packages/PyYAML-5.3.1-py3.7-linux-x86_64.egg'
##[error]Process completed with exit code 1.

As it turns out, re-running CPython 3.7.7 builds works, but I didn't absolutely love this as a solution since we're already pinned at the minor release (informed readers may already know this is the ending to the story, but don't let that discourage you from reading the tales below!), I felt as though patches should be natural and hopefully non-breaking (or breaking, but in a good way). Additionally, up-pinning our PyYAML version also works, which I considered to be a better approach at the time. Digging into why it works though is pretty bad.

There's a pretty curious error message that gets output:

ERROR: aws-sam-cli 0.53.0 has requirement requests==2.23.0, but you'll have requests 2.22.0 which is incompatible.

And I had spent a good amount of time wondering where this had come from, and what one of our three dependencies and their chain might be installing this; turns out, I installed pipdeptree locally and none of them are installing this.

So, I shelved that, and I went to go look at why two days ago (previous working runs) this hadn't been an issue. The setup-python GitHub Action doesn't look at the upstream PyPi for its resolution, but instead at python-versions, which still didn't tell me why two days ago this worked, since the latest release was older than that. From there, I looked into what's installed on the GitHub Action runners and found that it's installing the AWS SDK.

This is basically a long-winded way of saying that the AWS SDK is installing PyYAML at 5.3.1, which means that up-pinning PyYAML does work, but for all the wrong reasons (basically it says we already have that dependency and skips it, 🎉 ).

There's a lot more red-herrings in here around some other things like setuptools not working explicitly for pyyaml, but that doesn't seem correct either since we should be using an older version (47.1.0) as per the CPython release notes/changelog (verified), and upgrading to the fixed version in the linked issue doesn't fix the problem.

There are alternative answers here as well, such as up-pinning PyYAML (I may warm up to this idea, but it sounds fundamentally bad), or adding --ignore-installed to the pip command, but all in all I'd prefer this command to be something you'd expect someone to invoke locally as well. I think this is (probably) a non-issue for most people, since I think I'm one of the few unix users of cactbot, and more specifically one of the few people that even runs Ubuntu, and this still doesn't affect me since I don't have weird conflicting issues with PyYAML locally.

This is all to say that I don't have an answer, this is acceptable, and it "solves" broken things for the time being. Future me will probably shake his future head at past/current me, but until then I think this is "good enough" for now. Thanks for coming to my TED talk.

[quisquous]

Hmm, why would up-pinning pyyaml cause it to say we already have that dependency and then skip? Why wouldn't it use a more recent version? (Also why does black's install line ask for pyyaml anyway? If it's a dependency, shouldn't that be automatic?)

[panicstevenson]

PyYAML 5.3.1 (the latest version) is used by the AWS SDK; technically they pin to 5.X. It's installed on the system by the time we get there.

PyYAML is our dependency, so that's why it's listed (and subsequently why I pinned it).

[quisquous]

Ah right I forgot about the fishing script.

I guess I don't quite understand why uppinning pyyaml is a no-op. Wouldn't that also fit the criteria you said earlier, in that somebody could run this install command locally and it would work?

[panicstevenson]

Ah right I forgot about the fishing script.

First off: how dare you?

I guess I don't quite understand why uppinning pyyaml is a no-op. Wouldn't that also fit the criteria you said earlier, in that somebody could run this install command locally and it would work?

I'm taking no-op here to mean "something I'm avoiding doing," which, you'd be right, this would make me very warm and fuzzy in that someone could come through and run that command and they'd be running pylint/black against the cactbot repository and everything would be happy (including me, possibly). The issue is that the AWS SDK isn't pinning to a version, so if PyYAML came out with a new version we'd be in this mess again, since 5.4.0 or 5.3.2 would cause this to no longer say "I'm happy because that thing you wanted is right here, silly!" and break again. However, that's also in undefined future times (if ever) and we'll probably be on 3.7.9+ of CPython and none of this will be an issue.

It's not really a software issue, more of a "I don't like this because of ehh" issue, so if you (or people!) have opinions, mine is not so steadfast that it can't be changed.

There's also the opinion that we should probably just install with --user and set up the PATH environment to use the runner's cache directory as a place that can run binaries, but that's current/future me arguing with past me, and I can attempt to make that change (I'm assuming without verifying; that's never gone wrong) if we feel that's vaguely "better". I was mainly looking to unblock as of last night, since I feel pretty awful that the CI systems aren't completely transparent to contributors.

[quisquous]

Ah, I see. That is not the way I use the word "no-op".

Hmm. Is not specifying a pyyaml version on our end an option? Or do you feel equally bad about that?

[panicstevenson]

That's an option as well; technically none of our version dependencies need to be pinned (although we did run into that pylint error that one time on Travis 😉 (and that one time even more recently with isort)), but it feels nice to "guarantee" it works with a certain manifest of tool installs, but it's also kind of unnecessary in the sense that someone should be able to install whatever version of PyYAML (or requests) and it should just work with some reasonable amount of success. Things like black or pylint are a little more stringent since we directly depend on their exit codes, but really PyYAML (and requests) only has to be installed to avoid import errors, and for 100% of the current CI use-cases the versioning is irrelevant.

[panicstevenson]

After talking with @quisquous offline, I think I'm going to abandon this pull request; it doesn't sound like it's the correct solution in any sense. I opened it knowing that it wasn't going to be the permanent fix going forward, but now that it's simmered a bit it feels ultimately like a very poor solution.

We're well beyond technical solutions at this point, and I think now we're discussing what this should look like going forward. I think there are two potential solutions here with differing main goals; in no order:

• Unpin dependency versions for dependencies that we don't rely on their exit code for. At the moment, that would be requests and PyYAML. For black and pylint, we directly depend on their exit codes and it makes sense to move those dependencies up when we're comfortable with that and not have them randomly bark on a PR where someone's making a minor change and was unlucky enough to get the proverbial hot potato.
  • Pros: Intuitive and easy to grok
  • Cons: Breaks if the GitHub Action runners ever begin to use pylint or black with pinned dependencies that don't match ours
• Install dependencies with pip's --user flag. This will "permanently" isolate the build's dependencies from the system, but requires updating PATH variables and might be overkill for the casual developer looking to run the CI commands locally.
  • Pros: Should "never" break again
  • Cons: Difficult to grok, might make users looking to replicate CI commands confused

Going to cc @JLGarber here since he's pretty much the main consumer of the Python CI stuff beyond @quisquous and myself. I'm pretty indifferent to the options presented; I have a desire for the CI system to "just work" and stop being intrusive to the developer, but at the same time I don't want to add anything that makes the CI systems scarier than they need to be for anyone looking to contribute in that corner of the repository.

If anyone feels super strongly in either direction, I think I'm okay with taking either approach at this point.

[JLGarber]

I think it's fair to say that my level of care is mostly at "does it run when I submit a pull request?" and "how often will I get inscrutable errors when I run tests locally?" Solely on that basis I (think I) would prefer the first option, but of course that's mostly just kicking the can until (if?) the action runners pin different dependencies.

Maybe another way of looking at it is, how often woud you anticipate the action runners changing their pins, and how long would it be likely to take you to research and resolve this if/when that happens? If it's just a few minutes once every 3-5 months, I think that's a reasonable tradeoff. If it's an hour every 3-5 months, that's less reasonable.

[quisquous]

I think it seems quite unlikely that the action runners would start installing pylint or black if they weren't already. It's not just changing the pin, but it'd be installing it at all. So it's less kicking the can in the sense that the dependency exists and will eventually be updated and more instead taking a gamble that the dependency will ever be added directly or indirectly in the future to the action itself.

I think on the chance that happened, we could switch to the --user option, and it'd be pretty obvious from the error messages what was going on, especially after all this investigation.

[panicstevenson]

Keep in mind that it's not only the what the runners themselves install, but what the software that they install also installs; that being said, why would you add a linter as a dependency to your package? I think the risk is pretty minimal, and the fix is (and hopefully remains) easy.

[panicstevenson]

Closing in favor of #1548.