feat: check Python and dependency versions in generated GAPICs #2419
Conversation
|
Both the package dependency and the Python run-time checks depend on googleapis/python-api-core#832 |
product-auto-label
bot
added
size: m
| return parse_version(version_string) | ||
|
|
||
| try: | ||
| _dependency_package = "google.protobuf" |
There was a problem hiding this comment.
Is this the correct package name? It falls through to the except clause for me when I run the integration tests. Do I need to create a real GAPIC to test this properly?
|
This PR will now do basic checks even if There are still some presubmit failures that I will continue investigating. |
vchudnov-g
force-pushed
the
python-version-support-check
branch
2 times, most recently
from
dce9bdf to
8645ead
Compare
vchudnov-g
force-pushed
the
python-version-support-check
branch
from
aa6c360 to
34eb7ab
Compare
vchudnov-g
force-pushed
the
python-version-support-check
branch
11 times, most recently
from
7db28d8 to
a4eb323
Compare
vchudnov-g
added
the
do not merge
|
Do not merge yet while I work out the final details of this initial implementation and we get approval on the design. (I'm expecting pre-submits to pass now.) |
vchudnov-g
added
the
owlbot:run
vchudnov-g
force-pushed
the
python-version-support-check
branch
from
7a2994e to
406f48d
Compare
|
Removing |
vchudnov-g
removed
the
do not merge
|
|
||
| if hasattr(api_core, "check_python_version") and hasattr(api_core, "check_dependency_versions"): # pragma: NO COVER | ||
| api_core.check_python_version("google.cloud.asset_v1") # type: ignore | ||
| api_core.check_dependency_versions("google.cloud.asset_v1") # type: ignore |
There was a problem hiding this comment.
Just to make sure I understand: we expect to see duplicate warnings for each GCP library and api_core, right?
There was a problem hiding this comment.
Not exactly duplicate; just very similar, one for each of our packages losing support for the Python version currently being used. Paraphrasing the exact wording, here's an example:
Warning: This `api_core` package is running on Python 3.7. There will be no more updates to `api_core` on Python 3.7 installations.
Warning: This `google.cloud.foo_v1` package is running on Python 3.7. There will be no more updates to `google.cloud.foo_v1` on Python 3.7 installations.
|
|
||
| if hasattr(api_core, "check_python_version") and hasattr(api_core, "check_dependency_versions"): # pragma: NO COVER | ||
| api_core.check_python_version("google.cloud.asset_v1") # type: ignore | ||
| api_core.check_dependency_versions("google.cloud.asset_v1") # type: ignore |
There was a problem hiding this comment.
I wonder if api_core.check_dependency_versions should take in a list of dependencies, and have the generator populate it? Or at least provide the option to override? It seems a little unexpected that all the dependencies are always hard-coded in the api_core layer
There was a problem hiding this comment.
The intent was to have each package issue a warning saying it would soon stop getting updated if the specified dependencies were pinned to old versions. I guess when this PR started as a prototype PR, I was thinking that the dependencies in question were the same for both packages, so we would re-use the warnings.
But I think there's a lot of merit to what you're suggesting: each of our artifacts sets up the warning message for its direct, not transitive, dependencies.
Update: I updated googleapis/python-api-core#832 so we could pass the list of dependencies. If none are given, check_dependency_versions defaults to the dependencies in api_core. So we can start with this, and then change things so the dependencies are explicitly listed here, especially when we have GAPIC-specific dependencies.
There was a problem hiding this comment.
Awesome, that seems like a great change!
| "(b) you manually update your Python environment to use at " + | ||
| f"least version {_next_supported_version} of " + | ||
| f"{_dependency_package}.", | ||
| FutureWarning) |
There was a problem hiding this comment.
Do we have any tests covering this custom fallback block?
Any exceptions raised in this code could be very problematic, since it would presumably crash at import. Maybe we should wrap it all in a try block to be safe
There was a problem hiding this comment.
I wrapped this in a try block.
I'm having issues writing a test for this functionality (Gemini was not helpful) so I'm deferring this for now.
daniel-sanche
left a comment
daniel-sanche
left a comment
There was a problem hiding this comment.
LGTM.
I would feel more comfortable if we had tests for the custom fallback code, but having it wrapped in a try block seems good for now
Add a check to generated GAPICs to emit the appropriate usage warnings (most prominently, deprecation messages) for unsupported Python runtime versions and dependency versions.
The intent is that before we stop allowing some old versions of a package dependency, we'll have at least one release that adds a warning saying those versions of the dependency are deprecated, before we actually disallow those versions of the dependency in future versions of the GAPICs.
For Python run-times, once we depend on
google.api_coreafter the run-time checks have been implemented (in googleapis/python-api-core#832), these usage/deprecation warnings will be automatic. All we need to do is update the dates for (future) Python versions hard-coded therein.This PR also contains some fall-back warning code for users who can't upgrade
google.api.corebut can upgrade their GAPICs, so they, too, get the deprecation messages.