-
Notifications
You must be signed in to change notification settings - Fork 98
chore: require Python ≥ 3.9, protobuf ≥ 4.25.8 #877
Changes from 29 commits
1e45eb4
e3b50d9
4e73563
735b849
6d057a4
717c652
eb5ebef
807a2fa
03d47bd
9012e92
5ef9f1f
38cdc12
be692f4
397d7e1
32132a1
6e20230
9697839
657dcf5
26b8162
b49e1a8
eb08a55
e6d3c4c
c57a4cf
b5f1126
7294f00
a0a278a
0f0afeb
4339dd3
6d4c2eb
1ec9ad2
ff8b6b5
697680c
681ec47
c47dc40
60e9c90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -27,25 +27,7 @@ | |||
|
|
||||
| from google.api_core import exceptions, general_helpers | ||||
|
|
||||
| PROTOBUF_VERSION = google.protobuf.__version__ | ||||
|
|
||||
| # The grpcio-gcp package only has support for protobuf < 4 | ||||
| if PROTOBUF_VERSION[0:2] == "3.": # pragma: NO COVER | ||||
| try: | ||||
| import grpc_gcp | ||||
|
|
||||
| warnings.warn( | ||||
| """Support for grpcio-gcp is deprecated. This feature will be | ||||
| removed from `google-api-core` after January 1, 2024. If you need to | ||||
| continue to use this feature, please pin to a specific version of | ||||
| `google-api-core`.""", | ||||
| DeprecationWarning, | ||||
| ) | ||||
| HAS_GRPC_GCP = True | ||||
| except ImportError: | ||||
| HAS_GRPC_GCP = False | ||||
| else: | ||||
| HAS_GRPC_GCP = False | ||||
| HAS_GRPC_GCP = False | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created chaser PR #892 |
||||
|
|
||||
|
|
||||
| # The list of gRPC Callable interfaces that return iterators. | ||||
|
|
@@ -366,8 +348,7 @@ def create_channel( | |||
| result in `ValueError` as this combination is not yet supported. | ||||
|
|
||||
| kwargs: Additional key-word args passed to | ||||
| :func:`grpc_gcp.secure_channel` or :func:`grpc.secure_channel`. | ||||
| Note: `grpc_gcp` is only supported in environments with protobuf < 4.0.0. | ||||
| :func:`grpc.secure_channel`. | ||||
|
|
||||
| Returns: | ||||
| grpc.Channel: The created channel. | ||||
|
|
@@ -393,20 +374,6 @@ def create_channel( | |||
| default_host=default_host, | ||||
| ) | ||||
|
|
||||
| # Note that grpcio-gcp is deprecated | ||||
| if HAS_GRPC_GCP: # pragma: NO COVER | ||||
| if compression is not None and compression != grpc.Compression.NoCompression: | ||||
| warnings.warn( | ||||
| "The `compression` argument is ignored for grpc_gcp.secure_channel creation.", | ||||
| DeprecationWarning, | ||||
| ) | ||||
| if attempt_direct_path: | ||||
| warnings.warn( | ||||
| """The `attempt_direct_path` argument is ignored for grpc_gcp.secure_channel creation.""", | ||||
| DeprecationWarning, | ||||
| ) | ||||
| return grpc_gcp.secure_channel(target, composite_credentials, **kwargs) | ||||
|
|
||||
| if attempt_direct_path: | ||||
| target = _modify_target_for_direct_path(target) | ||||
|
|
||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,7 @@ | |
| from google.api_core.operations_v1.operations_rest_client_async import AsyncOperationsRestClient | ||
|
|
||
| __all__ += ["AsyncOperationsRestClient", "AsyncOperationsRestTransport"] | ||
| except ImportError: | ||
| except ImportError: # pragma: NO COVER | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mentioned removing the pragma mark in transports.init, but it seems like there are a few others we should handle the same way
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my other comment. I tried removing one and it didn't help, I don't think we should have a rule to remove them automatically; I'd rather be explicit. WDYT? |
||
| # This import requires the `async_rest` extra. | ||
| # Don't raise an exception if `AsyncOperationsRestTransport` cannot be imported | ||
| # as other transports are still available. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,7 @@ | |
| _transport_registry["rest_asyncio"] = cast( | ||
| OperationsTransport, AsyncOperationsRestTransport | ||
| ) | ||
| except ImportError: | ||
| except ImportError: # pragma: NO COVER | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should investigate why the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was necessary when I was working on this in December, before other work preempted continuing. I can try locally to remove the Are you referring just to this instance of the new
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I tested removing this one pragma and coverage fails. We could add these exceptions to =.coveragerc= or =pyproject.toml=, but honestly, I do like being explicit about where this is being triggered, so I'm tempted to keep the pragma. Thoughts? @parthea @daniel-sanche (I'm keeping the pragma deletion for the moment, but if we decide we're OK with being explicit, I'll revert that change so the presubmits pass again.)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think if we have to add a no cover mark, this would be the place to do it. But is there a way we can make sure it's covered? It sounds like we're never testing it without the extra? Is that what the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked through the code a bit more, and it looks like the logic suggests this should raise an ImportError if google auth is installed without the aiohttp option. Which is what would have happened with auth<2.25, because it doesn't even have the AsyncAuthorizedSession class But in 2.35, if aiohttp is missing, it suppresses the error at import time, and then raises it at init time. So it looks like these except clauses won't ever be evaluated with auth google-auth 2.35 installed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I think the right approach is to remove this except, and make it so that AsyncOperationsRestTransport is always exported, but will raise an exception on use if aiohttp isn't installed. That seems like the right behaviour
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think we should discuss and possibly do this separately. I do agree that removing dead code is good, but I din't think it should be included in this PR:
Removing this pattern should be thought out well and implemented consistently, and should include guidelines for what to do in the future. We can file a separate issue to follow up. [*] This pattern of catching
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think this part is true. if we lock to google-auth 2.35, this except block will never be triggered, so it doesn't really tell us anything. Looking at the code, it's responding to the presence of AsyncAuthorizedSession, not the aiohttp dependency. So this except block is only relevant when google-auth < 2.35 is used, and will never be hit with the new pin I'm fine with punting this change to a different release, but if that's the case, I think we should revert the google-auth version change as well. Otherwise we'll need to be adding in NO COVER marks over dead code paths, that can't actually be executed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The presubmits are passing with the code as is, so I think we should do this clean-up separately, both to avoid new problems and to have single-scope PRs. My comment about the Filed #891 . Happy to work on that right after we submit this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think what happened is that I must have opted for a uniform versions of |
||
| # This import requires the `async_rest` extra. | ||
| # Don't raise an exception if `AsyncOperationsRestTransport` cannot be imported | ||
| # as other transports are still available. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.