feat(api-core): pass retry from result() to done()

[IlyaFaer]

Towards: googleapis/python-bigquery#24

[IlyaFaer]

If I understood everything correctly, the last thing that's left is to pass retry arg of a result() method to done(). Here it is.

This related to another PR: googleapis/python-bigquery#41, which implements retry passing from inheritors to PollingFuture

[IlyaFaer]

@busunkim96, friendly ping

[tswast]

This should be passed as a keyword argument

Suggested change

	ifnotself.done(retry):
	ifnotself.done(retry=retry):

[tswast]

On further thought, since this is a new kwarg, we don't want to force all clients that subclass it to add it in order to use the next google-api-core (this is a breaking change).

We should really only populate retry at all when explicitly passed a value for one.

Maybe something like this?

Suggested change

	ifnotself.done(retry):
	done_kwargs= {}
	ifretryisnotDEFAULT_RETRY:
	done_kwargs["retry"] =retry
	ifnotself.done(**done_kwargs):

[tswast]

@IlyaFaer We've been getting customer support tickets regarding retry settings, so I'd like to resurrect this PR.

[tswast]

On further thought, since this is a new kwarg, we don't want to force all clients that subclass it to add it in order to use the next google-api-core (this is a breaking change).

We should really only populate retry at all when explicitly passed a value for one.

Maybe something like this?

Suggested change

	ifnotself.done(retry):
	done_kwargs= {}
	ifretryisnotDEFAULT_RETRY:
	done_kwargs["retry"] =retry
	ifnotself.done(**done_kwargs):

[tswast]

We need to be careful with this, as there are likely subclasses that don't have a retry parameter. Forcing them to add a retry parameter is a breaking change.

To avoid breaking them, we need logic to only populate retry when clients support it (which I think is always true if we end up getting passed a non-default value for retry)

[tswast]

I tested this on the current version of google-cloud-bigquery and got many test failures.

../../miniconda3/envs/bq-patch-1/lib/python3.8/site-packages/mock/mock.py:880: AssertionError
---------------------------------------------------------- Captured stdout setup -----------------------------------------------------------
----------------------------------------------------------- Captured stdout call -----------------------------------------------------------
Executing query with job ID: some-random-id
Query executing: 0.00s
----------------------------------------------------------- Captured stderr call -----------------------------------------------------------
ERROR:
_blocking_poll() got an unexpected keyword argument 'retry'
________________________________________________________ test_context_no_connection ________________________________________________________
@pytest.mark.usefixtures("ipython_interactive")
@pytest.mark.skipif(pandas is None, reason="Requires `pandas`")
def test_context_no_connection():
ip = IPython.get_ipython()
ip.extension_manager.load_extension("google.cloud.bigquery")
magics.context._project = None
magics.context._credentials = None
magics.context._connection = None
credentials_mock = mock.create_autospec(
google.auth.credentials.Credentials, instance=True
)
project = "project-123"
default_patch = mock.patch(
"google.auth.default", return_value=(credentials_mock, project)
)
job_reference = copy.deepcopy(JOB_REFERENCE_RESOURCE)
job_reference["projectId"] = project
query = "select * from persons"
resource = copy.deepcopy(QUERY_RESOURCE)
resource["jobReference"] = job_reference
resource["configuration"]["query"]["query"] = query
data = {"jobReference": job_reference, "totalRows": 0, "rows": []}
conn_mock = make_connection(resource, data, data, data)
conn_patch = mock.patch("google.cloud.bigquery.client.Connection", autospec=True)
list_rows_patch = mock.patch(
"google.cloud.bigquery.client.Client.list_rows",
return_value=google.cloud.bigquery.table._EmptyRowIterator(),
)
with conn_patch as conn, list_rows_patch as list_rows, default_patch:
conn.return_value = conn_mock
ip.run_cell_magic("bigquery", "", query)
# Check that query actually starts the job.
> list_rows.assert_called()
tests/unit/test_magics.py:244:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
_mock_self = <MagicMock name='list_rows' id='140185511524480'>
def assert_called(_mock_self):
"""assert that the mock was called at least once
"""
self = _mock_self
if self.call_count == 0:
msg = ("Expected '%s' to have been called." %
(self._mock_name or 'mock'))
> raise AssertionError(msg)
E AssertionError: Expected 'list_rows' to have been called.
../../miniconda3/envs/bq-patch-1/lib/python3.8/site-packages/mock/mock.py:880: AssertionError
----------------------------------------------------------- Captured stdout call -----------------------------------------------------------
Executing query with job ID: some-random-id
Query executing: 0.00s
----------------------------------------------------------- Captured stderr call -----------------------------------------------------------
ERROR:
_blocking_poll() got an unexpected keyword argument 'retry'
========================================================= short test summary info ==========================================================
FAILED tests/unit/test_job.py::TestQueryJob::test_iter - TypeError: _blocking_poll() got an unexpected keyword argument 'retry'
FAILED tests/unit/test_job.py::TestQueryJob::test_result - TypeError: _blocking_poll() got an unexpected keyword argument 'retry'
FAILED tests/unit/test_job.py::TestQueryJob::test_result_error - TypeError: _blocking_poll() got an unexpected keyword argument 'retry'
FAILED tests/unit/test_job.py::TestQueryJob::test_result_invokes_begins - TypeError: _blocking_poll() got an unexpected keyword argument ...
FAILED tests/unit/test_job.py::TestQueryJob::test_result_w_empty_schema - TypeError: _blocking_poll() got an unexpected keyword argument ...
FAILED tests/unit/test_job.py::TestQueryJob::test_result_w_page_size - TypeError: _blocking_poll() got an unexpected keyword argument 're...
FAILED tests/unit/test_job.py::TestQueryJob::test_result_w_timeout - TypeError: _blocking_poll() got an unexpected keyword argument 'retry'
FAILED tests/unit/test_job.py::TestQueryJob::test_result_with_max_results - TypeError: _blocking_poll() got an unexpected keyword argumen...
FAILED tests/unit/test_job.py::TestQueryJob::test_result_with_start_index - TypeError: _blocking_poll() got an unexpected keyword argumen...
FAILED tests/unit/test_job.py::TestQueryJob::test_to_arrow - TypeError: _blocking_poll() got an unexpected keyword argument 'retry'
FAILED tests/unit/test_job.py::TestQueryJob::test_to_dataframe - TypeError: _blocking_poll() got an unexpected keyword argument 'retry'
FAILED tests/unit/test_job.py::TestQueryJob::test_to_dataframe_bqstorage - TypeError: _blocking_poll() got an unexpected keyword argument...
FAILED tests/unit/test_job.py::TestQueryJob::test_to_dataframe_column_date_dtypes - TypeError: _blocking_poll() got an unexpected keyword...
FAILED tests/unit/test_job.py::TestQueryJob::test_to_dataframe_column_date_dtypes_wo_pyarrow - TypeError: _blocking_poll() got an unexpec...
FAILED tests/unit/test_job.py::TestQueryJob::test_to_dataframe_column_dtypes - TypeError: _blocking_poll() got an unexpected keyword argu...
FAILED tests/unit/test_job.py::TestQueryJob::test_to_dataframe_ddl_query - TypeError: _blocking_poll() got an unexpected keyword argument...
FAILED tests/unit/test_job.py::TestQueryJob::test_to_dataframe_with_progress_bar - TypeError: _blocking_poll() got an unexpected keyword ...
FAILED tests/unit/test_job.py::test_to_dataframe_bqstorage_preserve_order[select name, age from table order by other_column;] - TypeError...
FAILED tests/unit/test_job.py::test_to_dataframe_bqstorage_preserve_order[Select name, age From table Order By other_column;] - TypeError...
FAILED tests/unit/test_job.py::test_to_dataframe_bqstorage_preserve_order[SELECT name, age FROM table ORDER BY other_column;] - TypeError...
FAILED tests/unit/test_job.py::test_to_dataframe_bqstorage_preserve_order[select name, age from table order\nby other_column;] - TypeErro...
FAILED tests/unit/test_job.py::test_to_dataframe_bqstorage_preserve_order[Select name, age From table Order\nBy other_column;] - TypeErro...
FAILED tests/unit/test_job.py::test_to_dataframe_bqstorage_preserve_order[SELECT name, age FROM table ORDER\nBY other_column;] - TypeErro...
FAILED tests/unit/test_job.py::test_to_dataframe_bqstorage_preserve_order[SelecT name, age froM table OrdeR \n\t BY other_column;] - Type...
FAILED tests/unit/test_magics.py::test_context_connection_can_be_overriden - AssertionError: Expected 'list_rows' to have been called.
FAILED tests/unit/test_magics.py::test_context_no_connection - AssertionError: Expected 'list_rows' to have been called.

[IlyaFaer]

@tswast, I've locally replaced dependencies in the way that it used a git repository version of api_core in unit tests, and, yeah, I see it too. Pushed the changes

[tswast]

I checked out these changes locally and verified they pass against the BigQuery test suite.