chore: add `async_rest` extra for async rest dependencies #2195

ohmayr · 2024-09-24T21:59:40Z

Fixes #2187 🦕

gapic/templates/testing/_default_constraints.j2

gapic/templates/testing/constraints-3.7.txt.j2

parthea · 2024-09-26T00:52:55Z

gapic/templates/testing/_default_constraints.j2

+google-auth
+# from google-auth[aiohttp]
+aiohttp
+requests


We shouldn't include the async_rest extra in all constraints because we won't have any tests without the async_rest extra installed.

I'm not sure this is true. The nox documentation says "[session.install] invokes pip to install packages inside of the session’s virtualenv." We use session.install like this, specifying -c CONSTRAINTS_FILE. The -c constraints files option for pip is documented as saying (emphasis added) "Constraints files are requirements files that only control which version of a requirement is installed, not whether it is installed or not" .

So in other words, removing async_rest from the constraints will not do what we want. It doesn't matter whether we have it there or not, as long as we make sure that we do not install the extras. So I would argue it's better to not split the constraints files, but to have one test suite that installs the async_rest extra and one test suite that doesn't, both with the same constraints file.

Does this make sense? LMK if I've misunderstood something.

addressed. See my other comment and discussing this over our offline chat.

parthea · 2024-09-26T00:54:22Z

gapic/templates/testing/constraints-3.7.txt.j2

+aiohttp==3.6.2
+requests==2.20.0


Consider creating a separate constraints file so that we know that the client library without the async_rest extra works. See googleapis/python-api-core#701 where we have a separate constraints file testing/constraints-async-rest-3.7.txt and separate nox session for testing async_rest

See comment above.

parthea · 2024-09-26T00:55:28Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest_asyncio.py.j2

 {# NOTE: `pragma: NO COVER` is needed since the coverage for presubmits isn't combined. #}
 except ImportError as e:  # pragma: NO COVER
-    {# TODO(https://github.com/googleapis/google-auth-library-python/pull/1577): Update the version of google-auth once the linked PR is merged. #}
-    raise ImportError("async rest transport requires google-auth >= 2.35.0 with aiohttp extra. Install google-auth with the aiohttp extra using `pip install google-auth[aiohttp]==2.35.0`.") from e
+    raise ImportError("`rest_asyncio` transport requires the library to be installed with the `async_rest` extra. Install the library with the `async_rest` extra using `pip install <insert_library_name>[async_rest]`") from e


Can we use {{ api.naming.warehouse_package_name }} instead of <insert_library_name>?

tests/integration/goldens/redis/tests/unit/gapic/redis_v1/test_cloud_redis.py

tests/integration/goldens/redis/setup.py

...egration/goldens/redis/google/cloud/redis_v1/services/cloud_redis/transports/rest_asyncio.py

gapic/templates/setup.py.j2

vchudnov-g · 2024-09-27T17:51:29Z

gapic/templates/testing/_default_constraints.j2

+google-auth
+# from google-auth[aiohttp]
+aiohttp
+requests


I'm not sure this is true. The nox documentation says "[session.install] invokes pip to install packages inside of the session’s virtualenv." We use session.install like this, specifying -c CONSTRAINTS_FILE. The -c constraints files option for pip is documented as saying (emphasis added) "Constraints files are requirements files that only control which version of a requirement is installed, not whether it is installed or not" .

So in other words, removing async_rest from the constraints will not do what we want. It doesn't matter whether we have it there or not, as long as we make sure that we do not install the extras. So I would argue it's better to not split the constraints files, but to have one test suite that installs the async_rest extra and one test suite that doesn't, both with the same constraints file.

Does this make sense? LMK if I've misunderstood something.

vchudnov-g · 2024-09-27T17:56:54Z

gapic/templates/testing/constraints-3.7.txt.j2

+aiohttp==3.6.2
+requests==2.20.0


See comment above.

vchudnov-g · 2024-09-27T18:19:48Z

noxfile.py

            constraints_path = str(
-            f"{tmp_dir}/testing/constraints-{session.python}.txt"
+            f"{tmp_dir}/testing/constraints-{constraints_type}{session.python}.txt"


A note: unlike what I remarked on another comment thread in this PR, where we're using the constraints files as -c arguments to PIP (which hence do not control whether those dependencies are installed), here we are passing the file via -r (three lines below) thus making it into a requirements file where the listed dependencies are installed. AFAICT, we do not use -r for the (same??!) constraints files under templates/.

We should be clear about the semantics and how we are using the files. I'd suggest for the purposes of this file, creating proper requirements files that do list what will be installed.

Removed the constraints file.

We're infact installing the constraints file with a -r flag which is why this wasn't an issue here (at least at the generator layer). Anyways, this has been addressed by installing the deps with the extra.

vchudnov-g · 2024-09-27T18:25:39Z

gapic/templates/setup.py.j2

@@ -94,6 +98,9 @@ setuptools.setup(
    packages=packages,
    python_requires=">=3.7",
    install_requires=dependencies,
+{% if rest_async_io_enabled %}
+    extras_require=extras,


So if this wasn't there before, why weren't some tests failing? This would suggest a failure in our tests. At the GAPIC level, the test should silently pass if the GAPIC was generated without REST transport or installed without the extra. But at the generator level, we want to ensure that the tests with the extras pass, so how can we be notified if we run a test suite where we expect the extra and the associated tests, but it turns out the extra wasn't installed?

Tests weren't failing because we're not using the async_rest extra to install the dependencies. We've explicitly defined each required dependency within the constraints file.

noxfile.py

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2

noxfile.py

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2

product-auto-label bot added the size: s Pull request size is small. label Sep 24, 2024

parthea reviewed Sep 25, 2024

View reviewed changes

gapic/templates/testing/_default_constraints.j2 Outdated Show resolved Hide resolved

gapic/templates/testing/constraints-3.7.txt.j2 Outdated Show resolved Hide resolved

feat: add async rest option in setup

a6c1789

ohmayr force-pushed the add-async-rest-option-in-setup branch from 25c7c19 to a6c1789 Compare September 25, 2024 16:13

ohmayr added 2 commits September 25, 2024 17:23

address PR feedback

17a1c5f

update exception statements

a468255

product-auto-label bot added size: l Pull request size is large. and removed size: s Pull request size is small. labels Sep 25, 2024

reorder imports

3ec9135

ohmayr marked this pull request as ready for review September 25, 2024 21:13

ohmayr requested a review from a team as a code owner September 25, 2024 21:13

blunderbuss-gcf bot assigned vchudnov-g Sep 25, 2024

ohmayr requested a review from vchudnov-g September 25, 2024 21:14

minor fix in import statement

df3ff6f

ohmayr changed the title ~~feat: add async rest option in setup~~ feat: add async rest option in setup config Sep 25, 2024

parthea reviewed Sep 26, 2024

View reviewed changes

vchudnov-g reviewed Sep 26, 2024

View reviewed changes

ohmayr added 4 commits September 27, 2024 15:53

address PR feedback

7c4679e

update constraints for async rest

4247f57

update noxfile

50dc716

add whitespace

a75bfce

ohmayr changed the title ~~feat: add async rest option in setup config~~ chore: add async_rest extra for async rest dependencies Sep 27, 2024

vchudnov-g mentioned this pull request Sep 27, 2024

chore: add async_rest extra for async rest dependencies googleapis/python-api-core#701

Merged

vchudnov-g reviewed Sep 27, 2024

View reviewed changes

ohmayr added 5 commits September 27, 2024 18:57

remove constraints file

fcc4982

revert noxfile

1160ace

use extra to install the required deps

1214787

update noxfile for gapics

4607716

avoid conflict with 3.7 constraints

56e9f05

ohmayr added 2 commits September 27, 2024 20:02

update noxfile

715926a

address PR feedback

6533c77

vchudnov-g approved these changes Sep 27, 2024

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 Outdated Show resolved Hide resolved

noxfile.py Show resolved Hide resolved

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 Outdated Show resolved Hide resolved

address PR feedback

9a370b5

ohmayr merged commit 58191fd into async-rest-support-in-gapics Sep 30, 2024
73 checks passed

ohmayr deleted the add-async-rest-option-in-setup branch September 30, 2024 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add `async_rest` extra for async rest dependencies #2195

chore: add `async_rest` extra for async rest dependencies #2195

ohmayr commented Sep 24, 2024 •

edited

Loading

parthea Sep 26, 2024

ohmayr Sep 27, 2024

vchudnov-g Sep 27, 2024

ohmayr Sep 27, 2024

parthea Sep 26, 2024

ohmayr Sep 27, 2024

vchudnov-g Sep 27, 2024

parthea Sep 26, 2024

ohmayr Sep 27, 2024

vchudnov-g Sep 27, 2024

vchudnov-g Sep 27, 2024

vchudnov-g Sep 27, 2024

ohmayr Sep 27, 2024

vchudnov-g Sep 27, 2024

ohmayr Sep 27, 2024

		aiohttp==3.6.2
		requests==2.20.0

		aiohttp==3.6.2
		requests==2.20.0

chore: add async_rest extra for async rest dependencies #2195

chore: add async_rest extra for async rest dependencies #2195

Conversation

ohmayr commented Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chore: add `async_rest` extra for async rest dependencies #2195

chore: add `async_rest` extra for async rest dependencies #2195

ohmayr commented Sep 24, 2024 •

edited

Loading