-
Notifications
You must be signed in to change notification settings - Fork 348
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: Vertex AI Experiments GA #1410
Conversation
c0f099d
to
4609ebb
Compare
…platform into experiments-release
040c59f
to
786bd01
Compare
786bd01
to
978ecb4
Compare
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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.
|
||
class _VertexResourceArtifactResolver: | ||
|
||
_resource_to_artifact_type = {models.Model: "google.VertexModel"} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
||
@staticmethod | ||
def _get_experiment( | ||
experiment: Optional[Union[experiment_resources.Experiment, str]] = None, |
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
@@ -25,7 +25,7 @@ | |||
from google.auth import credentials | |||
|
|||
from google.cloud.aiplatform import initializer | |||
from google.cloud.aiplatform.metadata.metadata import metadata_service | |||
from google.cloud.aiplatform.metadata.metadata import _experiment_tracker |
There was a problem hiding this comment.
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)
8c5cde5
to
58e426f
Compare
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) |
There was a problem hiding this comment.
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.
No description provided.