This line of code is invalid for proto repeated fields, which is what we're attempting to set as the sub-response field for bundling responses. This is the root cause of googleapis/google-cloud-python#1869.

We did not catch this earlier, because we are testing with named tuples. As part of this fix, we might want to switch our tests to use a simple proto object.

From this discussion

Referring to https://doughellmann.com/blog/2012/11/12/the-performance-impact-of-using-dict-instead-of-in-cpython-2-7-2/

• This is similar to the issue fixed in #40 for _page_streamable
• here the fix is similar, except that kwargs need to passed through to the bundling.Executor.schedule
  • for a given bundling.Task, assume that only the **kwargs for the first call to bundling.Executor.schedule are used. This is exactly how request instances are treated.

The problem is here

This does not work for protocol buffers; repeated fields in protocol buffers must be added by extending the existing collection.

We should make it easier to override ADC; currently the only possible way to do this is to construct a custom Channel object and pass it in to the API wrapper's constructor.

Java GAX already supports this.

(FYI: @garrettjonesgoogle @tbetbetbe)

Tried to instantiate PathTemplate, but it fails with the following stack trace:

ERROR:root:match error: impossible match
Traceback (most recent call last):
File "example.py", line 14, in run
project_name = PubApi.project_path(project_id)
File "/usr/local/google/home/jgeiger/xnewgitonborg/gapi-dev/gapi-pubsub-python/gcloud/pubsub/publisher_api.py", line 57, in project_path
return cls._PROJECT_PATH_TEMPLATE.instantiate({'project': project, })
File "/usr/local/google/home/jgeiger/xnewgitonborg/gapi-dev/gapi-pubsub-python/.tox/py27/local/lib/python2.7/site-packages/google/gax/path_template.py", line 125, in
instantiate
self.match(path)
File "/usr/local/google/home/jgeiger/xnewgitonborg/gapi-dev/gapi-pubsub-python/.tox/py27/local/lib/python2.7/site-packages/google/gax/path_template.py", line 170, in
match
raise ValidationException('match error: impossible match')
ValidationException: match error: impossible match

After #94, the snippet is wrong, it still refers to page_streaming

• messages.py will soon be part of the vgened python code
• initially it will just extend the protobuf messages, and provide docstrings that document all the properties
  and the message itself

Proposal:
update the individual messages classes so that behave like namedtuples by default, and remove clutter from the protobuf message. This provides a much more usable return object to vgen api calls.

Implementation Details:
We might do this with a metaclass, or with code-gen if that's simple.
CallOptions would be updated to have a flag indicating what type of response should be returned

oneof renders as null
map renders as list[value type]
an enum nested inside a message may render incorrectly enum.value vs message_name.enum.value

Change or remove 'src/api/google/logging/v2/logging.proto for documentation'

From this discussion

From this discussion

From this discussion

Make all samples self contained to include the import for pubsub pb2

• update the package dependency
• this involves a surface change
• update documentation around installation, which should be much simpler
• this should allow testing on travis

After using an API instance at the Python REPL prompt, I try to exit Python using Ctrl-D: the process hangs indefinitely, until I interrupt with Ctrl-C, at which point I get the following traceback:

^CError in atexit._run_exitfuncs:
Traceback (most recent call last):
  File "/opt/Python-2.7.11/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
    func(*targs, **kargs)
  File "/home/tseaver/projects/agendaless/Google/src/gcloud-python/.tox/py27/lib/python2.7/site-packages/concurrent/futures/thread.py", line 39, in _python_exit
    t.join(sys.maxint)
  File "/opt/Python-2.7.11/lib/python2.7/threading.py", line 951, in join
    self.__block.wait(delay)
  File "/opt/Python-2.7.11/lib/python2.7/threading.py", line 359, in wait
    _sleep(delay)
KeyboardInterrupt
Error in sys.exitfunc:
Traceback (most recent call last):
  File "/opt/Python-2.7.11/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
    func(*targs, **kargs)
  File "/home/tseaver/projects/agendaless/Google/src/gcloud-python/.tox/py27/lib/python2.7/site-packages/concurrent/futures/thread.py", line 39, in _python_exit
    t.join(sys.maxint)
  File "/opt/Python-2.7.11/lib/python2.7/threading.py", line 951, in join
    self.__block.wait(delay)
  File "/opt/Python-2.7.11/lib/python2.7/threading.py", line 359, in wait
    _sleep(delay)
KeyboardInterrupt

I then have to Ctrl-C again to get back to the shell prompt.

#28 introduces two pylint disable no-member directives; the errors that are suppressed seem to be spurious (given that the tests pass) but should still be investigated.

Currently, the retry_params timeout overrides the passed-in timeout even for calls with no retry_codes, i.e., calls which should not be treated as retrying.

Single backticks in parameter comments need to be doubled for sphinx doc generation.

During the usability study, participants intuitively understood the role of the CallOptions parameter, and its implications on their API requests.

However, for those that investigated further, it was not clear what the default CallOptions setting were, and thus it was difficult to predict the behavior of their requests without first manually setting the options.

Recommendation: Ensure that default CallOptions are accurately and clearly documented. If default options differ by API request, consider adding the defaults to each method’s reference documentation, so user can accurately predict the behavior of their request.

/cc @rok987
/cc @andrewmacvean
/cc @tbetbetbe

What:
Use datetime.timedelta for the gRPC timeout and the bundling delay_threshold. Currently we use numeric values, but we should additionally support Pythonic datetime.timedelta objects.

Why:
Be more Pythonic.

Some rpcs return google.protobuf.Empty

The generated wrapper call for these rpcs should have no return value.

We've agreed that the bundling request size is to be computed by summing the bundled field sizes, with a 10% buffer to account for the underapproximation of the total request size. We should compute the bundled field size correctly, and add the buffer in.

https://github.com/googleapis/gax-python/blob/master/google/gax/bundling.py#L335 assumes that the self.timer will be reset to None after it fires, but we don't actually do that. This causes indefinite hangs.

See discussion.

RetryOptions currently silently overrides timeout if both are specified in CallOptions. This should not be valid.

These two objects really can just be one -- they are always used side-by-side in the codebase.

At the moment, the generated code uses raw sphinx directives.
Google-style docstrings are easier to maintain.

As observed during the informal internal usability bug bash, the linux installation doesn't work without virtual environments. Please update the installation instructions in all environments to use virtual environments.

As of #17, a gax-python has a naive implementation that just uses str(...) to compute a length of the message field.

This is incorrect, it'd be more correct to coerce the message field to a bytearray and take the length of that. Before correcting it, it'd be useful to establish if the bytesize checked by the threshold refers to the overall aggregate message size or just the total size of the repeated bundled fields.

This should prevent problems similar to the one that led to googleapis/gax-nodejs#18

Proposal: create a tox environment that runs pylint on the google/gax directory with only requirements.txt (not test-requirements.txt) installed.

Currently we have CallSettings and CallOptions, and it's hard to say their roles and the differences.

Actually CallSettings is the settings (or configurations) for an API call (or a method), such as the page descriptors or bundling descriptors, retry parameters.

On the other hand, CallOptions is the optional data to modify the behavior of individual invocation of the methods.

To me,

• CallSettings isn't a very clear name. 'Call-' prefix sounds like per-call (per-invocation) thing. Maybe, MethodSettings?
• the 'merge' method to create a new call settings doesn't look a good design after the redesign of create_api_call. I feel like CallSettings (or MethodSettings) should hold the default CallOptions instance, and CallOptions instance should have the merge() method to create the actual options for the invocation.

Thoughts?

Update all example values for project names to be domain-less ‘my-project’ and not ‘google.com:my-project’

For each method, the generated docs currently says that a GaxError is raised if the RPC is aborted. It should instead document:

a) What GAX errors are commonly raised by this method
b) What is the underlying meaning of the error

Also, consider adding a common page for aggregating all types of GAX errors which provide the information on:

a) What GAX errors exist
b) What is the underlying meaning of each error.

/cc @rok987
/cc @andrewmacvean
/cc @tbetbetbe

Installation on linux is slow - both with and without virtual environments

The new language-independent client config uses CamelCase method names, but GAX expects snake_case names that match the method names in the API object.

From this discussion

Bundling tests should have caught #113. We should add tests to cover the full lifespan of a delay threshold-triggered bundle, not just the construction of the timer object.

See discussion

@tbetbetbe Please add the code-generation label for me (I don't think I have privileges).

message_count_threshold => element_count_threshold
message_bytesize_threshold => request_byte_threshold

This would bring us in line with the more precise terminology used by the code generator.

Instead of

 def __init__(self,
              service_path=SERVICE_ADDRESS,
              port=DEFAULT_SERVICE_PORT,
              channel=None,
              ssl_creds=None,
              scopes=_ALL_SCOPES,
              retrying_override=None,
              bundling_override=None,
              timeout=30,
              app_name=None,
              app_version=None):

do

  _GAX_VERSION  = pkg_resource.get_distribution('google.gax').version # a class constant
  ...

 def __init__(self,
              service_path=SERVICE_ADDRESS,
              port=DEFAULT_SERVICE_PORT,
              channel=None,
              ssl_creds=None,
              scopes=None,
              retrying_override=None,
              bundling_override=None,
              timeout=30,
              app_name='gax',
              app_version=_GAX_VERSION):
  ...
      if scopes is None:
          scopes = self._ALL_SCOPES

Early feedback suggests that in addition to simple iteration, a batch mode of operation should also be supported for page-streamed calls, where the user is able to iterate by batches of results. E.g,

E.g, currently for page streaming the following works

>>> from google.pubsub.v1.publisher_api import PublisherApi
>>> api = PublisherApi()
>>> topic = api.topic_path('google.com:pubsub-demo', 'my-topic')
>>> all_subs = list(api.list_topic_subscriptions(topic))
>>> all_subs
[‘sub_1’, ‘sub_2’, ‘sub_3’, ‘sub_2’]

There should be a way to support batching:

>>> from google.pubsub.v1.publisher_api import PublisherApi
>>> api = PublisherApi()
>>> topic = api.topic_path('google.com:pubsub-demo', 'my-topic')
>>> all_subs = list(??? with batchsize=2)
>>> all_subs
[(‘sub_1’, ‘sub_2’), (‘sub_3’, ‘sub_2’)]

Proposals

1. Add a helper to google.gax to support batch iteration. Users will import it and use it when they need it.
2. Add a helper to google.gax as in 1. Also, add a field to CallOptions, batch_size which users set to batch in pages

Add a helper to google.gax to support batch iteration

>>> from google.pubsub.v1.publisher_api import PublisherApi
>>> from google.gax import batch_iter
>>> api = PublisherApi()
>>> topic, batch_size = api.topic_path('google.com:pubsub-demo', 'my-topic'), 2
>>> all_subs = list(batch_iter(api.list_topic_subscriptions(topic), batch_size))
>>> all_subs
[(‘sub_1’, ‘sub_2’), (‘sub_3’, ‘sub_2’)]

Pros

• the generated code stays simple, there are no changes required to the generator
• the new feature batch_iter is not difficult, and is reminiscent of the helper methods in the itertools library

Cons

• users have more of the gax-python surface to learn; the batch_iter func is a new feature to be learned.

Add a helper to google.gax, also add batch_size to CallOptions

>>> from google.pubsub.v1.publisher_api import PublisherApi
>>> from google.gax import batch_iter
>>> api = PublisherApi()
>>> topic, batch_size = api.topic_path('google.com:pubsub-demo', 'my-topic'), 2
>>> all_subs = list(api.list_topic_subscriptions(topic, 
...    options=CallOptions(batch_size=batch_size)))
>>> all_subs
[(‘sub_1’, ‘sub_2’), (‘sub_3’, ‘sub_2’)]

Pros

• keeps the change to the surface elements the user needs to know about to a minimum; i.e to use this feature, the user only needs to know about an addition field that we will document in CallOptions

Cons

• the generated code for methods that support page-streaming is slightly more complex

@jmuk, @geigerj, @bjwatson, @anthmgoogle PTAL and discuss

Decision

• the surface will look like this

>>> from google.pubsub.v1.publisher_api import PublisherApi
>>> api = PublisherApi()
>>> api.topic_path('google.com:pubsub-demo', 'my-topic')
>>> all_subs = list(api.list_topic_subscriptions(topic, 
...    options=CallOptions(is_page_streaming=False)))
>>> all_subs  # whatever the server page size is 
[(‘sub_1’, ‘sub_2’), (‘sub_3’), ('sub_4')]

There's an open question that is OK to resolve after implementation begins

I get the below warnings at runtime when I use the pubsub vkit libraries. Is there any way to not get these? I am using gax-google-pubsub-v1 version 1.0.11 on mac El Capitan.

/Users/vsubramani/Documents/Projects/VTKSample/vtk/lib/python2.7/site-packages/google/pubsub/v1/pubsub_pb2.py:1168: DeprecationWarning: the alpha API (includes this package) is deprecated, unmaintained, and no longer tested. Please migrate to the beta API.
from grpc.early_adopter import implementations as early_adopter_implementations
/Users/vsubramani/Documents/Projects/VTKSample/vtk/lib/python2.7/site-packages/grpc/_adapter/fore.py:39: DeprecationWarning: the alpha API (includes this package) is deprecated, unmaintained, and no longer tested. Please migrate to the beta API.
from grpc.framework.base import interfaces as base_interfaces
/Users/vsubramani/Documents/Projects/VTKSample/vtk/lib/python2.7/site-packages/grpc/early_adopter/implementations.py:36: DeprecationWarning: the alpha API (includes this package) is deprecated, unmaintained, and no longer tested. Please migrate to the beta API.
from grpc.framework.alpha import _face_utilities
/Users/vsubramani/Documents/Projects/VTKSample/vtk/lib/python2.7/site-packages/grpc/framework/alpha/_face_utilities.py:35: DeprecationWarning: the alpha API (includes this package) is deprecated, unmaintained, and no longer tested. Please migrate to the beta API.
from grpc.framework.face import interfaces as face_interfaces # pylint: disable=unused-import

Probably should have the same behavior as proto, i.e., missing string field == ""; missing numerical field == 0

Instead of creating an api_call every time there is a call, create an api_callable object for each API method in the constructor, and pass the request and options to that api_callable. For example, existing implementation of publisher_api.create_topic:

        req = pubsub_pb2.Topic(name=name)
        settings = self._defaults['create_topic'].merge(options)
        api_call = api_callable.create_api_call(self.stub.CreateTopic,
                                                settings=settings)
        return api_call(req, metadata=self._headers)

Would change to:

        req = pubsub_pb2.Topic(name=name)
        return self._create_topic_callable(req, options)

New initialization code needs to be added to the constructor:

        self._create_topic_callable = api_callable.create(
            self.stub.CreateTopic,
            settings=defaults['create_topic'])

As for headers, they could be plumbed through defaults instead of passed individually to each api_callable.

As for page streaming, the api_callable would need to dynamically determine whether page streaming would be on or off based on the options.

Callers who want to override the defaults for an API instance should be able to pass in a dictionary containing those values: if not passed, the constructor can then fall back to loading the JSON string from the file in the same directory. E.g.:

    def __init__(self,
                 service_path=SERVICE_ADDRESS,
                 port=DEFAULT_SERVICE_PORT,
                 channel=None,
                 ssl_creds=None,
                 scopes=None,
                 client_config=None,
                 retrying_override=None,
                 bundling_override=None,
                 timeout=_DEFAULT_TIMEOUT,
                 app_name='gax',
                 app_version=_GAX_VERSION):
        """Constructor.

        Args:
          service_path (string): The domain name of the API remote host.
          port (int): The port on which to connect to the remote host.
          channel (:class:`grpc.beta.implementations.Channel`): A ``Channel``
            object through which to make calls.
          ssl_creds (:class:`grpc.beta.implementations.ClientCredentials`):
            A `ClientCredentials` for use with an SSL-enabled channel.
          client_config (dict)
            dictionary that overrides default client configuration
          retrying_override (dict[string, :class:`google.gax.RetryOptions`]): A
            dictionary that overrides default retrying settings.
            ``retrying_override`` maps method names (e.g., ``'list_foo'``) to
            custom RetryOptions objects, or to None. A value of None indicates
            that the method in question should not retry.
          bundling_override (dict[string, :class:`google.gax.BundleOptions`]): A
            dictionary that overrides default bundling settings.
            ``bundling_override`` maps bundling method names (e.g.,
            'publish_foo') to custom BundleOptions objects, or to None. It is
            invalid to have a key for a method that is not bundling-enabled. A
            value of None indicates that the method in question should not
            bundle.
          timeout (int): The default timeout, in seconds, for calls made
            through this client
          app_name (string): The codename of the calling service.
          app_version (string): The version of the calling service.

        Returns:
          A PublisherApi object.
        """
        if scopes is None:
            scopes = self._ALL_SCOPES
        bundling_override = bundling_override or {}
        retrying_override = retrying_override or {}
        if client_config is None:
            client_config_json = pkg_resources.resource_string(
                __name__, 'publisher_client_config.json')
            client_config = json.loads(client_config_json)
        self._defaults = api_callable.construct_settings(
            'google.pubsub.v1.Publisher',
            client_config,
            bundling_override,
            retrying_override,
            config.STATUS_CODE_NAMES,
            timeout,
            bundle_descriptors=self._BUNDLE_DESCRIPTORS,
            page_descriptors=self._PAGE_DESCRIPTORS)
        google_apis_agent = '{}/{};{};gax/{};python/{}'.format(
            app_name, app_version, self._CODE_GEN_NAME_VERSION,
            self._GAX_VERSION, platform.python_version())
        self._headers = [('x-google-apis-agent', google_apis_agent)]
        self.stub = config.create_stub(pubsub_pb2.beta_create_Publisher_stub,
                                       service_path,
                                       port,
                                       ssl_creds=ssl_creds,
                                       channel=channel,
                                       scopes=scopes)

The green pixel matrix is a bit lacking, aesthetically :)