-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GAPIC Header Consistency: Vision #3050
Conversation
Taking a look now. |
vision/google/cloud/vision/_gax.py
Outdated
@@ -25,10 +26,14 @@ class _GAPICVisionAPI(object): | |||
|
|||
:type client: :class:`~google.cloud.vision.client.Client` | |||
:param client: Instance of ``Client`` object. | |||
|
|||
:type kwargs: dict | |||
:param kwargs: Additional keyword arguments are sent to the GAPIC client. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
vision/google/cloud/vision/client.py
Outdated
@@ -109,7 +109,8 @@ def _vision_api(self): | |||
""" | |||
if self._vision_api_internal is None: | |||
if self._use_gax: | |||
self._vision_api_internal = _GAPICVisionAPI(self) | |||
self._vision_api_internal = _GAPICVisionAPI(self, | |||
credentials=None) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
vision/unit_tests/test__gax.py
Outdated
from google.cloud.gapic.vision.v1.image_annotator_client import ( | ||
ImageAnnotatorClient) | ||
|
||
from .test_client import _make_credentials |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
vision/unit_tests/test__gax.py
Outdated
|
||
# Create the GAX client. | ||
credentials = _make_credentials() | ||
self._make_one(client=object(), credentials=credentials) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
vision/unit_tests/test__gax.py
Outdated
# Assert that the GAPIC constructor was called once, and | ||
# that the credentials were sent. | ||
iac.assert_called_once() | ||
self.assertIs(iac.mock_calls[0][2]['credentials'], credentials) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
vision/unit_tests/test__gax.py
Outdated
# that lib_name and lib_version were sent. | ||
iac.assert_called_once() | ||
self.assertEqual(iac.mock_calls[0][2]['lib_name'], 'gccl') | ||
self.assertEqual(iac.mock_calls[0][2]['lib_version'], __version__) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
vision/google/cloud/vision/_gax.py
Outdated
self._client = client | ||
self._annotator_client = image_annotator_client.ImageAnnotatorClient() | ||
self._annotator_client = image_annotator_client.ImageAnnotatorClient( | ||
credentials=credentials, lib_name='gccl', lib_version=__version__) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@daspecster Please verify now. (Also, obviously, waiting on tests.) |
vision/google/cloud/vision/_gax.py
Outdated
@@ -64,7 +70,7 @@ def _to_gapic_feature(feature): | |||
:param feature: Local ``Feature`` class to be converted to gRPC ``Feature`` | |||
instance. | |||
|
|||
:rtype: :class:`~google.cloud.grpc.vision.v1.image_annotator_pb2.Feature` | |||
:rtype: :class:`~google.cloudvision.v1.image_annotator_pb2.Feature` |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
vision/google/cloud/vision/client.py
Outdated
@@ -109,7 +109,8 @@ def _vision_api(self): | |||
""" | |||
if self._vision_api_internal is None: | |||
if self._use_gax: | |||
self._vision_api_internal = _GAPICVisionAPI(self) | |||
self._vision_api_internal = _GAPICVisionAPI( | |||
self, credentials=None) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -99,7 +99,7 @@ def from_api_repr(cls, response): | |||
def from_pb(cls, response): | |||
"""Factory: construct an instance of ``Annotations`` from protobuf. | |||
|
|||
:type response: :class:`~google.cloud.grpc.vision.v1.\ | |||
:type response: :class:`~google.cloudvision.v1.\ |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Pending travis/circle this LGTM. I would recommend running the system tests as well if you haven't yet. |
The Travis errors suggest the credentials are not being passed correctly. |
vision/unit_tests/test__gax.py
Outdated
|
||
# Create the GAX client. | ||
credentials = _make_credentials() | ||
client = Client(credentials=credentials) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@lukesneeringer cool looks like that worked. Just a lint issue now. |
@daspecster Would you please take a longer look at this one? I found the "two clients" thing really confusing and am not sure I did this correctly.
In particular, I am pretty sure the existing code threw away the
credentials
argument when being used with GAX. (Not sure how anyone would successfully set any of the other arguments, either.) I updated this and added a test for it.