feat: add progress bar for to_arrow method #352

HemangChothani · 2020-10-29T13:17:06Z

Fixes #343

PR open for the feedback and suggestions.
Currently implemented for 'QueryJob.to_arrow' method
When the cacheHit for result is true at time query_plan is blank so can't implement progress bar.

tswast · 2020-10-29T14:20:14Z

google/cloud/bigquery/job.py

@@ -20,6 +20,9 @@
 import copy
 import re
 import threading
+import tqdm


This needs to be an optional import. See:

python-bigquery/google/cloud/bigquery/table.py

Lines 39 to 42 in 20f473b

try:

import tqdm

except ImportError: # pragma: NO COVER

tqdm = None

tswast · 2020-10-29T14:21:45Z

google/cloud/bigquery/job.py

@@ -3275,6 +3283,27 @@ def result(
        rows._preserve_order = _contains_order_by(self.query)
        return rows

+    def _get_progress_bar(self, progress_bar_type, description, total, unit):


Seems like this is very close to the logic in table.py

python-bigquery/google/cloud/bigquery/table.py

Line 1374 in 20f473b

def _get_progress_bar(self, progress_bar_type):

I'd suggest creating a _tqdm_helpers.py module similar to our pandas helpers to hold this logic for both Table and Job logic.

…into bigquery_issue_343

tswast · 2020-11-03T15:46:04Z

google/cloud/bigquery/job.py

@@ -3337,7 +3339,49 @@ def to_arrow(

        ..versionadded:: 1.17.0
        """
-        return self.result().to_arrow(
+        if self.query_plan and progress_bar_type:


Is this logic necessary if we are calling to_arrow? Can't we rely on to_arrow's progress bar support?

Sorry, didn't get this point, are you talking about table.to_arrow() progress bar support?

Oh, I was getting this confused with RowIterator, where to_dataframe relies on to_arrow's progress bar support. I do wonder if some of this "waiting for the query to finish" logic could be refactored into a _tqdm_helpers.py function, since it should be identical between to_dataframe and to_arrow

tswast · 2020-11-03T15:46:58Z

google/cloud/bigquery/job.py

@@ -3406,7 +3450,46 @@ def to_dataframe(
        Raises:
            ValueError: If the `pandas` library cannot be imported.
        """
-        return self.result().to_dataframe(
+        query_plan = self.query_plan
+        if query_plan and progress_bar_type:


query_plan can get updated as the job progresses. I'd prefer if this always created a progress bar, but had a generic message when the job is still pending and there isn't a query plan.

…into bigquery_issue_343

tswast · 2020-11-05T16:52:46Z

google/cloud/bigquery/job.py

+                    )
+
+                    try:
+                        query_result = self.result(timeout=0.5)


We should make constants for this (PROGRESS_BAR_UPDATE_INTERVAL, for example)

tswast

Looking good. A few nits regarding naming and indentation.

tswast · 2020-11-10T22:04:46Z

google/cloud/bigquery/_tqdm_helpers.py

+    return None
+
+
+def _query_job_result_helper(query_job, progress_bar_type=None):


Let's use active verbs in this helper name.

Suggested change

def _query_job_result_helper(query_job, progress_bar_type=None):

def wait_for_query(query_job, progress_bar_type=None):

tswast · 2020-11-10T22:06:33Z

google/cloud/bigquery/_tqdm_helpers.py

+
+def _query_job_result_helper(query_job, progress_bar_type=None):
+    """Return query result and display a progress bar while the query running, if tqdm is installed."""
+    if progress_bar_type:


We can reduce the level of indentation if we exit early. Also, I think we'll want to pass through some keyword arguments to result(), correct?

Suggested change

if progress_bar_type:

if progress_bar_type is None:

return query_job.result()

Aside (not relevant for this PR): we'll eventually want to pass additional arguments to result() whenever we implement #296

tswast · 2020-11-10T22:07:56Z

google/cloud/bigquery/_tqdm_helpers.py

+_PROGRESS_BAR_UPDATE_INTERVAL = 0.5
+
+
+def _get_progress_bar(progress_bar_type, description, total, unit):


We want to use these helpers from other modules, let's remove the (redundant because of private module) leading _.

Suggested change

def _get_progress_bar(progress_bar_type, description, total, unit):

def get_progress_bar(progress_bar_type, description, total, unit):

tswast · 2020-11-10T22:15:26Z

google/cloud/bigquery/_tqdm_helpers.py

+        progress_bar = _get_progress_bar(
+            progress_bar_type, "Query is running", 1, "query"
+        )
+        if query_job.query_plan:


This should be one level deeper. I'd like to see the while True loop, even when query_plan is not initially populated.

…into bigquery_issue_343

tswast

I'd expect the while loop to contain the following steps:

Call result() with a short-ish timeout.
Call reload()
Update the progress bar (different cases for query_plan present or not)

tswast · 2020-11-12T18:36:36Z

google/cloud/bigquery/_tqdm_helpers.py

+    """Return query result and display a progress bar while the query running, if tqdm is installed."""
+    if progress_bar_type is None:
+        query_result = query_job.result()
+    else:


I meant that you could return query_job.result() here. The else statement is then unnecessary, saving us 1 level of indentation.

tswast · 2020-11-12T22:30:34Z

google/cloud/bigquery/_tqdm_helpers.py

+                            i += 1
+                    continue
+            else:
+                query_result = query_job.result()


We need a timeout here. It's not clear to me why the above try block is even in the if statement.

Looks like the only difference is the presence of total, which could be set to a default value in the case where query_plan is not present.

tswast · 2020-11-12T22:32:25Z

google/cloud/bigquery/_tqdm_helpers.py

+        while True:
+            if query_job.query_plan:
+                total = len(query_job.query_plan)
+                query_job.reload()  # Refreshes the state via a GET request.


Wouldn't we only want to call reload after result times out?

Also, why would we only call reload inside this if statement? The job might not have a query_plan until after reload is called in many cases.

…into bigquery_issue_343

HemangChothani · 2020-11-13T10:57:24Z

system test failed not related to changes.

tswast

Love it!

P.S. I have opened #388 to fix those failing tests.

feat: add progress bar for to_arrow method

05f101e

HemangChothani requested a review from tswast October 29, 2020 13:17

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 29, 2020

tswast reviewed Oct 29, 2020

View reviewed changes

feat: add progress bar for to_dataframe

288739f

HemangChothani marked this pull request as ready for review November 2, 2020 07:51

HemangChothani requested a review from a team November 2, 2020 07:51

Merge branch 'master' of https://github.com/googleapis/python-bigquery …

d94894c

…into bigquery_issue_343

tswast requested changes Nov 3, 2020

View reviewed changes

HemangChothani added 2 commits November 4, 2020 19:13

feat: add default progress bar and unit test

fa1bb49

Merge branch 'master' of https://github.com/googleapis/python-bigquery …

485bcf5

…into bigquery_issue_343

tswast reviewed Nov 5, 2020

View reviewed changes

feat: resolve conflict and move code to _tqdm_helper file

f9b2ff7

tswast reviewed Nov 10, 2020

View reviewed changes

HemangChothani added 2 commits November 12, 2020 17:23

feat: nit

481fa05

Merge branch 'master' of https://github.com/googleapis/python-bigquery …

c1ee80e

…into bigquery_issue_343

tswast requested changes Nov 12, 2020

View reviewed changes

HemangChothani added 2 commits November 13, 2020 13:28

feat: result timout for without queryplan

47d393c

Merge branch 'master' of https://github.com/googleapis/python-bigquery …

e1d017c

…into bigquery_issue_343

tswast approved these changes Nov 16, 2020

View reviewed changes

tswast merged commit dc78edd into googleapis:master Nov 16, 2020

tswast mentioned this pull request Nov 16, 2020

Implement initial "waiting" logs with tqdm? googleapis/python-bigquery-pandas#327

Closed

willbowditch mentioned this pull request Nov 25, 2020

Wait for Query hangs indefinitely for some queries #403

Closed

release-please bot mentioned this pull request Jan 4, 2022

chore(main): release python-bigquery 1.27.1 #1097

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add progress bar for to_arrow method #352

feat: add progress bar for to_arrow method #352

HemangChothani commented Oct 29, 2020

tswast Oct 29, 2020

tswast Oct 29, 2020

tswast Nov 3, 2020

HemangChothani Nov 4, 2020

tswast Nov 5, 2020

tswast Nov 3, 2020

tswast Nov 5, 2020

tswast left a comment

tswast Nov 10, 2020

tswast Nov 10, 2020

tswast Nov 10, 2020

tswast Nov 10, 2020

tswast left a comment

tswast Nov 12, 2020

tswast Nov 12, 2020

tswast Nov 12, 2020

tswast Nov 12, 2020

HemangChothani commented Nov 13, 2020

tswast left a comment

	try:
	import tqdm
	except ImportError: # pragma: NO COVER
	tqdm = None

		return None


		def _query_job_result_helper(query_job, progress_bar_type=None):

	def _query_job_result_helper(query_job, progress_bar_type=None):
	def wait_for_query(query_job, progress_bar_type=None):

	if progress_bar_type:
	if progress_bar_type is None:
	return query_job.result()

		_PROGRESS_BAR_UPDATE_INTERVAL = 0.5


		def _get_progress_bar(progress_bar_type, description, total, unit):

	def _get_progress_bar(progress_bar_type, description, total, unit):
	def get_progress_bar(progress_bar_type, description, total, unit):

feat: add progress bar for to_arrow method #352

feat: add progress bar for to_arrow method #352

Conversation

HemangChothani commented Oct 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tswast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tswast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HemangChothani commented Nov 13, 2020

tswast left a comment

Choose a reason for hiding this comment