Skip to content
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

Fix import error introduced in 21.1 #9835

Merged
merged 1 commit into from
Apr 25, 2021
Merged

Conversation

jamescurtin
Copy link
Contributor

Fixes #9831 by guarding the import of NoReturn in a if TYPE_CHECKING: block because this type was only introduced in Python 3.6.2, but pip is compatible with Python >=3.6.0

@@ -93,6 +93,7 @@ jobs:
matrix:
os: [Ubuntu, MacOS]
python:
- 3.6.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea here of pinning to 3.6.0 to make sure we don't regress, but doing so will (if I'm reading the docs correctly) result in 3.6.0 getting downloaded, rather than using the preinstalled version of 3.6. That will slow down our CI a bit, and we have an ongoing fight to speed up our CI. @pypa/pip-committers awhat do you think? Is this worthwhile?

Also, we really don't need both 3.6.0 and 3.6...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More than happy to remove this if you're comfortable. I added it as a way to demonstrate that this patch resolves the issue, but perhaps the value is outweighed by the interest in speeding up CI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea in principle, but let's see what the other committers think.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could have scheduled pipelines where you enable such configuration(s).. ?

@pfmoore
Copy link
Member

pfmoore commented Apr 24, 2021

Also, it looks like black wants to mess with the changed code 🙁

@pradyunsg
Copy link
Member

pradyunsg commented Apr 24, 2021

Also, it looks like black wants to mess with the changed code 🙁

Not black, isort.


From our documentation:

pip works with CPython versions 3.6, 3.7, 3.8, 3.9 and also PyPy.

This means pip works on the latest patch version of each of these minor versions. Previous patch versions are supported on a best effort approach.

We explicitly state that we support the latest patch version releases of CPython versions and other versions are on a best-effort basis. I'm definitely a strong -1 on adding 3.6.0 to our CI.


I'm a bit bleh on this PR overall TBH. I don't think this fix is worth making unless it's a quick bugfix release and we're yanking the 21.1 release because of this issue. And... Well, I don't think this is worth doing that much churn anyway?

@pradyunsg
Copy link
Member

Anyway, that doesn't work either:

Error: Version 3.6.0 with arch x64 not found

@jamescurtin
Copy link
Contributor Author

jamescurtin commented Apr 24, 2021

We explicitly state that we support the latest patch version releases of CPython versions and other versions are on a best-effort basis. I'm definitely a strong -1 on adding 3.6.0 to our CI.

Thanks for the feedback on that: I have removed it.

I'm a bit bleh on this PR overall TBH. I don't think this fix is worth making unless it's a quick bugfix release and we're yanking the 21.1 release because of this issue. And... Well, I don't think this is worth doing that much churn anyway?

Based on the principle that previous patch versions are supported on a "best effort" basis, it seems that resolving this regression would be reasonable. If the maintainers felt strongly about not wanting to make this patch, I'd advocate for updating setup.py to set the minimum Python version to 3.6.2.

Thoughts?

@pradyunsg
Copy link
Member

pradyunsg commented Apr 24, 2021

I'm personally not going to be doing anything for < 3.6.2 support.

Honestly, I couldn't care less about what we do here. 3.6.2 is from 2017 (https://www.python.org/dev/peps/pep-0494/#id1), we explicitly state that the supported Python versions are the latest patch version. Any support for significantly older versions is kindness for folks on those older versions, and I have other things to do that I care about more.

I only said something here because there were changes to the CI for guaranteeing something we explicitly state the opposite of in our documentation. That change wouldn't have worked anyway, so I'm gonna shut up now. :)

@pradyunsg
Copy link
Member

pradyunsg commented Apr 24, 2021

That said, I'm not the only maintainer here, so if some other maintainer does have any interest in fixing this, I'm not going to be blocking them. :)

@jamescurtin
Copy link
Contributor Author

@pfmoore (or other maintainers):

Is there any interest in reviewing and accepting this PR such that pip is compatible with all patch versions of 3.6?

@sbidoul
Copy link
Member

sbidoul commented Apr 24, 2021

I'm personally not opposed to merging this.

I'd add a comment explaining why NoReturn is under TYPE_CHECKING, so it is not accidentally removed. Can you also squash the commits?

However, I'm not sure this alone warrants a bugfix release.

@uranusjr
Copy link
Member

+1 to merging this. We probably have a need for a patch release anyway (for the headers mismatch thing) and this fixes a real issue (albeit edge case) with virtually zero downsides.

@jamescurtin
Copy link
Contributor Author

I'm personally not opposed to merging this.

I'd add a comment explaining why NoReturn is under TYPE_CHECKING, so it is not accidentally removed. Can you also squash the commits?

However, I'm not sure this alone warrants a bugfix release.

Done--thank you for taking a look!

@pfmoore
Copy link
Member

pfmoore commented Apr 24, 2021

I see no harm in merging it, so I've approved. But I'll leave the merge to someone else.

By the way, this thing were I have to approve every CI run for a new contributor, not just the first one, is a complete pain in the a***. (But I'm not blaming you, @jamescurtin, just to be clear!)

@sbidoul sbidoul added this to the 21.1.1 milestone Apr 25, 2021
@sbidoul sbidoul merged commit 7a77484 into pypa:main Apr 25, 2021
@sbidoul
Copy link
Member

sbidoul commented Apr 25, 2021

Thank you @jamescurtin

@jamescurtin jamescurtin deleted the 9831-bugfix branch April 25, 2021 12:13
@sergiitk
Copy link

Thank you for fixing this! May I ask when is it expected to be released? As I understand, this is going out in 21.1.1. grpc/grpc has a few buildscripts running tests with python 3.6.1, and now they're broken. Just to understand whether to pin to the previous release, or wait until 21.1.1 is released.

@maciejkula
Copy link

+1 for a speedy release (and thank you for the fix)!

copybara-service bot pushed a commit to tensorflow/recommenders that referenced this pull request Apr 26, 2021
…oo large.

Work around pypa/pip#9835.

PiperOrigin-RevId: 370472029
copybara-service bot pushed a commit to tensorflow/recommenders that referenced this pull request Apr 26, 2021
…oo large.

Work around pypa/pip#9835.

PiperOrigin-RevId: 370560208
@pradyunsg
Copy link
Member

I suggest subscribing to #9761 for release-related updates. Please be mindful about making comments there though, since lots of folks have subscribed to that issue for release-related updates.

lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Apr 28, 2021
Issue is get_pip.py is moved to pip 21.1 (https://github.com/pypa/get-pip/commits/main) which is not compatible with 3.6.
Issue of pip itself is fixed as part of 21.1.1 in pip community (pypa/pip#9835).
However get-pip.py is still not updated to latest pip. Also get.pip.py does not support python 3.6 version explicitly (pypa/get-pip#88)

Step 15/29 : RUN curl https://bootstrap.pypa.io/get-pip.py | python3.6
 ---> Running in bece31f49267
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 1891k  100 1891k    0     0  9564k      0 --:--:-- --:--:-- --:--:-- 9600k
Traceback (most recent call last):
  File "<stdin>", line 24298, in <module>
  File "<stdin>", line 139, in main
  File "<stdin>", line 115, in bootstrap
  File "<stdin>", line 96, in monkeypatch_for_cert
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/commands/__init__.py", line 9, in <module>
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/cli/base_command.py", line 12, in <module>
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/cli/cmdoptions.py", line 30, in <module>
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/utils/hashes.py", line 2, in <module>
ImportError: cannot import name 'NoReturn'
The command '/bin/sh -c curl https://bootstrap.pypa.io/get-pip.py | python3.6' returned a non-zero code: 1
How I did:

Got the file from https://github.com/pypa/get-pip/tree/21.0 and added to the buildimage
pin pip to the previous release 21.0.1. (Similar is done in other public repos eg: grpc/grpc-java#8115)

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
lguohan pushed a commit to lguohan/sonic-buildimage that referenced this pull request Apr 28, 2021
Issue is get_pip.py is moved to pip 21.1 (https://github.com/pypa/get-pip/commits/main) which is not compatible with 3.6.
Issue of pip itself is fixed as part of 21.1.1 in pip community (pypa/pip#9835).
However get-pip.py is still not updated to latest pip. Also get.pip.py does not support python 3.6 version explicitly (pypa/get-pip#88)

Step 15/29 : RUN curl https://bootstrap.pypa.io/get-pip.py | python3.6
 ---> Running in bece31f49267
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 1891k  100 1891k    0     0  9564k      0 --:--:-- --:--:-- --:--:-- 9600k
Traceback (most recent call last):
  File "<stdin>", line 24298, in <module>
  File "<stdin>", line 139, in main
  File "<stdin>", line 115, in bootstrap
  File "<stdin>", line 96, in monkeypatch_for_cert
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/commands/__init__.py", line 9, in <module>
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/cli/base_command.py", line 12, in <module>
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/cli/cmdoptions.py", line 30, in <module>
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/utils/hashes.py", line 2, in <module>
ImportError: cannot import name 'NoReturn'
The command '/bin/sh -c curl https://bootstrap.pypa.io/get-pip.py | python3.6' returned a non-zero code: 1
How I did:

Got the file from https://github.com/pypa/get-pip/tree/21.0 and added to the buildimage
pin pip to the previous release 21.0.1. (Similar is done in other public repos eg: grpc/grpc-java#8115)

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
yxieca pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Apr 29, 2021
* [build] Fix the snmp docker build error. (#7452)

Issue is get_pip.py is moved to pip 21.1 (https://github.com/pypa/get-pip/commits/main) which is not compatible with 3.6.
Issue of pip itself is fixed as part of 21.1.1 in pip community (pypa/pip#9835).
However get-pip.py is still not updated to latest pip. Also get.pip.py does not support python 3.6 version explicitly (pypa/get-pip#88)

Step 15/29 : RUN curl https://bootstrap.pypa.io/get-pip.py | python3.6
 ---> Running in bece31f49267
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 1891k  100 1891k    0     0  9564k      0 --:--:-- --:--:-- --:--:-- 9600k
Traceback (most recent call last):
  File "<stdin>", line 24298, in <module>
  File "<stdin>", line 139, in main
  File "<stdin>", line 115, in bootstrap
  File "<stdin>", line 96, in monkeypatch_for_cert
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/commands/__init__.py", line 9, in <module>
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/cli/base_command.py", line 12, in <module>
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/cli/cmdoptions.py", line 30, in <module>
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/utils/hashes.py", line 2, in <module>
ImportError: cannot import name 'NoReturn'
The command '/bin/sh -c curl https://bootstrap.pypa.io/get-pip.py | python3.6' returned a non-zero code: 1
How I did:

Got the file from https://github.com/pypa/get-pip/tree/21.0 and added to the buildimage
pin pip to the previous release 21.0.1. (Similar is done in other public repos eg: grpc/grpc-java#8115)

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
copybara-service bot pushed a commit to google-deepmind/dm-haiku that referenced this pull request Apr 29, 2021
PiperOrigin-RevId: 371166775
Change-Id: I111e81557f0129841e6bdd7c63b0e2b03be595fc
LuiSzee pushed a commit to CentecNetworks/sonic-buildimage that referenced this pull request Aug 10, 2021
Issue is get_pip.py is moved to pip 21.1 (https://github.com/pypa/get-pip/commits/main) which is not compatible with 3.6.
Issue of pip itself is fixed as part of 21.1.1 in pip community (pypa/pip#9835).
However get-pip.py is still not updated to latest pip. Also get.pip.py does not support python 3.6 version explicitly (pypa/get-pip#88)

Step 15/29 : RUN curl https://bootstrap.pypa.io/get-pip.py | python3.6
 ---> Running in bece31f49267
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 1891k  100 1891k    0     0  9564k      0 --:--:-- --:--:-- --:--:-- 9600k
Traceback (most recent call last):
  File "<stdin>", line 24298, in <module>
  File "<stdin>", line 139, in main
  File "<stdin>", line 115, in bootstrap
  File "<stdin>", line 96, in monkeypatch_for_cert
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/commands/__init__.py", line 9, in <module>
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/cli/base_command.py", line 12, in <module>
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/cli/cmdoptions.py", line 30, in <module>
  File "/tmp/tmp5fnxrz0a/pip.zip/pip/_internal/utils/hashes.py", line 2, in <module>
ImportError: cannot import name 'NoReturn'
The command '/bin/sh -c curl https://bootstrap.pypa.io/get-pip.py | python3.6' returned a non-zero code: 1
How I did:

Got the file from https://github.com/pypa/get-pip/tree/21.0 and added to the buildimage
pin pip to the previous release 21.0.1. (Similar is done in other public repos eg: grpc/grpc-java#8115)

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minimum Python version for 21.1 is 3.6.2
8 participants