feat: Import client_abc from open source Vizier in Vertex Vizier SDK. by halio-g · Pull Request #1619 · googleapis/python-aiplatform

halio-g · 2022-08-29T20:29:07Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

…and system tests for them.

… the open source vizier.

…e confest.py test

…class.

rosiezou · 2022-09-30T22:55:18Z


    @classmethod
-    def from_uid(
+    def from_resource_name(


This is a change to the public interface. It could potentially break existing customers' code.

We can create another function of it. If it makes more sense.

rosiezou · 2022-09-30T22:56:22Z

        ]

-    def materialize_study_config(self) -> vz.StudyConfig:
+    def materialize_problem_statement(self) -> vz.ProblemStatement:


same comment as the one I left for def from_resource_name. I think this constitutes a breaking change.

We can create another function of it. If it makes more sense.

rosiezou · 2022-09-30T22:57:54Z

        cls,
        display_name: str,
-        problem: vz.ProblemStatement,
+        problem: vz.StudyConfig,


Looks like ProblemStatement is imported in pyvizier/__init__.py. Is the class no longer being used and its features are now completely replaced and/or incapsulated in StudyConfig?

The ProblemStatement is the rename of Study config.

ProblemStatement is the base class for StudyConfig, so both can be used from a pytype perspective.
Using StudyConfig here means that we want the Vertex specific StudyConfig, and not any subclass of ProblemStatement.

We expect Vertex users to only create StudyConfig objects, not ProblemStatement objects.

rosiezou · 2022-09-30T22:58:51Z

+    def from_resource_name(
        cls: Type[_T],
-        uid: str,
+        name: str,


same comment as above. This is a breaking change for customers who use keyword arguments in their function calls

Looks like the Vertex Vizier SDK is not used too much by the customers. I think it's fine to make the change on the API ? https://datastudio.google.com/c/u/0/reporting/d8a64931-8a91-4f07-9abc-93fece2e318d/page/p_n2odnmsgmc

rosiezou · 2022-09-30T22:59:16Z


-    def should_stop(self) -> bool:
-        """Returns true if the Trial should stop."""
+    def check_early_stopping(self) -> bool:


same comment as above. this is a breaking change

Looks like the Vertex Vizier SDK is not used too much by the customers. I think it's fine to make the change on the API ? https://datastudio.google.com/c/u/0/reporting/d8a64931-8a91-4f07-9abc-93fece2e318d/page/p_n2odnmsgmc

sagipe

halio@ this LGTM from my side. Please address other comments.

sararob · 2022-10-07T20:16:06Z

We are doing maintenance on our repo starting today and need to close all open PRs. When this is done next week, you'll be able to open a new PR.

halio-g and others added 30 commits April 27, 2022 13:41

Added the Vizier client in the aiplatform folder.

1be4dfa

Merge branch 'main' of https://github.com/halio-g/python-aiplatform

7e9e933

Merge branch 'googleapis:main' into main

8a83967

Copied the pyvizier from the open source vizier.

3efab09

Merge branch 'googleapis:main' into main

2934491

Added the Trial in the aiplatform folder.

f58f5a2

Merge remote-tracking branch 'upstream/main'

dfeaa67

Merge branch 'main' of https://github.com/halio-g/python-aiplatform

6336040

Forked the pyvizier from the pyVizier.

9524e9a

imported the objects from the pyvizier.

46cbe4a

Added the Vizier client and types in the aiplatform init file.

8122807

Made the pyvizier converters compatible with the Vertex SDK proto.

bb7c731

Forked the framework for the Vertex Vizier from the Open Source Vizier.

24425ae

Implemented the interfaces for Study and Trial. Added the unit tests …

6f24f9f

…and system tests for them.

Merge branch 'googleapis:main' into main

c188ed7

Merge remote-tracking branch 'upstream/main'

ca96954

Merge branch 'main' of https://github.com/halio-g/python-aiplatform

3ac5758

Setup the dependencies for the Vizier.

62ac4d7

Fix the lint error by running the nox -s blacken.

101fc60

Fixed the lint errors for the Vizier.

ede8356

Made the unit test import the google credentials.

f414c0f

Merge branch 'googleapis:main' into main

eb71423

Merge branch 'googleapis:main' into main

1929caf

Disable the coverage check for the pyvizier. It will be imported from…

6dc42e7

… the open source vizier.

Remove the converage dependency to avoid the conflicts.

cf8b213

Merge branch 'googleapis:main' into main

73fd1f0

Fixing the py-3.9 issue in the sample/module-builder

60a8653

Convert the lambda function to avoid the import numpy be called in th…

35e8799

…e confest.py test

Revert the requirements file.

38b55e0

Fix the lint error by running the nox -s blacken.

d681cea

halio-g and others added 2 commits August 29, 2022 13:22

Import the client_abc from vizier.client instead of vizier.pyvizier.

25ac646

Merge branch 'googleapis:main' into main

8d2e631

halio-g requested a review from a team August 29, 2022 20:29

product-auto-label Bot added size: l Pull request size is large. api: vertex-ai Issues related to the googleapis/python-aiplatform API. labels Aug 29, 2022

halio-g requested review from chansoo-google, rosiezou and sagipe August 29, 2022 20:47

sagipe reviewed Aug 29, 2022

View reviewed changes

Comment thread google/cloud/aiplatform/vizier/study.py

Comment thread google/cloud/aiplatform/vizier/trial.py

halio-g and others added 2 commits September 2, 2022 16:33

Merge branch 'googleapis:main' into main

8a04ace

Add the usage of from_resource_name.

e5b72bd

halio-g requested a review from sasha-gitg September 6, 2022 17:42

rosiezou suggested changes Sep 6, 2022

View reviewed changes

Comment thread google/cloud/aiplatform/vizier/trial.py Outdated

rosiezou added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Sep 8, 2022

sararob added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Sep 15, 2022

nayaknishant assigned rosiezou Sep 26, 2022

halio-g and others added 3 commits September 27, 2022 15:06

Merge branch 'googleapis:main' into main

e00c454

Fixed the issue in the trial to get project and location from parent …

42330b9

…class.

Added the dependency of google-vizier to 0.0.10

8a904f3

halio-g requested a review from a team September 30, 2022 20:21

halio-g and others added 2 commits September 30, 2022 13:22

Merge branch 'main' into main

9e56c93

Merge branch 'main' into main

4a5ae35

rosiezou suggested changes Sep 30, 2022

View reviewed changes

nayaknishant added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 30, 2022

sagipe approved these changes Oct 3, 2022

View reviewed changes

sararob removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 4, 2022

sararob closed this Oct 7, 2022

Conversation

halio-g commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagipe left a comment

Choose a reason for hiding this comment

Uh oh!

sararob commented Oct 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

halio-g commented Aug 29, 2022 •

edited

Loading