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

IlyaFaer · 2020-02-21T10:02:05Z

IlyaFaer · 2020-02-21T10:06:33Z

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 · 2020-06-24T06:59:50Z

@busunkim96, friendly ping

tswast · 2020-10-06T17:36:31Z

google/api_core/future/polling.py

        """Check if the future is done and raise if it's not."""
-        if not self.done():
+        if not self.done(retry):


This should be passed as a keyword argument

Suggested change

if not self.done(retry):

if not self.done(retry=retry):

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

if not self.done(retry):

done_kwargs = {}

if retry is not DEFAULT_RETRY:

done_kwargs["retry"] = retry

if not self.done(**done_kwargs):

google/api_core/future/polling.py

tswast · 2020-10-06T17:38:23Z

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

tswast · 2020-10-06T21:51:47Z

google/api_core/future/polling.py

        """Check if the future is done and raise if it's not."""
-        if not self.done():
+        if not self.done(retry):


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

if not self.done(retry):

done_kwargs = {}

if retry is not DEFAULT_RETRY:

done_kwargs["retry"] = retry

if not self.done(**done_kwargs):

tswast · 2020-10-06T21:53:53Z

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)

tests/unit/future/test_polling.py

…pi-core into retry_into_done

tests/unit/future/test_polling.py

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.

google/api_core/future/polling.py

tests/unit/future/test_polling.py

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

google/api_core/future/polling.py

tests/unit/future/test_polling.py

tswast

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

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

512a65f

IlyaFaer added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Feb 21, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 21, 2020

IlyaFaer mentioned this pull request Feb 21, 2020

feat: pass retry from Job.result() to Job.done() googleapis/python-bigquery#41

Merged

fix tests

a5288b2

IlyaFaer requested review from busunkim96 and tswast February 21, 2020 10:34

IlyaFaer marked this pull request as ready for review February 21, 2020 10:34

Merge branch 'master' into retry_into_done

b6a0655

busunkim96 requested review from tseaver and crwilcox and removed request for tswast March 3, 2020 01:25

tswast reviewed Oct 6, 2020

View reviewed changes

tswast requested changes Oct 6, 2020

View reviewed changes

IlyaFaer added 2 commits October 7, 2020 10:37

Merge branch 'master' into retry_into_done

3c58c28

pass retry into done() only if it is not default

e8c1de4

IlyaFaer requested a review from a team as a code owner October 7, 2020 08:16

Merge branch 'master' into retry_into_done

0f145ac

tswast reviewed Oct 12, 2020

View reviewed changes

tests/unit/future/test_polling.py Show resolved Hide resolved

IlyaFaer added 2 commits October 13, 2020 10:58

add more unit tests

812a2e5

Merge branch 'retry_into_done' of https://github.com/q-logic/python-a…

f6b1749

…pi-core into retry_into_done

tswast reviewed Oct 13, 2020

View reviewed changes

tests/unit/future/test_polling.py Show resolved Hide resolved

IlyaFaer added 2 commits October 13, 2020 18:04

use super() in the overriden result() method

6e411ec

fix super() function call for py-2.x

0c17181

tswast approved these changes Oct 13, 2020

View reviewed changes

tswast requested changes Oct 14, 2020

View reviewed changes

google/api_core/future/polling.py Outdated Show resolved Hide resolved

google/api_core/future/polling.py Outdated Show resolved Hide resolved

tests/unit/future/test_polling.py Show resolved Hide resolved

add kwargs guard

2b21471

IlyaFaer commented Oct 15, 2020

View reviewed changes

google/api_core/future/polling.py Outdated Show resolved Hide resolved

google/api_core/future/polling.py Outdated Show resolved Hide resolved

tests/unit/future/test_polling.py Show resolved Hide resolved

tswast approved these changes Oct 16, 2020

View reviewed changes

Merge branch 'master' into retry_into_done

57468f8

tswast added the automerge Merge the pull request once unit tests and other checks pass. label Oct 16, 2020

gcf-merge-on-green bot merged commit 6623b31 into googleapis:master Oct 16, 2020

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 16, 2020

IlyaFaer deleted the retry_into_done branch October 19, 2020 09:03

tswast mentioned this pull request Dec 14, 2022

2.11.0 version breaks retrocompatibility with python-bigquery 1.x branch #477

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

IlyaFaer commented Feb 21, 2020 •

edited

Loading

IlyaFaer commented Feb 21, 2020

IlyaFaer commented Jun 24, 2020

tswast Oct 6, 2020

tswast Oct 6, 2020

tswast commented Oct 6, 2020

tswast Oct 6, 2020

tswast commented Oct 6, 2020

tswast left a comment

IlyaFaer left a comment

tswast left a comment

-        if not self.done(retry):
+        done_kwargs = {}
+        if retry is not DEFAULT_RETRY:
+            done_kwargs["retry"] = retry
+        if not self.done(**done_kwargs):

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

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

Conversation

IlyaFaer commented Feb 21, 2020 • edited Loading

IlyaFaer commented Feb 21, 2020

IlyaFaer commented Jun 24, 2020

tswast Oct 6, 2020

Choose a reason for hiding this comment

tswast Oct 6, 2020

Choose a reason for hiding this comment

tswast commented Oct 6, 2020

tswast Oct 6, 2020

Choose a reason for hiding this comment

tswast commented Oct 6, 2020

tswast left a comment

Choose a reason for hiding this comment

IlyaFaer left a comment

Choose a reason for hiding this comment

tswast left a comment

Choose a reason for hiding this comment

IlyaFaer commented Feb 21, 2020 •

edited

Loading