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

Add version check for Python 3.6 to simple stubs #22549

Merged
merged 2 commits into from
Apr 2, 2020

Conversation

gnossen
Copy link
Contributor

@gnossen gnossen commented Apr 2, 2020

#21954 introduced files using f-strings and type annotations guarded by an interpreter version check. That version check was not stringent enough.

What's more, it turns out we don't have presubmit tests for Python 3.5. #22546 includes reports of users of 3.5 failing to import grpc.experimental.

@gnossen gnossen added kind/bug lang/Python release notes: no Indicates if PR should not be in release notes labels Apr 2, 2020
@gnossen gnossen requested a review from lidizheng April 2, 2020 06:47
@ericgribkoff
Copy link
Contributor

This was also hitting some of our own CI (the Go xDS tests on master, but I think it's just luck other workers haven't been using 3.5 as well).

@@ -85,6 +85,6 @@ def _wrapper(*args, **kwargs):
'insecure_channel_credentials',
)

if sys.version_info[0] >= 3:
if sys.version_info[0] == 3 and sys.version_info[1] >= 6:
Copy link
Contributor

Choose a reason for hiding this comment

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

If you remove the f-string, then we can support 3.6 users. Simple stubs can be available to more users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. This was originally a middle-of-the-night quick fix. Now that the pressure is off, I'll take some time to try to extend support back to 3.5.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. Claim supporting 3.5 might need extra efforts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lidi, I took another look at this and the problem is more than just f-strings. It's also PEP 526 variable annotations that don't work in 3.5. I think it's reasonable to have the same interpreter requirements as our asyncio support, namely, 3.6+.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry... I mean, to securely claim 3.5 is properly supported. We will need the infrastructure that we discussed today. Manual tests might work for now, but might not protect this environment in the future. So, I agree that a separate PR is better than try to support 3.5 in this one.

@@ -85,6 +85,6 @@ def _wrapper(*args, **kwargs):
'insecure_channel_credentials',
)

if sys.version_info[0] >= 3:
if sys.version_info[0] == 3 and sys.version_info[1] >= 6:
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. Claim supporting 3.5 might need extra efforts.

@gnossen gnossen merged commit 0ec3263 into grpc:master Apr 2, 2020
gnossen added a commit that referenced this pull request Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug lang/Python release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants