Conversation

vinnysenthil

Colab for manual testing

Summary of Changes

  • Added a _list_method property to AiPlatformResourceNoun to store GAPIC method name for each noun
  • Added a create_time and update_time property to AiPlatformResourceNoun
  • Added a single list() method that takes four optional fields and returns a list of SDK types
    • All of the fields except order_by are available in every GAPIC list methods
    • Added local sorting for GAPIC list methods that do not take order_by
  • Added 3 unit tests to check correct GAPIC calls and local sorting
  • Added aiplatform.init() to test class setup and dropped it from some unit tests

Fixes b/183498826 🦕

@google-clagoogle-cla bot added the cla: yesThis human has signed the Contributor License Agreement.label Apr 7, 2021
@vinnysenthilvinnysenthil marked this pull request as ready for review April 7, 2021 17:57
@vinnysenthilvinnysenthil requested a review from a team as a code owner April 7, 2021 17:57
@vinnysenthilvinnysenthil changed the title feat: Add list() method to all resource nouns (Work In Progress) feat: Add list() method to all resource nouns Apr 7, 2021
Returns:
Sequence[AiPlatformResourceNoun] - A list of SDK resource objects
"""
_UNSUPPORTED_LIST_ORDER_BY_TYPES = (
Copy link
Member

Choose a reason for hiding this comment

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

Strong preference to break down this conditional logic at the subclass level instead.

One option is to have two private list classes and pass in the filtering attributes.

class AiPlatformResourceNoun:
  def _list(..., 
                order_by: Optional[str],
                gapic_field_filter_name: Optional[str]:None,
                cls_field_filter_name: Optional[str]: None):
    ...
    cls_filter_schema = getattr(cls, cls_field_filter_schema, None) if cls_field_filter_name else set([])
    final_list = [
                self._construct_sdk_resource_from_gapic(
                    gapic_resource, credentials=creds
                )
                for gapic_resource in resource_list
                if gapic_field_filter_key and getattr(gapic_resource, gapic_field_filter_key)
                in cls_filter_schema
            ]
  def _list_with_local_order(...,order_by: Optional[str]):
     li = cls._list(..., order_by=None)
     # order here
     return li
     
  def list(...):
    return cls._list(...)
     
class _TrainingJob:
   def list(...):
      return cls._list_with_local_order(
        ...,
        order,
        gapic_field_filter_name='training_task_definition',
        cls_field_filter_name='_supported_training_schemas') # could just pass in the cls attribute as well

This will be more extensible in the future and allow external teams to use the list functionality without needing to change the base class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for putting together this example! Adopting that option but will have the private _list method take a single cls_filter Callable that takes a gca_resource returns a bool that decides whether to include or exclude a given GAPIC object. Lmk if you have any concerns with that approach.

elif order_by:
list_request["order_by"] = order_by

resource_list = resource_list_method(request=list_request) or []
Copy link
Member

Choose a reason for hiding this comment

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

Why is the empty list needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into a few rare instances where the service returned a None instead of an empty list, this just serves as a fail safe against that.

@vinnysenthil

@sasha-gitg I've made changes that reflect the structure you suggested, PTAL at the commit and lmk if it looks good. If so, I'll add doc strings to the private methods and a bit more test coverage 🙂

Choose a reason for hiding this comment

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

LGTM! Thanks for this one. I have one main question on ordering.

@vinnysenthilvinnysenthil added the automergeMerge the pull request once unit tests and other checks pass.label Apr 11, 2021
@gcf-merge-on-greengcf-merge-on-green bot merged commit 3ec9386 into dev Apr 11, 2021
@gcf-merge-on-greengcf-merge-on-green bot deleted the mb-list branch April 11, 2021 05:34
@gcf-merge-on-greengcf-merge-on-green bot removed the automergeMerge the pull request once unit tests and other checks pass.label Apr 11, 2021
Sign up for free to join this conversation on . Already have an account? Sign in to comment
cla: yesThis human has signed the Contributor License Agreement.
None yet

Successfully merging this pull request may close these issues.