Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Set prediction client when listing Endpoints #512

Merged
merged 5 commits into from Jun 30, 2021

Conversation

vinnysenthil
Copy link
Contributor

Overrides VertexAiResourceNounWithFutureManager._construct_sdk_resource_from_gapic with Endpoint._construct_sdk_resource_from_gapic.

Subclass method initializes the Prediction service client, allowing users to call predict() / explain() on an Endpoint returned by a call to list()

Fixes b/192002630 馃

@product-auto-label product-auto-label bot added the api: aiplatform Issues related to the AI Platform API. label Jun 25, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 25, 2021
Copy link
Member

@sasha-gitg sasha-gitg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should also include a test to confirm the fix. Thanks!


endpoint._gca_resource = gapic_resource
endpoint._prediction_client = self._instantiate_prediction_client(
location=location or initializer.global_config.location,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized there is a bug here and in the implementation it's based on:

https://github.com/googleapis/python-aiplatform/blob/master/google/cloud/aiplatform/models.py#L119-L122

If passed a full resource name that doesn't match location or init.location then will instantiate the client in the incorrect location.

Please change this here and in the reference above to the use the constructed endpoint resource's location.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added calls to _get_and_validate_project_location() in both Endpoint.__init__() and Endpoint._construct_sdk_resource_from_gapic() to allow the full resource name to override location.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I realized that downstream calls in VertexAiResourceNounWithFutureManager._empty_constructor() and VertexAiResourceNounWithFutureManager.__init__() already call _get_and_validate_project_location().

I've updated the call to _instantiate_prediction_client() to use the location set by those methods instead of the location argument.

google/cloud/aiplatform/models.py Outdated Show resolved Hide resolved
Copy link
Member

@sasha-gitg sasha-gitg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. Thanks, Vinny!

@vinnysenthil vinnysenthil merged commit 95639ee into master Jun 30, 2021
@vinnysenthil vinnysenthil deleted the pred-listed-endpoint branch June 30, 2021 21:40
ji-yaqi pushed a commit to ji-yaqi/python-aiplatform that referenced this pull request Jun 30, 2021
* fix: Set prediction client when listing Endpoints

* Address reviewer comments

* Remove redundant init() in TestEndpoints

* Update location passed to _instantiate_prediction_client()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: aiplatform Issues related to the AI Platform API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants