- Notifications
You must be signed in to change notification settings - Fork95
feat(api-core): pass retry from result() to done()#9
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
IlyaFaer commentedFeb 21, 2020
If I understood everything correctly, the last thing that's left is to pass This related to another PR:googleapis/python-bigquery#41, which implements |
IlyaFaer commentedJun 24, 2020
@busunkim96, friendly ping |
google/api_core/future/polling.py Outdated
| def_done_or_raise(self,retry=DEFAULT_RETRY): | ||
| """Check if the future is done and raise if it's not.""" | ||
| ifnotself.done(): | ||
| ifnotself.done(retry): |
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 should be passed as a keyword argument
| ifnotself.done(retry): | |
| ifnotself.done(retry=retry): |
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.
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 nextgoogle-api-core (this is a breaking change).
We should really only populateretry at all when explicitly passed a value for one.
Maybe something like this?
| ifnotself.done(retry): | |
| done_kwargs= {} | |
| ifretryisnotDEFAULT_RETRY: | |
| done_kwargs["retry"]=retry | |
| ifnotself.done(**done_kwargs): |
Uh oh!
There was an error while loading.Please reload this page.
tswast commentedOct 6, 2020
@IlyaFaer We've been getting customer support tickets regarding retry settings, so I'd like to resurrect this PR. |
google/api_core/future/polling.py Outdated
| def_done_or_raise(self,retry=DEFAULT_RETRY): | ||
| """Check if the future is done and raise if it's not.""" | ||
| ifnotself.done(): | ||
| ifnotself.done(retry): |
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.
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 nextgoogle-api-core (this is a breaking change).
We should really only populateretry at all when explicitly passed a value for one.
Maybe something like this?
| ifnotself.done(retry): | |
| done_kwargs= {} | |
| ifretryisnotDEFAULT_RETRY: | |
| done_kwargs["retry"]=retry | |
| ifnotself.done(**done_kwargs): |
tswast commentedOct 6, 2020
We need to be careful with this, as there are likely subclasses that don't have a To avoid breaking them, we need logic to only populate |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
tswast left a comment
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 tested this on the current version ofgoogle-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-idQuery 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-idQuery 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.Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
IlyaFaer left a comment
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.
@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
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
tswast left a comment
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 checked out these changes locally and verified they pass against the BigQuery test suite.
Uh oh!
There was an error while loading.Please reload this page.
Towards:googleapis/python-bigquery#24