feat: Add attempt_direct_path argument to create_channel by parthea · Pull Request #583 · googleapis/python-api-core

parthea · 2024-01-26T20:39:21Z

Fixes#482
b/267782870

vchudnov-g

This is looking good. I only reviewed the sync file and not the tests yet; I'll pick this back up tomorrow, but feel free to start responding to the comments here (or wait if you prefer; all good).

vchudnov-g · 2024-01-31T22:42:11Z

+ else:
+ # Use grpc.compute_engine_channel_credentials in order to support Direct Path.
+ # See https://grpc.github.io/grpc/python/grpc.html#grpc.compute_engine_channel_credentials
+ # TODO(b/323073050): Although `grpc.compute_engine_channel_credentials`


Since this is public code, I would prefer to a public (GitHub) issue in this TODO.

Fixed in e9199a0

vchudnov-g · 2024-01-31T22:43:59Z

+ attempt_direct_path (Optional[bool]): If set, Direct Path will be attempted when
+ the request is made. Direct Path provides a proxyless connection which
+ increases the available throughput, reduces latency, and increases
+ reliability. Outside of GCE, the direct path request may fallback


Do we need to spell out what GCE stands for in public docs? Not everyone looking at this file may be using GCE.

Fixed in e7e3cf7

vchudnov-g · 2024-01-31T22:44:18Z

+ If a `ServiceUnavailable` response is received when the request is sent, it is
+ recommended that the client repeat the request with `attempt_direct_path` set to `False`
+ as the Service may not support Direct Path. Using `ssl_credentials` with `attempt_direct_path`
+ set to `True` will result in `ValueError` as it is not yet supported.


Suggested change
setto`True`willresultin`ValueError`asitisnotyetsupported.
setto`True`willresultin`ValueError`asthiscombinationisnotyetsupported.

Fixed in e7e3cf7

vchudnov-g · 2024-01-31T22:55:17Z

+ attempt_direct_path (Optional[bool]): If set, Direct Path will be attempted when
+ the request is made. Direct Path provides a proxyless connection which
+ increases the available throughput, reduces latency, and increases
+ reliability. Outside of GCE, the direct path request may fallback
+ to DNS if this is configured by the Service. This argument should only
+ be set in a GCE environment and for Services that are known to support Direct Path.
+ If a `ServiceUnavailable` response is received when the request is sent, it is
+ recommended that the client repeat the request with `attempt_direct_path` set to `False`
+ as the Service may not support Direct Path. Using `ssl_credentials` with `attempt_direct_path`
+ set to `True` will result in `ValueError` as it is not yet supported.


"Should only be set in a GCE environment" is confusing in light of "Outside of GCE, the request may fall back". Clarify. I suspect you mean something like this:
Suggested change
attempt_direct_path (Optional[bool]): Ifset, DirectPathwillbeattemptedwhen
therequestismade. DirectPathprovidesaproxylessconnectionwhich
increasestheavailablethroughput, reduceslatency, andincreases
reliability. OutsideofGCE, thedirectpathrequestmayfallback
toDNSifthisisconfiguredbytheService. Thisargumentshouldonly
besetinaGCEenvironmentandforServicesthatareknowntosupportDirectPath.
Ifa`ServiceUnavailable`responseisreceivedwhentherequestissent, itis
recommendedthattheclientrepeattherequestwith`attempt_direct_path`setto`False`
astheServicemaynotsupportDirectPath. Using`ssl_credentials`with`attempt_direct_path`
setto`True`willresultin`ValueError`asitisnotyetsupported.
attempt_direct_path (Optional[bool]): Ifset, DirectPathwillbeattemptedwhen
therequestismade. DirectPathisonlyavailablewithinaGoogleCompute
Engineenvironmentandprovidesaproxylessconnectionwhichincreasesthe
availablethroughput, reduceslatency, andincreasesreliability.
-Thisargumentshouldonly
besetinaGCEenvironmentandforServicesthatareknowntosupportDirect
Path.
-IfthisargumentissetoutsideofGCE, thenthisrequestwillfail
unlesstheback-endservicehappenstohaveconfiguredfall-backtoDNS.
-Iftherequestcausesa`ServiceUnavailable`response, we
recommendthattheclientrepeattherequestwith`attempt_direct_path`setto
`False`astheServicemaynotsupportDirectPath.
-Using`ssl_credentials`with`attempt_direct_path`
setto`True`willresultin`ValueError`asthiscombinationisnotyet
supported.
(And similarly for the async version)
WDYT?

Fixed in e7e3cf7

vchudnov-g · 2024-01-31T22:55:39Z

+ attempt_direct_path (Optional[bool]): If set, Direct Path will be attempted when
+ the request is made. Direct Path provides a proxyless connection which
+ increases the available throughput, reduces latency, and increases
+ reliability. Outside of GCE, the direct path request may fallback


Suggested change
reliability. OutsideofGCE, thedirectpathrequestmayfallback
reliability. OutsideofGCE, thedirectpathrequestmayfallback
but see comment below

Fixed in e7e3cf7

vchudnov-g · 2024-01-31T22:57:33Z

+ target (str): The target service address which is converted into a format compatible with Direct Path.
+ If the target contains `dns:///` or does not have contain `:///`, the target will be converted in
+ a format compatible with Direct Path, otherwise the original target will be returned.


Suggested change
target (str): ThetargetserviceaddresswhichisconvertedintoaformatcompatiblewithDirectPath.
Ifthetargetcontains`dns:///`ordoesnothavecontain`:///`, thetargetwillbeconvertedin
aformatcompatiblewithDirectPath,otherwisetheoriginaltargetwillbereturned.
target (str): ThetargetserviceaddresswhichisconvertedintoaformatcompatiblewithDirectPath.
Ifthetargetcontains`dns:///`ordoesnotcontain`:///`, thetargetwillbeconvertedin
aformatcompatiblewithDirectPath;otherwisetheoriginaltargetwillbereturned.
So the idea is that a :/// (except for dns:///) already denotes Direct Path, so no action is needed, correct? It might be good to be explicit about this assumption.

Done in 44d5845

vchudnov-g · 2024-02-01T01:29:20Z

+ a format compatible with Direct Path, otherwise the original target will be returned.
+ """
+
+ dns_prefix = "dns:///"


It might be helpful to clarify in a comment what the dns_prefix means, which I take it is to be explicit about an endpoint living in the Internet (ie outside GCP).

Fixed in e6ea7ec

vchudnov-g · 2024-02-01T01:36:54Z

 default_scopes=None,
 default_host=None,
 compression=None,
+ attempt_direct_path: Optional[bool] = None,


Suggested change
attempt_direct_path: Optional[bool] =None,
attempt_direct_path: Optional[bool] =False,
Since it's meant to be a Boolean, might as well.

Fixed in 0f3a0e4

vchudnov-g

LGTM! Nothing blocking, but I think some of my suggestions can make things tighter/clearer.

vchudnov-g · 2024-02-01T18:05:41Z

+ # If `ssl_credentials` is set and `attempt_direct_path` is set to `True`,
+ # raise ValueError as this is not yet supported.
+ # See https://github.com/googleapis/python-api-core/issues/590
+ if ssl_credentials is not None and attempt_direct_path:


Could we simplify to just this?:
Suggested change
ifssl_credentialsisnotNoneandattempt_direct_path:
ifssl_credentialsandattempt_direct_path:

Fixed in 60b8acb

vchudnov-g · 2024-02-01T18:10:47Z

 "Compression argument is being ignored for grpc_gcp.secure_channel creation."
 )
+ if attempt_direct_path:
+ warnings.warn(


For consistency, the _LOGGER.debug in the previous lines should probably also become a warnings.warn

Fixed in 1a7578d and d96499f

vchudnov-g · 2024-02-01T18:13:31Z

+ Given a target, return a modified version which is compatible with Direct Path.
+
+ Args:
+ target (str): The target service address in the format 'hostname:port', 'dns://hostname' or other


Suggested change
target (str): Thetargetserviceaddressintheformat'hostname:port', 'dns://hostname'orother
target (str): Thetargetserviceaddressintheformat'hostname[:port]', 'dns://hostname[:port]'orother

Fixed in af3a21b

vchudnov-g · 2024-02-01T18:13:56Z

+
+ Args:
+ target (str): The target service address in the format 'hostname:port', 'dns://hostname' or other
+ compatible format.


"other compatible format": link to a place that lists compatible formats, or remove this phrase which I think is puzzling by itself.

Removed in af3a21b

vchudnov-g · 2024-02-01T18:21:07Z

+ direct_path_prefix = ":///"
+ if direct_path_prefix not in target:


Suggested change
direct_path_prefix=":///"
ifdirect_path_prefixnotintarget:
direct_path_separator=":///"
ifdirect_path_separatornotintarget:

Fixed in c0052e6

vchudnov-g · 2024-02-01T18:39:08Z

 )


+def _modify_target_for_direct_path(target: str) -> str:


Q: In general, an API endpoint could depend on the URL path and not just the host+port. Is this true of Google APIs? If so, where do we deal with the path part of the URI? (I realize these functions specify only host+port as inputs, so it's clear what they expect and they're doing the right thing, but I was wondering about this more general question.)

If so, where do we deal with the path part of the URI?
I believe this is part of the transcode method
Transcodes a grpc request pattern into a proper HTTP request following the rules outlined here,
python-api-core/google/api_core/path_template.py
Lines 250 to 259 in b72929f
deftranscode(http_options, message=None, **request_kwargs):
"""Transcodes a grpc request pattern into a proper HTTP request following the rules outlined here,
https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L44-L312
Args:
http_options (list(dict)): A list of dicts which consist of these keys,
'method' (str): The http method
'uri' (str): The path template
'body' (str): The body field name (optional)
(This is a simplified representation of the proto option `google.api.http`)
https://github.com/googleapis/gapic-generator-python/blob/b4e6cfc0be8594e2575910699389600f5e92e552/gapic/templates/%25namespace/%25name_%25version/%25sub/services/%25service/transports/rest.py.j2#L372

vchudnov-g · 2024-02-01T18:45:33Z

 import functools

-from typing import Generic, Iterator, AsyncGenerator, TypeVar
+from typing import AsyncGenerator, Generic, Iterator, Optional, TypeVar


General notes:
The comments from the sycn version apply here too
Just a note for the future (not blocking this PR): a lot of the code seems identical or very similar to the sync code. We should consolidate sensibly to eliminate duplication. (I just filed #593 to track more such instances)

Thanks for opening that issue! I also made changes to the async code based on the sync code feedback.

vchudnov-g · 2024-02-01T18:47:05Z

-def test_create_channel_implicit(grpc_secure_channel, default, composite_creds_call):
+def test_create_channel_implicit(
+ grpc_secure_channel,
+ default,


default is too generic a name. I assume this is meant to take the google.auth.default you have patched in the decorators. Is there any way this parameter name could be made more descriptive (like auth-default, say), or does @mock preclude that?
(I realize this was pre-existing, so no need to spend too much time on this. But if it's easy...)

Fixed in d63402e

vchudnov-g · 2024-02-01T19:05:21Z



-@mock.patch("grpc.composite_channel_credentials")
+@pytest.mark.parametrize(


Would it be worth making this test function take a target parameter, and then parametrizing it as we do for test_create_channel_implicit_with_default_host below? target seems to be used the same way in both places, and this would allow us to check here the dns:/// and another-c2p:/// cases you parametrize below.

Done in 396d8fa

vchudnov-g · 2024-02-01T19:11:24Z



-@mock.patch("grpc.composite_channel_credentials")
+@pytest.mark.parametrize(


Same comments as in sync version apply here.

Done in 396d8fa

product-auto-labelBot added the size: m Pull request size is medium. label Jan 26, 2024

parthea force-pushed the add-support-for-direct-path branch 5 times, most recently from 676ca58 to e777b53Compare January 31, 2024 00:56

product-auto-labelBot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jan 31, 2024

parthea force-pushed the add-support-for-direct-path branch from e777b53 to 0e974b6Compare January 31, 2024 01:09

parthea mentioned this pull request Jan 12, 2026
Add support for using ssl_credentials and attempt_direct_path in google.api_core.grpc_helpers.create_channelgoogleapis/google-cloud-python#15061
Closed

parthea force-pushed the add-support-for-direct-path branch 2 times, most recently from c4f537d to 257d4dfCompare January 31, 2024 01:22

feat: Add attempt_direct_path argument to create_channel
421becd

parthea force-pushed the add-support-for-direct-path branch from 1d40546 to 421becdCompare January 31, 2024 01:28

parthea added 2 commits January 30, 2024 20:28

Merge branch 'main' into add-support-for-direct-path
65b994e

add more test cases
b1010ea

parthea marked this pull request as ready for review January 31, 2024 01:33

parthea requested review from a teamJanuary 31, 2024 01:33

parthea assigned vchudnov-g Jan 31, 2024

parthea added 2 commits January 31, 2024 20:03

fix docstring
9a1957a

fix docstring
b223b85

vchudnov-g reviewed Feb 1, 2024
View reviewed changes

vchudnov-g approved these changes Feb 1, 2024
View reviewed changes

parthea added 6 commits February 4, 2024 14:54

update docstring of attempt_direct_path arg
e7e3cf7

update docstring of target arg
44d5845

Add comment for dns_prefix local variable
e6ea7ec

Set the default value of attempt_direct_path to False
0f3a0e4

simplify conditional statement
60b8acb

use warnings.warn instead of _LOGGER.debug
1a7578d

parthea added 4 commits February 4, 2024 15:06

update docstring of target arg in _modify_target_for_direct_path
af3a21b

s/direct_path_prefix/direct_path_separator
c0052e6

default->google_auth_default
d63402e

parametrize target in def test_create_channel_implicit
396d8fa

parthea force-pushed the add-support-for-direct-path branch from b4a6edf to 396d8faCompare February 4, 2024 17:48

Merge branch 'main' into add-support-for-direct-path
d3598ee

parthea mentioned this pull request Feb 4, 2024
Add ability to detect Google Compute Engine Environment in _create_composite_credentials#598
Closed

parthea added 3 commits February 4, 2024 17:53

Add github issue for TODO
e9199a0

filter deprecation warning related to grpcio-gcp
d96499f

format docstring
d41ca10

parthea merged commit 94726e7 into mainFeb 4, 2024

parthea deleted the add-support-for-direct-path branch February 4, 2024 20:00

release-pleaseBot mentioned this pull request Feb 4, 2024
chore(main): release 2.17.0 #599
Merged

This was referenced May 30, 2025
chore(main): release 2.0.0 #822
Closed
chore(main): release 2.0.0 #823
Closed

	setto`True`willresultin`ValueError`asitisnotyetsupported.
	setto`True`willresultin`ValueError`asthiscombinationisnotyetsupported.

-attempt_direct_path (Optional[bool]): Ifset, DirectPathwillbeattemptedwhen
-therequestismade. DirectPathprovidesaproxylessconnectionwhich
-increasestheavailablethroughput, reduceslatency, andincreases
-reliability. OutsideofGCE, thedirectpathrequestmayfallback
-toDNSifthisisconfiguredbytheService. Thisargumentshouldonly
-besetinaGCEenvironmentandforServicesthatareknowntosupportDirectPath.
-Ifa`ServiceUnavailable`responseisreceivedwhentherequestissent, itis
-recommendedthattheclientrepeattherequestwith`attempt_direct_path`setto`False`
-astheServicemaynotsupportDirectPath. Using`ssl_credentials`with`attempt_direct_path`
-setto`True`willresultin`ValueError`asitisnotyetsupported.
+attempt_direct_path (Optional[bool]): Ifset, DirectPathwillbeattemptedwhen
+therequestismade. DirectPathisonlyavailablewithinaGoogleCompute
+Engineenvironmentandprovidesaproxylessconnectionwhichincreasesthe
+availablethroughput, reduceslatency, andincreasesreliability.
+-Thisargumentshouldonly
+besetinaGCEenvironmentandforServicesthatareknowntosupportDirect
+Path.
+-IfthisargumentissetoutsideofGCE, thenthisrequestwillfail
+unlesstheback-endservicehappenstohaveconfiguredfall-backtoDNS.
+-Iftherequestcausesa`ServiceUnavailable`response, we
+recommendthattheclientrepeattherequestwith`attempt_direct_path`setto
+`False`astheServicemaynotsupportDirectPath.
+-Using`ssl_credentials`with`attempt_direct_path`
+setto`True`willresultin`ValueError`asthiscombinationisnotyet
+supported.

	reliability. OutsideofGCE, thedirectpathrequestmayfallback
	reliability. OutsideofGCE, thedirectpathrequestmayfallback

-target (str): ThetargetserviceaddresswhichisconvertedintoaformatcompatiblewithDirectPath.
-Ifthetargetcontains`dns:///`ordoesnothavecontain`:///`, thetargetwillbeconvertedin
-aformatcompatiblewithDirectPath,otherwisetheoriginaltargetwillbereturned.
+target (str): ThetargetserviceaddresswhichisconvertedintoaformatcompatiblewithDirectPath.
+Ifthetargetcontains`dns:///`ordoesnotcontain`:///`, thetargetwillbeconvertedin
+aformatcompatiblewithDirectPath;otherwisetheoriginaltargetwillbereturned.

	attempt_direct_path: Optional[bool] =None,
	attempt_direct_path: Optional[bool] =False,

	ifssl_credentialsisnotNoneandattempt_direct_path:
	ifssl_credentialsandattempt_direct_path:

	target (str): Thetargetserviceaddressintheformat'hostname:port', 'dns://hostname'orother
	target (str): Thetargetserviceaddressintheformat'hostname[:port]', 'dns://hostname[:port]'orother

		direct_path_prefix = ":///"
		if direct_path_prefix not in target:

-direct_path_prefix=":///"
-ifdirect_path_prefixnotintarget:
+direct_path_separator=":///"
+ifdirect_path_separatornotintarget:

	deftranscode(http_options, message=None, **request_kwargs):
	"""Transcodes a grpc request pattern into a proper HTTP request following the rules outlined here,
	https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L44-L312

	Args:
	http_options (list(dict)): A list of dicts which consist of these keys,
	'method' (str): The http method
	'uri' (str): The path template
	'body' (str): The body field name (optional)
	(This is a simplified representation of the proto option `google.api.http`)



		@mock.patch("grpc.composite_channel_credentials")
		@pytest.mark.parametrize(



		@mock.patch("grpc.composite_channel_credentials")
		@pytest.mark.parametrize(

Uh oh!

Conversation

parthea commented Jan 26, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

vchudnov-gJan 31, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

parthea commented Jan 26, 2024•
edited
Loading

vchudnov-gJan 31, 2024•
edited
Loading