Skip to content

feat: Vertex AI Experiments GA#1410

Merged
sasha-gitg merged 17 commits intogoogleapis:mainfrom
sasha-gitg:experiments-release
Jun 13, 2022
Merged

feat: Vertex AI Experiments GA#1410
sasha-gitg merged 17 commits intogoogleapis:mainfrom
sasha-gitg:experiments-release

Conversation

@sasha-gitg
Copy link
Copy Markdown
Member

No description provided.

@sasha-gitg sasha-gitg requested a review from a team June 6, 2022 16:50
@sasha-gitg sasha-gitg requested a review from a team as a code owner June 6, 2022 16:50
@sasha-gitg sasha-gitg requested a review from dandhlee June 6, 2022 16:50
@product-auto-label product-auto-label Bot added size: xl Pull request size is extra large. api: vertex-ai Issues related to the googleapis/python-aiplatform API. labels Jun 6, 2022
@snippet-bot
Copy link
Copy Markdown

snippet-bot Bot commented Jun 6, 2022

Here is the summary of changes.

You are about to add 20 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@sasha-gitg sasha-gitg force-pushed the experiments-release branch 10 times, most recently from c0f099d to 4609ebb Compare June 7, 2022 17:26
@sasha-gitg sasha-gitg force-pushed the experiments-release branch 2 times, most recently from 040c59f to 786bd01 Compare June 8, 2022 13:41
@sasha-gitg sasha-gitg force-pushed the experiments-release branch from 786bd01 to 978ecb4 Compare June 8, 2022 13:52
@jialuzh jialuzh requested a review from SinaChavoshi June 8, 2022 17:43
Copy link
Copy Markdown
Contributor

@sararob sararob left a comment

Choose a reason for hiding this comment

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

Here's my review of the Artifact class.

Comment thread tests/unit/aiplatform/test_metadata.py Outdated
Comment thread google/cloud/aiplatform/metadata/artifact.py
Comment thread google/cloud/aiplatform/metadata/artifact.py
Comment thread google/cloud/aiplatform/metadata/artifact.py
Comment thread google/cloud/aiplatform/metadata/artifact.py Outdated
Comment thread google/cloud/aiplatform/metadata/artifact.py
Comment thread google/cloud/aiplatform/metadata/artifact.py Outdated
Comment thread google/cloud/aiplatform/metadata/artifact.py
Comment thread google/cloud/aiplatform/metadata/artifact.py
Comment thread tests/unit/aiplatform/test_metadata_resources.py
Copy link
Copy Markdown
Contributor

@samgoodman samgoodman left a comment

Choose a reason for hiding this comment

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

Small fixes and suggestions, overall LGTM

Comment thread google/cloud/aiplatform/initializer.py Outdated
Comment thread google/cloud/aiplatform/initializer.py Outdated
Comment thread google/cloud/aiplatform/initializer.py Outdated
Comment thread google/cloud/aiplatform/metadata/artifact.py Outdated
Comment thread google/cloud/aiplatform/metadata/artifact.py Outdated
Comment thread google/cloud/aiplatform/metadata/experiment_run_resource.py
Comment thread google/cloud/aiplatform/metadata/metadata_store.py
Comment thread google/cloud/aiplatform/metadata/metadata.py
Comment thread google/cloud/aiplatform/metadata/experiment_run_resource.py Outdated
Comment thread google/cloud/aiplatform/metadata/utils.py Outdated
@sasha-gitg sasha-gitg added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 9, 2022
Copy link
Copy Markdown
Contributor

@sararob sararob left a comment

Choose a reason for hiding this comment

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

Review of Context, Execution, Experiment Resources, and some tests.

Comment thread google/cloud/aiplatform/metadata/execution.py
Comment thread google/cloud/aiplatform/metadata/execution.py
Comment thread google/cloud/aiplatform/metadata/execution.py
Comment thread google/cloud/aiplatform/metadata/execution.py Outdated
Comment thread google/cloud/aiplatform/metadata/context.py
Comment thread google/cloud/aiplatform/metadata/experiment_resources.py
Comment thread google/cloud/aiplatform/metadata/experiment_resources.py Outdated
Comment thread google/cloud/aiplatform/metadata/experiment_resources.py
Comment thread google/cloud/aiplatform/metadata/experiment_resources.py
Comment thread google/cloud/aiplatform/metadata/experiment_resources.py Outdated
@sasha-gitg sasha-gitg removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 9, 2022
Comment thread google/cloud/aiplatform/__init__.py
Comment thread google/cloud/aiplatform/metadata/constants.py
Comment thread google/cloud/aiplatform/metadata/artifact.py
Comment thread google/cloud/aiplatform/metadata/artifact.py Outdated

class _VertexResourceArtifactResolver:

_resource_to_artifact_type = {models.Model: "google.VertexModel"}
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.

IIUC, this resolver would automatically link a resource to a MLMD artifact? And if so, is Model the only one we would support for GA? How about managed dataset?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I opened a ticket and added TODO for managed dataset. Let me sync with the service team to see if we can get this in before GA.

Comment thread google/cloud/aiplatform/metadata/context.py Outdated
Comment thread google/cloud/aiplatform/metadata/execution.py
Comment thread google/cloud/aiplatform/metadata/execution.py
Comment thread google/cloud/aiplatform/metadata/metadata.py Outdated
Comment thread google/cloud/aiplatform/metadata/metadata.py
Comment thread google/cloud/aiplatform/metadata/experiment_run_resource.py Outdated

@staticmethod
def _get_experiment(
experiment: Optional[Union[experiment_resources.Experiment, str]] = 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.

Flagging since in the docstring this is called experiment_name and is required.

Comment thread google/cloud/aiplatform/metadata/experiment_run_resource.py Outdated
Comment thread google/cloud/aiplatform/metadata/experiment_run_resource.py
Comment thread google/cloud/aiplatform/metadata/experiment_run_resource.py
Comment thread google/cloud/aiplatform/metadata/experiment_run_resource.py Outdated
Comment thread google/cloud/aiplatform/metadata/metadata.py Outdated
Comment thread google/cloud/aiplatform/metadata/metadata.py Outdated
Comment thread google/cloud/aiplatform/metadata/metadata.py Outdated
Comment thread google/cloud/aiplatform/tensorboard/tensorboard_resource.py
Copy link
Copy Markdown
Contributor

@sararob sararob left a comment

Choose a reason for hiding this comment

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

My last batch of review, great job!

Comment thread tests/system/aiplatform/test_experiments.py Outdated
Comment thread tests/system/aiplatform/test_experiments.py
Comment thread tests/system/aiplatform/test_tensorboard.py

from google.cloud.aiplatform import initializer
from google.cloud.aiplatform.metadata.metadata import metadata_service
from google.cloud.aiplatform.metadata.metadata import _experiment_tracker
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.

Here's the error when I try to run test_initializer:

__________________________________________________________________________________________________________ ERROR collecting tests/unit/aiplatform/test_initializer.py __________________________________________________________________________________________________________
ImportError while importing test module '/Users/sararob/Dev/sara-fork/python-aiplatform/tests/unit/aiplatform/test_initializer.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.8/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/unit/aiplatform/test_initializer.py:28: in <module>
    from google.cloud.aiplatform.metadata.metadata import _experiment_tracker
E   ImportError: cannot import name '_experiment_tracker' from 'google.cloud.aiplatform.metadata.metadata' (/Users/sararob/Library/Python/3.8/lib/python/site-packages/google/cloud/aiplatform/metadata/metadata.py)

@sasha-gitg sasha-gitg force-pushed the experiments-release branch from 8c5cde5 to 58e426f Compare June 13, 2022 17:42
@sasha-gitg sasha-gitg merged commit 24d1bb6 into googleapis:main Jun 13, 2022
for key, value in metadata.items():
# Note: This only support nested dictionaries one level deep
if isinstance(value, collections.Mapping):
gca_resource.metadata[key].update(value)
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.

What is the proposed way to remove keys from a nested dictionary?

P.S. This also seems to fail when the key does not already exist in the metadata.

javabrett added a commit to javabrett/python-aiplatform that referenced this pull request Apr 19, 2026
…ing metadata store init

Fixes googleapis#6610.

Two bugs in `metadata/execution.py` caused `Execution.create()` to fail
with "503 Getting metadata from plugin failed with error: before_request":

1. The `credentials` parameter on both `create()` and `_create()` used
   `=` instead of `:` for the type annotation:
   `credentials=Optional[auth_credentials.Credentials]` instead of
   `credentials: Optional[auth_credentials.Credentials] = None`.
   This made the default value the `typing.Optional` type object itself
   (not `None`), which the gRPC auth stack tried to call
   `.before_request()` on.

2. `create()` skipped the `ensure_default_metadata_store_exists()` call
   that `Artifact.create()` makes, so standalone `Execution.create()`
   would also fail if no prior operation had initialized the metadata
   store.

Both issues have been present since PR googleapis#1410 (June 2022).
`artifact.py` and `context.py` both have the correct annotation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: vertex-ai Issues related to the googleapis/python-aiplatform API. size: xl Pull request size is extra large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants