Skip to content

feat: LRO handler#30

Merged
sirtorry merged 38 commits into
googleapis:devfrom
sirtorry:long-running-op
Oct 29, 2020
Merged

feat: LRO handler#30
sirtorry merged 38 commits into
googleapis:devfrom
sirtorry:long-running-op

Conversation

@sirtorry
Copy link
Copy Markdown
Contributor

  • sets refresh, cancel and result_type to default
  • automatically attaches callback

fixes b/169779287

@google-cla google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 19, 2020
@sirtorry sirtorry requested a review from sasha-gitg October 19, 2020 22:26
Comment thread google/cloud/aiplatform/lro.py Outdated
Comment thread google/cloud/aiplatform/lro.py Outdated
Comment thread google/cloud/aiplatform/lro.py Outdated
Comment thread google/cloud/aiplatform/lro.py Outdated
Comment thread google/cloud/aiplatform/lro.py Outdated
Comment thread google/cloud/aiplatform/lro.py Outdated
Comment thread tests/unit/aiplatform/test_lro.py Outdated
Comment thread tests/unit/aiplatform/test_lro.py Outdated
Copy link
Copy Markdown
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.

High level comment, taking this LRO as an example: https://github.com/googleapis/python-aiplatform/blob/dev/google/cloud/aiplatform/models.py#L211

When we receive an LRO from the GAPIC API call, it's already initialized and polling so we may not want to initialize a separate LRO and have two LROs polling for the same result. Unless we want to delete the initial LRO which I think adds too much complexity for what we want.

Instead, I think we want a Class that takes the LRO initialized by the GAPIC library as an attribute, exposes methods that are relevant to the MB usage (result, add_done_callback).

Additionally, we have a requirement to be able to update a MB object with resource properties once the LRO is complete. In the example linked above that would look something like:

lro = api_client.upload_model(...)
mb_lro = LRO(lro)
empty_model = cls.init_from_lro(api_client, mb_lro)
return empty_model

The init_from_lro on Model would look something like:

@classmethod
def init_from_lro(cls, api_client, lro):
  self = cls.__new__(cls)
  self.api_client = api_client
  self.lro = lro
  self.lro.add_update_resource_callback(
     self, lro_result_key='model, api_get=lambda name: api_client.get_model(name=name) )
  return self

So the LRO class would look something like:

class LRO:

  def __init__(self, gapic_lro: operation.Operation):
    self._lro = gapic_lro

 def add_update_resource_callback(self,
   resource_noun_obj: base.AiPlatformResourceNoun,
   result_key: str, api_get: Callable):
   def callback(result):
     result_value = getattr(result, result_key)
     resource = api_get(result_value)
     resource_noun_obj._gca_resource = resource
   self._lro.add_done_callback(callback)

Comment thread google/cloud/aiplatform/lro.py Outdated
Comment thread google/cloud/aiplatform/lro.py Outdated
Comment thread tests/unit/aiplatform/test_lro.py Outdated
Comment thread google/cloud/aiplatform/lro.py Outdated
Comment thread google/cloud/aiplatform/lro.py Outdated
Comment thread google/cloud/aiplatform/lro.py
Comment thread tests/unit/aiplatform/test_lro.py Outdated
Comment thread tests/unit/aiplatform/test_lro.py Outdated
@dizcology dizcology added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 20, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 20, 2020
Comment thread google/cloud/aiplatform/lro.py
Comment thread google/cloud/aiplatform/lro.py Outdated
return operation_proto


def make_operation_future(client_operations_responses=None):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For later: move this into test fixtures.

Comment thread tests/unit/aiplatform/test_lro.py Outdated
@sirtorry
Copy link
Copy Markdown
Contributor Author

adding a blocking version of resource updating

@sirtorry sirtorry changed the title basic LRO handler feat: LRO handler Oct 26, 2020
Copy link
Copy Markdown
Contributor

@dizcology dizcology left a comment

Choose a reason for hiding this comment

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

Please make sure the kokoro build passes before merging.

Comment thread google/cloud/aiplatform/lro.py
Comment thread tests/unit/aiplatform/test_lro.py Outdated
@sirtorry sirtorry merged commit 8036360 into googleapis:dev Oct 29, 2020
@sirtorry sirtorry deleted the long-running-op branch October 29, 2020 19:40
dizcology added a commit to dizcology/python-aiplatform that referenced this pull request Nov 30, 2020
* first lro attempt

* address comments

* add test

Co-authored-by: Yu-Han Liu <yuhanliu@google.com>
dizcology added a commit to dizcology/python-aiplatform that referenced this pull request Nov 30, 2020
* first lro attempt

* address comments

* add test

Co-authored-by: Yu-Han Liu <yuhanliu@google.com>
dizcology added a commit to dizcology/python-aiplatform that referenced this pull request Nov 30, 2020
* first lro attempt

* address comments

* add test

Co-authored-by: Yu-Han Liu <yuhanliu@google.com>
dizcology added a commit to dizcology/python-aiplatform that referenced this pull request Nov 30, 2020
* first lro attempt

* address comments

* add test

Co-authored-by: Yu-Han Liu <yuhanliu@google.com>
dizcology added a commit to dizcology/python-aiplatform that referenced this pull request Nov 30, 2020
* first lro attempt

* address comments

* add test

Co-authored-by: Yu-Han Liu <yuhanliu@google.com>
dizcology added a commit to dizcology/python-aiplatform that referenced this pull request Dec 22, 2020
* first lro attempt

* address comments

* add test

Co-authored-by: Yu-Han Liu <yuhanliu@google.com>
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.

6 participants