Conversation

sasha-gitg

No description provided.

@sasha-gitgsasha-gitg added the do not mergeIndicates a pull request not ready for merge, due to either quality or timing.label May 14, 2021
@sasha-gitgsasha-gitg requested review from a team as code owners May 14, 2021 15:55
@product-auto-labelproduct-auto-label bot added the api: aiplatformIssues related to the AI Platform API.label May 14, 2021
@google-clagoogle-cla bot added the cla: yesThis human has signed the Contributor License Agreement.label May 14, 2021
@sasha-gitgsasha-gitg removed theIndicates a pull request not ready for merge, due to either quality or timing.label May 17, 2021
ivanmkc
ivanmkc reviewed May 17, 2021
View reviewed changes
ivanmkc
ivanmkc reviewed May 17, 2021
View reviewed changes
ivanmkc
ivanmkc reviewed May 17, 2021
View reviewed changes
ivanmkc
ivanmkc reviewed May 17, 2021
View reviewed changes
vinnysenthil
vinnysenthil approved these changes May 18, 2021
View reviewed changes

Choose a reason for hiding this comment

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

Looks great, thanks for getting this in time for the release 👏🏽
Just a few nits and a requested change around search_algorithms.

  • Code review
  • Successful manual test of HPT
  • Successful manual test of CustomJob

google/cloud/aiplatform/base.py
Comment on lines +569 to +572
@property
def gca_resource(self) -> proto.Message:
"""The underlying resource proto represenation."""
return self._gca_resource
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we exposing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Customers may need a field we haven't exposed. This allows us to provide code that doesn't require referencing a private attribute.

sasha-gitg and others added 9 commits May 18, 2021 09:12
@sasha-gitg @vinnysenthil
Update google/cloud/aiplatform/hyperparameter_tuning.py
a492741
Co-authored-by: Vinny Senthil <[email protected]>
@sasha-gitg @vinnysenthil
Update google/cloud/aiplatform/jobs.py
24d3949
Co-authored-by: Vinny Senthil <[email protected]>
@sasha-gitg @vinnysenthil
Update google/cloud/aiplatform/jobs.py
8e3e994
Co-authored-by: Vinny Senthil <[email protected]>
@sasha-gitg @vinnysenthil
Update google/cloud/aiplatform/jobs.py
2f35579
Co-authored-by: Vinny Senthil <[email protected]>
@sasha-gitg @vinnysenthil
Update google/cloud/aiplatform/jobs.py
c30a80f
Co-authored-by: Vinny Senthil <[email protected]>
@sasha-gitg @vinnysenthil
Update google/cloud/aiplatform/jobs.py
3a66659
Co-authored-by: Vinny Senthil <[email protected]>
@sasha-gitg @vinnysenthil
Update google/cloud/aiplatform/jobs.py
1c91685
Co-authored-by: Vinny Senthil <[email protected]>
@sasha-gitg @vinnysenthil
Update google/cloud/aiplatform/jobs.py
e6a53b5
Co-authored-by: Vinny Senthil <[email protected]>
@sasha-gitg @vinnysenthil
Update google/cloud/aiplatform/jobs.py
4e07e6d
Co-authored-by: Vinny Senthil <[email protected]>
@sasha-gitg

Thanks for the awesome review Vinny!

@sasha-gitgsasha-gitg merged commit aab9e58 into googleapis:master May 18, 2021
Sign up for free to join this conversation on . Already have an account? Sign in to comment

@ivanmkc ivanmkc ivanmkc left review comments

@vinnysenthil vinnysenthil vinnysenthil approved these changes

@morgandu morgandu Awaiting requested review from morgandu

No one assigned
api: aiplatformIssues related to the AI Platform API.cla: yesThis human has signed the Contributor License Agreement.
None yet
No milestone

Successfully merging this pull request may close these issues.