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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: parse project location when passed full resource name to get apis #297

Conversation

morgandu
Copy link
Contributor

@morgandu morgandu commented Apr 8, 2021

  • add _get_and_validate_project_location for resource_name in AiPlatformResourceNoun in base.py
  • pass in resource_name for AiPlatformResourceNoun subclasses.

unit tests:

  • add tests through dataset.Dataset
  • add tests through training_jobs.CustomTrainingJob.get

Fixes <b/180923329>

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 8, 2021
@morgandu morgandu force-pushed the mor--parse-project-location-when-passed-full-resource-name-to-get-apis branch from a2cc4a8 to 3907e4e Compare April 8, 2021 01:38
@morgandu morgandu requested a review from sasha-gitg April 8, 2021 01:39
@@ -320,6 +320,16 @@ def _get_gca_resource(self, resource_name: str) -> proto.Message:
project=self.project,
location=self.location,
)
(
Copy link
Member

Choose a reason for hiding this comment

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

I think this check needs to happen earlier in the stack than _get_gca_resource because the project/location of the object get set before this:

https://github.com/googleapis/python-aiplatform/blob/dev/google/cloud/aiplatform/datasets/dataset.py#L73
->
https://github.com/googleapis/python-aiplatform/blob/dev/google/cloud/aiplatform/base.py#L279

In this current implementation the location attribute and project attribute on the object may match the resource and client.

I think the most direct way to support this is to to add something like this to the AIPlatformResourceNoun base class:

def _get_and_validate_project_location(cls, project, location, resource_name_or_id):
  if not project and not location:   # no issue
    return project, location
  
  fields = extract_fields_from_resource_name(resource_name_or_id, cls._resource_noun)
  if not fields:  # not a resource name
        return project, location
 
  # skip validating project because it's not needed and would require we correlate project no. with project ids 

  if location and field.location != location:  # this is a problem
      raise runtimeerror
  
  return fields.project, fields.location

And update the constructor with the something like:

def __init__(self, project: Optional, location: Optional, creds: Optional, resource_name_or_id: Optional[str]):
    if resource_name_or_id:
      project, location = self._get_and_validate_project_location(project, location, resource_name_or_id)
    ...

And update subclass call sites to pass in the resource noun in the constructor.

@morgandu morgandu force-pushed the mor--parse-project-location-when-passed-full-resource-name-to-get-apis branch from 3907e4e to 2e02b75 Compare April 8, 2021 22:32
@morgandu morgandu requested a review from a team as a code owner April 8, 2021 22:32
@morgandu morgandu requested a review from sasha-gitg April 8, 2021 22:37
@morgandu morgandu changed the title [WIP] Feat: parse project location when passed full resource name to get apis Feat: parse project location when passed full resource name to get apis Apr 8, 2021
@morgandu morgandu force-pushed the mor--parse-project-location-when-passed-full-resource-name-to-get-apis branch from 2e02b75 to 7a158ee Compare April 8, 2021 22:38
@morgandu morgandu force-pushed the mor--parse-project-location-when-passed-full-resource-name-to-get-apis branch from 7a158ee to 98612c2 Compare April 8, 2021 22:40
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.

LGTM! Thanks Morgan!

@morgandu morgandu merged commit 674227d into googleapis:dev Apr 9, 2021
@morgandu morgandu deleted the mor--parse-project-location-when-passed-full-resource-name-to-get-apis branch September 1, 2021 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

3 participants