Skip to content
This repository has been archived by the owner on Dec 31, 2023. It is now read-only.

tests: add cases for int64 field, map pagination, field that start with capital letter #105

Merged
merged 8 commits into from
Aug 23, 2021

Conversation

georgiyekkert
Copy link
Contributor

tests for:

  1. Map response pagination
  2. int 64 field - b/190201970
  3. a field that starts with cap letter - b/184008102

@georgiyekkert georgiyekkert requested review from a team as code owners August 13, 2021 18:12
@product-auto-label product-auto-label bot added the api: compute Issues related to the googleapis/python-compute API. label Aug 13, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 13, 2021
tests/system/test_smoke.py Outdated Show resolved Hide resolved
tests/system/test_smoke.py Outdated Show resolved Hide resolved
tests/system/test_instance_group.py Show resolved Hide resolved
Comment on lines 84 to 91
presented = False
for zone, types in result:
if zone == "zones/" + self.DEFAULT_ZONE:
for ac_type in types.accelerator_types:
if ac_type.name == "nvidia-tesla-t4":
presented = True
break
self.assertTrue(presented)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop-with-break seems complicated, especially with the condition outside the loop: I had to pause, re-read, and reconstruct what it meant. Could be simplified as something like:

        zone_acc_types = {
            zone: set(types.accelerator_types) for zone, types in result
        }
        default_zone = "zones/" + self.DEFAULT_ZONE
        self.assertIn(zone_acc_types[default_zone], "nvidia-tesla-t4")

which will raise a KeyError if the default zone is not present in the result, and a normal assertion if the expected acclerator type is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we want to test that pagination works our of the box, i.e. you don't need to provide nextPageToken if your aggregatedList() response has more than one page, remaining pages will be retrieved automatically.

if we use dict comprehension here, values in zone_acc_types will be overridden every time we retrieve a new page

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean here: the dict comprehension is the equivalent of:

        zone_acc_types = {}
        for zone, types in result:
            zone_acc_types[zone] = list(types.accelerator_types)

which iterates over all the items in the response.

Do you mean that a given "scope" (the docs call the key to the items mapping a scope) would appear more than once in the items returned from the iterator? Those docs imply that items is a map (key: string, value: object), which would mean that repeating a key would violate them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pagination is a feature where we want to have one iterator over multiple API calls.

from https://cloud.google.com/compute/docs/reference/rest/v1/acceleratorTypes/aggregatedList

maxResults | integer (uint32 format)The maximum number of results per page that should be returned. If the number of available results is larger than maxResults, Compute Engine returns a nextPageToken that can be used to get the next page of results in subsequent list requests. Acceptable values are 0 to 500, inclusive. (Default: 500)

So if there are 1500 items and we set maxResults to 500 then we have to make 3 aggregatedList() calls to retrieve 3 pages. To make this scenario easy for users we have AggregatedListPager class - https://github.com/googleapis/python-compute/blob/master/google/cloud/compute_v1/services/accelerator_types/pagers.py#L30
Which:
If there are more pages, the __iter__ method will make additional
AggregatedList requests and continue to iterate
through the items field on the
corresponding responses.

if we use dict comprehension:
zone_acc_types = {
zone: set(types.accelerator_types) for zone, types in result
}

result here contains items from the first page and as soon as we finish iterating the first page, pager will make another aggregatedList() call and update result to the response without stopping iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in other words, yes, scope will appear more than once because we process multiple responses from the API.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The page_token passed to each page of responses (after the first) should guarantee the values from previously-fetched pages are not present in subsequent pages. At least, that is the way that paging works in all the other APIs which use page tokens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From https://github.com/googleapis/python-compute/blob/master/google/cloud/compute_v1/services/accelerator_types/pagers.py

def __iter__(self) -> Iterable[Tuple[str, compute.AcceleratorTypesScopedList]]:
    for page in self.pages:
        yield from page.items.items()

@property
def pages(self) -> Iterable[compute.AcceleratorTypeAggregatedList]:
    yield self._response
    while self._response.next_page_token:
        self._request.page_token = self._response.next_page_token
        self._response = self._method(self._request, metadata=self._metadata)
        yield self._response

We iterate over pages, a page always has the same set of keys(zones like us-central1-a) but different values(accelerator-types).

Because keys are always the same, the values will be overridden as long as we retrieve new pages, so we can't use dict comprehension here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, as in all the other libraries which use the generated page iterators, pagination doesn't repeat items, because the next_page_token from each prior response is passed as page_token to the next response: that token acts as a "cursor" into the overall list of items, and allows iterating through lists that are bigger than any given page size.

The code above doesn't iterate over individual pages: it iterates over the concatenated stream of items from each page. If you save them as a list, you get 90 entries:

>>> from google.cloud.compute_v1.services.accelerator_types import AcceleratorTypesClient
>>> client = AcceleratorTypesClient()
>>> zone_acc_types_list = [
...     (zone, [at.name for at in types.accelerator_types])
...        for zone, types in client.aggregated_list(project="precise-truck-742")
... ]  # consume all items
>>> zones = [item[0] for item in zone_acc_types_list]
>>> len(zones)
90

and each list item has a unique scope (zone):

>>> len(set(zones))
90

A dict comprehension also has 90 elements, matching those in the list:

>>> zone_acc_types_dict = {
...     scope: [at.name for at in types.accelerator_types]
...     for scope, types in client.aggregated_list(project="precise-truck-742")
... }
>>> len(zone_acc_types_dict)
90
>>> set(zone_acc_types_dict) == zones
True
>>> for zone, types in zone_acc_types_list:
...     assert zone_acc_types_dict[zone] == acc_type_names
... 
>>>

And we can test for the desired accelerator type name with either one:

>>> default_zone = "zones/us-central1-a"
>>> 'nvidia-tesla-t4' in zone_acc_types_dict[zone]
True
>>> 'nvidia-tesla-t4' in [names for zone, names in zone_acc_types_list if zone == default_zone][0]
True

Copy link

@tseaver tseaver Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just now looked at the pages returned when passing max_results=3, and see that indeed the backend is returning the full set of zones, with actual types keys set only for up to 3 of them -- the others are all flagged with warnings. I have no idea why the backend would be implemented so: it is completely out of the expected behavior for other APIs, and explicitly violates the documented behavior for maxResults.

Field Type Description
maxResults integer (uint32 format) The maximum number of results per page that should be returned. If the number of available results is larger than maxResults, Compute Engine returns a nextPageToken that can be used to get the next page of results in subsequent list requests. Acceptable values are 0 to 500, inclusive. (Default: 500)

Given that backend implementation weirdness, the dict comprehension I entered above is indeed not workable when setting max_results. However, the following should work:

import collections
...
    def test_auto_paging_map_response(self):
        client = AcceleratorTypesClient()
        request = AggregatedListAcceleratorTypesRequest(
            project=self.DEFAULT_PROJECT, max_results=3
        )
        result = client.aggregated_list(request=request)
        zone_acc_types = collections.defaultdict(list)
        for zone, types in result:
            zone_acc_types[zone].extend(at.name for at in types.accelerator_types)
        default_zone = "zones/" + self.DEFAULT_ZONE
        self.assertIn(zone_acc_types[default_zone], "nvidia-tesla-t4")

and has the benefit of showing how to work around the backend's weirdness to get a "normalized" result set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@vam-google vam-google merged commit 69cba08 into googleapis:master Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: compute Issues related to the googleapis/python-compute API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants