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: Added deploy_model_with_dedicated_resources_sample, deploy_model_with_automatic_resources_sample, upload_model and get_model samples #337
Conversation
Here is the summary of changes. You are about to add 4 region tags.
This comment is generated by snippet-bot.
|
32ef90b
to
7a646b4
Compare
@@ -16,6 +16,7 @@ | |||
from uuid import uuid4 | |||
|
|||
from google.auth import credentials | |||
from google.cloud import aiplatform |
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 needed to import this for aiplatform.explain.ExplanationMetadata
samples/model-builder/conftest.py
Outdated
yield mock | ||
|
||
@pytest.fixture | ||
def mock_batch_predict_model(mock_model): |
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.
Updated these fixtures to use the new mock_model
e4be6f4
to
704ca9f
Compare
deployed_model_display_name: Optional[str] = None, | ||
traffic_percentage: Optional[int] = 0, | ||
traffic_split: Optional[Dict[str, int]] = None, | ||
min_replica_count: Optional[int] = 1, |
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.
Please remove the Optional type hint (just leave int
) on min_replica_count
and max_replica_count
. After reviewing this, I realized Endpoint.deploy and Model.deploy have inconsistent types on those arguments. I opened a PR to fix the function type hints in the SDK here: #345
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.
Done.
deployed_model_display_name: Optional[str] = None, | ||
traffic_percentage: Optional[int] = 0, | ||
traffic_split: Optional[Dict[str, int]] = None, | ||
min_replica_count: Optional[int] = 1, |
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.
Same comment on min_replica_count
and max_replica_count
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.
Done.
from google.cloud.aiplatform import explain | ||
|
||
# [START aiplatform_sdk_deploy_model_sample] | ||
def deploy_model_sample( |
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'm assuming this is for models with automatic resources as machine type is not exposed. If that's the case, I think the following arguments can be removed but please confirm:
accelerator_type
accelerator_count
explanation_metadata
explanation_parameters
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.
Yes it's for automatic resources.
I checked the internal deploy function and it looks like accelerator_type
and accelerator_count
are only used if machine-type
is provided. However, even if it intuitively makes sense, I would argue that the docstrings should make their co-dependence clear.
I would also recommend a refactor to bundle machine_type
, accelerator_type
and accelerator_count
together, since they can't be used independently. If you want to keep the user-facing API flat, that's fine but in private functions it will be cleaner bundled. This would also reduce duplication of logic (search for if machine_type:
). Let me know if you agree, and I can do this in a new PR.
From looking at _deploy_call
, I don't see explanation_metadata
and explanation_parameters
relying on machine_type
being set. Is there some restriction from the service the requires `machine_type?
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.
Agree on the refactor and updating the docstring. Only custom models take explanation_metadata
and explanation_parameters
as AutoMLs support it natively (if supported) and machine_type is required from custom models.
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.
@vinnysenthil to confirm
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.
@sasha-gitg Logged a bug for refactor: https://b.corp.google.com/issues/186121905
30acf42
to
bd4e0a8
Compare
|
||
model = aiplatform.Model(model_name=model_name) | ||
|
||
# AutoML models require a machine_type of 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.
@vinnysenthil do all AutoML Models require automatic resources?
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.
Actually, we talked about this and it's a little unclear what the truth of this is.
We're still trying to find out the answer from the predict team.
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.
@sasha-gitg I talked to prediction team and unfortunately, they do not have the answer. I will remove the comment for now and update this if I find out more.
035197f
to
d8c3d8a
Compare
d8c3d8a
to
8d474d6
Compare
8d474d6
to
4131edf
Compare
Added deploy_model_with_dedicated_resources_sample, deploy_model_with_automatic_resources_sample, upload_model and get_model samples and tests.