-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Core: Add client options. #8265
Conversation
@@ -142,6 +144,9 @@ def __init__( | |||
API requests. If ``None``, then default info will be used. | |||
Generally, you only need to set this if you're developing | |||
your own client library. | |||
client_options (google.api_core.client_options.ClientOptions): | |||
Client options used to set user options on the client. API Endpoint | |||
should be set through client_options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be very confusing for users: we have three similarly-named arguments, one of which is deprecated, all of which exist to allow users to override bits of the default configuration for the client. Could weadd the api_endpoint
attribute to google.api_core.client_info.ClientInfo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This client options class is designed to be the central place for client-level user configuration, not just for endpoints. We are planning to add credentials, pre-defined headers and other stuff.
I agree that right now it seems like we have 'client_config', 'client_info' and 'client_options' which all look the same. Is it possible that we merge these similar classes into one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can look into rolling the others into client_options, since that is the "new" name that's going to be used across clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@busunkim96 I don't think you've addressed the confusion factor here: why do we want people to configure the client using both client_info
and client_options
(leaving aside my never-answered question of why client_config
is deprecated)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct me if any of this isn't correct, but I think:
client_config
-> this is about gRPC timeout/retry configuration
client_info
-> this should probably named user_agent_info
or something more accurate. This is the data attached to requests to identify the particular client better.
client_options
-> this new property bag, currently for endpoints. Eventually also additional things.
It is possible user_agent_info
should also be under client_options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your description of the three arguments roles is correct. I'm arguing that three is two too many. Users should only need to thing about one thing to pass in to customize the behavior of the client.
@lukesneeringer Can you please comment on why client_config
is deprecated? How else would users be expected to tweak e.g. gRPC timeouts for client-invoked methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a link to the extensible options in Python doc that should be accessible to all of [email protected]. It has more context about Client Options and what we're looking to add to it in the future.
The goal of client_options
is to have a more straight-forward way to configure clients. We want to provide a consistent interface to set options across the generated, handwritten, and Apiary clients. The experience will also be more predictable across languages.
User agent is on the list of things we would like to be configurable with client_options
. I think we want to move what is in client_info
(user agent) into client_options
. In the near term users will be able to use both, with the user_agent in client_options
taking precedence. We will deprecate client_info
eventually.
Luke's response as to why client_config was deprecated:
The original rationale for deprecating that was that the client options were grpc specific and may not work well with other transports.
The idea was to use the transport itself as the replacement. Based on a request from Chris recently, however, we might need to rethink that, possibly by allowing folks to specify the arguments to the transport that are passed through (without having to instantiate the transport themselves).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. I left some comments.
@@ -142,6 +144,9 @@ def __init__( | |||
API requests. If ``None``, then default info will be used. | |||
Generally, you only need to set this if you're developing | |||
your own client library. | |||
client_options (google.api_core.client_options.ClientOptions): | |||
Client options used to set user options on the client. API Endpoint | |||
should be set through client_options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This client options class is designed to be the central place for client-level user configuration, not just for endpoints. We are planning to add credentials, pre-defined headers and other stuff.
I agree that right now it seems like we have 'client_config', 'client_info' and 'client_options' which all look the same. Is it possible that we merge these similar classes into one?
CC @wora |
options (dict): A dictionary with client options. | ||
""" | ||
|
||
client_options = ClientOptions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would allow users to do something like:
client = pubsub_v1.PublisherClient(client_options={"api_endpoint": "...", "user_project": "..."})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm satisfied with this direction, given the design doc has been approved and we now have a plan for all these "options, info, config" objects.
Thanks for reviewing this Tim. |
This just adds API Endpoint (but more options will be coming, like credentials).
I manually edited the IoT client to show what changes need to be made to the generated clients. I will undo those changes before merging.
go/extensible-options-in-python <-- shared with [email protected]
go/extensible-options-in-client
CC @shinfan