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: Add done method for pipeline, training, and batch prediction jobs #1062

Merged
merged 10 commits into from
Mar 9, 2022

Conversation

sararob
Copy link
Contributor

@sararob sararob commented Mar 4, 2022

Added a done method via a DoneMixin class to check the status of long running jobs (returns True or False based on job state):

  • Implemented by PipelineJob, _Job, and _TrainingJob
  • Added system tests in aiplatform/tests/system/aiplatform/test_e2e_tabular.py
  • Added pipeline job tests in tests/unit/aiplatform/test_pipeline_jobs.py
  • Still need to add unit tests in test_jobs and test_training_jobs

Fixes b/215396514

@sararob sararob requested a review from sasha-gitg March 4, 2022 18:53
@parthea parthea added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Mar 5, 2022
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Mar 5, 2022
google/cloud/aiplatform/base.py Show resolved Hide resolved
network=_TEST_NETWORK,
)

mock_pipeline_service_create.assert_called_once_with(
Copy link
Member

Choose a reason for hiding this comment

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

You can skip the assertions that are tested in the other unit test.



class StatefulResource(DoneMixin):
"""Extends DoneMixin to check whether a job returning a stateful resource has compelted."""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Extends DoneMixin to check whether a job returning a stateful resource has compelted."""
"""Extends DoneMixin to check whether a job returning a stateful resource has completed."""

"""Extends StatefulResource to include a check for self._gca_resource."""

def done(self) -> bool:
if self._gca_resource:
Copy link
Member

Choose a reason for hiding this comment

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

Related to the test below that throws if run has not been invoked, which is surfaced from the state call above. An alternative is to always return False in those cases as well. This check can be updated to:

Suggested change
if self._gca_resource:
if self._gca_resource and self._gca_resource.name:

@sararob sararob requested a review from sasha-gitg March 9, 2022 19:45
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. Great work!

@sararob sararob added the automerge Merge the pull request once unit tests and other checks pass. label Mar 9, 2022
@gcf-merge-on-green gcf-merge-on-green bot merged commit f3338fc into googleapis:main Mar 9, 2022
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants