-
Notifications
You must be signed in to change notification settings - Fork 350
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: Add list() method to all resource nouns #294
Conversation
google/cloud/aiplatform/base.py
Outdated
Returns: | ||
Sequence[AiPlatformResourceNoun] - A list of SDK resource objects | ||
""" | ||
_UNSUPPORTED_LIST_ORDER_BY_TYPES = ( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 [] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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 🙂 |
There was a problem hiding this comment.
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.
Colab for manual testing
Summary of Changes
_list_method
property toAiPlatformResourceNoun
to store GAPIC method name for each nouncreate_time
andupdate_time
property toAiPlatformResourceNoun
list()
method that takes four optional fields and returns a list of SDK typesorder_by
are available in every GAPIC list methodsorder_by
aiplatform.init()
to test class setup and dropped it from some unit testsFixes b/183498826 🦕