Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add credentials parameter to read_gbq #78

Merged
merged 3 commits into from
Sep 6, 2023
Merged

Add credentials parameter to read_gbq #78

merged 3 commits into from
Sep 6, 2023

Conversation

jrbourbeau
Copy link
Member

This PR adds a new credentials= kwarg to read_gbq and pipes it down to the appropriate Google client library calls. For testing, I've copied our existing test_to_gbq_with_credentials test (which testing the same thing, but for to_gbq) and added a read_gbq where we pass credentials through.

This seems like the right thing to do, but I'm not very familiar with how Google handles auth. cc @ntabris who has more experience in that department

Closes #77

@ntabris
Copy link
Member

ntabris commented Aug 28, 2023

Hm, I'd expect this to work, but personally I'd prefer the API to take credentials object, rather than taking the dict. This would give more flexibility, and be more consistent with Google APIs.

@jrbourbeau
Copy link
Member Author

Thanks for taking a look at this @ntabris

personally I'd prefer the API to take credentials object, rather than taking the dict. This would give more flexibility, and be more consistent with Google APIs.

My guess is that folks coming from Dask will be used to passing as dict but, as you said, folks coming from Google APIs will be used to working with credentials objects. I think supporting both is inputs is reasonable. If we're happy with the current state of this PR, I'd like to stick with the dict for now (it matches what we already have for to_gbq) and extend to accepting Google credentials objects as needed in a follow up

@dchudz
Copy link

dchudz commented Aug 28, 2023

it matches what we already have for to_gbq

Consistency with what we already have in this package seems important, yeah! More important than these other considerations.

@ntabris
Copy link
Member

ntabris commented Aug 28, 2023

If we're happy with the current state of this PR, I'd like to stick with the dict for now (it matches what we already have for to_gbq) and extend to accepting Google credentials objects as needed in a follow up

Sure.

@j-bennet
Copy link
Contributor

Hm, I'd expect this to work, but personally I'd prefer the API to take credentials object, rather than taking the dict. This would give more flexibility, and be more consistent with Google APIs.

Credenticals object can't be pickled, no such problem with a dict.

@nick-amplify
Copy link

@jrbourbeau thanks for adding a fix so fast!!

@jrbourbeau
Copy link
Member Author

I'm going to merge this in as the hanging Python 3.8 build on macOS is unrelated to the changes in this PR (xref #80)

@jrbourbeau jrbourbeau merged commit e28e815 into main Sep 6, 2023
@jrbourbeau jrbourbeau deleted the read-creds branch September 6, 2023 17:05
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Passing Creds to read_gbq
5 participants