project_id as optional #127

max-sixty · 2018-02-20T02:42:30Z

The underlying Google libraries can read default projects, so there's no need for us to force users to supply one

codecov-io · 2018-02-20T02:45:19Z

Codecov Report

❗ No coverage uploaded for pull request base (master@187a57f). Click here to learn what that means.
The diff coverage is 52.94%.

@@            Coverage Diff            @@
##             master     #127   +/-   ##
=========================================
  Coverage          ?   34.97%           
=========================================
  Files             ?        8           
  Lines             ?     1521           
  Branches          ?        0           
=========================================
  Hits              ?      532           
  Misses            ?      989           
  Partials          ?        0

Impacted Files	Coverage Δ
pandas_gbq/gbq.py	`30.48% <50%> (ø)`
pandas_gbq/tests/test_gbq.py	`28.44% <53.84%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 187a57f...a3e6d2f. Read the comment docs.

tswast

Nice! For sanity, please add a system test similar to https://github.com/pydata/pandas-gbq/blob/11559583d1451fc64108c12f332d1c93dec4c80c/pandas_gbq/tests/test_gbq.py#L223-L229 which runs when default credentials are available and verifies that a query can be run without setting an explicit project ID.

tswast · 2018-02-23T11:24:11Z

pandas_gbq/gbq.py

@@ -761,7 +761,7 @@ def read_gbq(query, project_id=None, index_col=None, col_order=None,
    ----------
    query : str
        SQL-Like Query to return data values
-    project_id : str
+    project_id : str (optional)


The project_id is still required when using user authentication, so we should clarify that it is only optional if using default credentials.

Or maybe also optional with a service account? I think we might have to parse as JSON and grab the project ID from the service account and use https://google-auth.readthedocs.io/en/latest/reference/google.oauth2.service_account.html#google.oauth2.service_account.Credentials.from_service_account_info for that to work.

Thanks for bearing with me on the authentication questions

The project_id is still required when using user authentication

Is this right? If I'm running localling with user auth, google.cloud.bigquery can pick up my default project fine:

In [20]: import google.cloud.bigquery In [21]: c=google.cloud.bigquery.Client() In [22]: c Out[22]: <google.cloud.bigquery.client.Client at 0x1138e44a8> In [23]: c.project Out[23]: 'sixty-capital'

Auth:

gcloud auth list Credentialed Accounts ACTIVE ACCOUNT * []@sixtycapital.com To set the active account, run: $ gcloud config set account `ACCOUNT`

google.cloud.bigquery.Client() does user auth using the Cloud SDK. I'm referring to when the Cloud SDK is not installed, where pandas-gbq does it's user auth flow.

To be more specific the credential creation at

https://github.com/pydata/pandas-gbq/blob/16747609dc58f0b6df2c4e896db5a3f365c4677c/pandas_gbq/gbq.py#L272-L283

does not include a default project.

Bear with me once more:
Why don't we add it? It's in the JSON file:

In 3LO there are two projects we are concerned with:

The client secrets project. In this case, it is the project for Pandas GBQ. It has the BigQuery API enabled, but users shouldn't have permissions to run queries against this project. If we did use it, it would bill the Pandas team, not the user.

The user's project. We know this in the case of a service account or when using the gcloud command, but we don't know it if using client secrets.

OK I see now. Because we do the app_flow, those creds don't come with a project.
Though not sure what 3LO is

3LO - 3-legged OAuth 2. (The protocol that app_flow follows.)

max-sixty · 2018-04-01T03:14:24Z

pandas_gbq/tests/test_gbq.py

+def project(request):
+    if request.param == 'env':
+        return _get_project_id()
+    elif request.param == 'none':


(this didn't get used, but could param over it in the future)

max-sixty · 2018-04-01T03:21:07Z

Updated!

tswast · 2018-04-02T18:24:41Z

pandas_gbq/gbq.py

@@ -7,6 +7,7 @@
 from distutils.version import StrictVersion
 from time import sleep

+import google


google is a namespace package for many different packages. Could you import google.auth instead?

Also, we don't tend to import our direct dependencies here, since we want the code for imports checking to run before crashing on a failed import.

…fault-project

tswast · 2018-04-02T23:52:15Z

pandas_gbq/gbq.py

@@ -170,6 +170,9 @@ def __init__(self, project_id, reauth=False,
        from google.api_core.exceptions import GoogleAPIError
        from google.api_core.exceptions import ClientError
        self.http_error = (ClientError, GoogleAPIError)
+        if not project_id:
+            from google.auth import default
+            _, project_id = default()


On second thought, this could cause us to go through the whole "application default credentials" flow twice, which could be quite slow. (It checks for env var, checks for GCE, checks for App Engine, ...)

Could we change https://github.com/maxim-lian/pandas-gbq/blob/a7f6c43434bc21e1d2fc7633d3c5cb9c04060a4a/pandas_gbq/gbq.py#L223 to not discard the project instead?

OK, and it's not cached?

Yeah not great:

In [4]: %timeit google.auth.default() 573 ms ± 14.1 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

What do you think about changing those methods to return 2-tuples?

I think tuples would be reasonable. The other (more disruptive, probably) option would be to pass in the project to the credentials methods and have them return a google.cloud.bigquery.Client.

I updated this PR to tuples as we discussed.

max-sixty · 2018-04-03T22:49:10Z

This will close #72

…-default-project

stickler-ci · 2018-04-25T18:03:06Z

pandas_gbq/gbq.py

-            return credentials
+
+        # Try to retrieve Application Default Credentials
+        credentials, default_project = self.get_application_default_credentials()


E501 line too long (81 > 79 characters)

I think BigQuery stopped checking for valid project on queries with no data access.

max-sixty · 2018-04-25T18:52:59Z

Looks like you're updating @tswast - thanks. I shouldn't have let this hang around for so long - then I hit merge conflicts and slowed down even more

Feel free to discard anything ancillary to the main task here (e.g. specifying the auth type in the test run rather than the hacky function that skip in travis), and I'll add them separately

Thanks!

tswast · 2018-04-25T19:25:09Z

Cool. I'll get this to the finish line. Thanks for your work so far, @maxim-lian

Re: auth type tests. Yeah, I'll revert back to the wonky _skip_in_travis() functions for now. I'm thinking of doing a refactor soon that pull the auth functions out of GbqConnector and into their own module. Then I think it'll make more sense to test auth separately from read/to.

project_id is optional

c85a685

tswast mentioned this pull request Feb 23, 2018

Set project_id (and other settings) once for all subsequent queries so you don't have to pass every time #103

Closed

tswast requested changes Feb 23, 2018

View reviewed changes

max-sixty and others added 15 commits March 31, 2018 16:17

Merge branch 'master' into default-project

8786e6d

gitignore

17a2ede

docstring

86b6fc5

don't skip if no project

11c146e

formatting

817c63a

add marks to allow test selecting

f0a8d36

assign mark to test

4ecfcfc

explicitly chose with auth to do from travis

c402952

too hasty to change the pandas installation

476bcf1

do what needs doing

ef59bca

Fixing style errors.

2112acd

docstring & import order

6cc99f6

correct mark expression

56436ff

project not required only if default creds available

5347858

remove any more branching in travis

cd9b37d

max-sixty commented Apr 1, 2018

View reviewed changes

tswast reviewed Apr 2, 2018

View reviewed changes

max-sixty added 2 commits April 2, 2018 19:42

Merge branch 'master' of https://github.com/pydata/pandas-gbq into de…

fa97b0f

…fault-project

google import inline

a7f6c43

tswast approved these changes Apr 2, 2018

View reviewed changes

tswast reviewed Apr 2, 2018

View reviewed changes

tswast mentioned this pull request Apr 19, 2018

ENH: Enable cross project queries ibis-project/ibis#1428

Closed

Merge remote-tracking branch 'upstream/master' into pr-127-maxim-lian…

2d50519

…-default-project

stickler-ci reviewed Apr 25, 2018

View reviewed changes

Use tuple for credentials & project for default project detection.

7932c59

tswast force-pushed the default-project branch from 3de277b to 7932c59 Compare April 25, 2018 18:04

tswast added 3 commits April 25, 2018 11:05

lint errors.

31e001f

Remove extra project detection.

08477cc

Update bad_project_id test to query actual data.

1f1f2c4

I think BigQuery stopped checking for valid project on queries with no data access.

Skip credentials tests if key not present.

d920959

DOC: add project_id optional to changelog

a3e6d2f

tswast merged commit 7711bb0 into googleapis:master Apr 25, 2018

max-sixty mentioned this pull request Apr 29, 2018

Explicit auth tests #170

Closed

project_id as optional #127

project_id as optional #127

Uh oh!

Conversation

max-sixty commented Feb 20, 2018

Uh oh!

codecov-io commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tswast left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-sixty Mar 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-sixty commented Apr 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-sixty commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-sixty commented Apr 25, 2018

Uh oh!

tswast commented Apr 25, 2018

Uh oh!

Uh oh!

codecov-io commented Feb 20, 2018 •

edited

Loading

max-sixty Mar 3, 2018 •

edited

Loading