Pass kwargs to discovery.build() when instantiating GSCClient.
#2291
Conversation
danthelion
force-pushed
the
gcs_client_discovery_build_kwargs
branch
from
1841585
to
97c140e
Compare
|
@dlstadther These changes are quite simple and would help a lot to debug using the logs. Any possibility of an admin having a look? |
|
@danthelion could you rebase this branch with the new version of gcs? |
danthelion
force-pushed
the
gcs_client_discovery_build_kwargs
branch
from
97c140e
to
e802bc3
Compare
|
@dlstadther Done. |
luigi/contrib/gcs.py
Outdated
| else: | ||
| self.client = discovery.build('storage', 'v1', cache_discovery=False, **authenticate_kwargs) | ||
| self.client = discovery.build('storage', 'v1', **build_kwargs) |
There was a problem hiding this comment.
Yes, the original intention of my PR was regarding this parameter a well.
I think it's better to modify it on the client side (by propagating it with build_kwargs) and True is always a sane default for caching functionalities.
There was a problem hiding this comment.
@himikof Would you like to voice your opinion on moving the default back to True?
There was a problem hiding this comment.
Well, though I in general agree that turning caching on is a sane default in general, I do disagree in this particular case. The discovery caching in googleapiclient is mostly broken, as indicated in the linked issues, and nobody tried to fix this upstream for several years. The chances of file caching working are even more reduced by #2361 dropping oauth2client dependency.
The best solution would be to have some kind of simple in-memory cache without external dependencies by default in the upstream googleapiclient, but I do not believe this is a Luigi issue.
What Luigi can do, though, is to ensure GCP-related functionality works out of the box, without getting scary ImportError log spam or learning about API discovery caching (which is not that important to know about).
So I would say that getting a way to pass custom discovery kwargs is certainly an improvement, but I feel that Luigi should change the default of the cache_discovery value, at least until the discovery caching works out of the box in the upstream library. It is a simple matter of doing kwargs.setdefault('cache_discovery', False).
danthelion
force-pushed
the
gcs_client_discovery_build_kwargs
branch
from
e802bc3
to
8d927f4
Compare
|
@himikof I have added |
Description
https://github.com/google/google-api-python-client/blob/89906ac33b37c6017c893841743aa4f45729c91f/googleapiclient/discovery.py#L175
https://github.com/google/google-api-python-client/blob/89906ac33b37c6017c893841743aa4f45729c91f/googleapiclient/discovery.py#L297
The
buildandbuild_from_documentmethods has some useful keyword arguments that are currently not useable from theGSCClientinterface.Motivation and Context
My use case for these changes is getting rid of warning log spam because of a default argument (
cache_discovery=True)googleapis/google-api-python-client#299
googleapis/oauth2client#673
Have you tested this? If so, how?
Locally, running my pipeline with the
cache_discovery=Falseargument passed on instantiation.