Skip to content
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

ODSC 29065/Model Deployment opctl for local dev #134

Merged
merged 23 commits into from
May 2, 2023
Merged

Conversation

z7ye
Copy link
Contributor

@z7ye z7ye commented Apr 5, 2023

Description

Download Model

ads opctl model download ocid1.datasciencemodel.oc1.*** -f --bucket-uri oci://ads_test@ociodscdev/model

image

Auto Detect and Install Conda

ads opctl predict --ocid ocid1.datasciencemodel.oc1.iad.*** --payload '[[-1.68671955,2.25814541,1.48317577,1.51661083,-0.20669947,-0.4260807,-1.02519643,-0.5068027,0.25248417,0.62665134,0.23441123,-0.27358368,0.82731396,-0.14530245,0.80733585]]' --bucket-uri oci://ads_test@ociodscdev/model --conda-pack-folder ~/.ads_ops/conda2
image

Small artifacts

ads opctl predict --ocid ocid1.datasciencemodel.oc1.iad.*** --payload '[[-1.68671955,2.25814541,1.48317577,1.51661083,-0.20669947,-0.4260807,-1.02519643,-0.5068027,0.25248417,0.62665134,0.23441123,-0.27358368,0.82731396,-0.14530245,0.80733585]]' --conda-slug onnx110_p38_cpu_v2
image

Artifacts Dir

ads opctl predict --artifact-directory ~/.ads_ops/models/ocid1.datasciencemodel.oc1.iad.*** --payload '[[-1.68671955,2.25814541,1.48317577,1.51661083,-0.20669947,-0.4260807,-1.02519643,-0.5068027,0.25248417,0.62665134,0.23441123,-0.27358368,0.82731396,-0.14530245,0.80733585]]' --conda-slug onnx110_p38_cpu_v2
image

Large Artifacts

ads opctl predict --ocid ocid1.datasciencemodel.oc1.iad.*** --payload '[[-1.68671955,2.25814541,1.48317577,1.51661083,-0.20669947,-0.4260807,-1.02519643,-0.5068027,0.25248417,0.62665134,0.23441123,-0.27358368,0.82731396,-0.14530245,0.80733585]]' --conda-slug onnx110_p38_cpu_v2 --bucket-uri oci://ads_test@ociodscdev/model

image

@oracle-contributor-agreement
Copy link

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. label Apr 5, 2023
@z7ye z7ye changed the base branch from main to develop April 5, 2023 03:54
@oracle-contributor-agreement oracle-contributor-agreement bot added OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. labels Apr 5, 2023
@z7ye z7ye marked this pull request as draft April 5, 2023 03:54
f"Conda pack {conda_pack_path} not found. Please run `ads opctl conda create` or `ads opctl conda install`."
)
if install:
logger.info(f"Downloading the conda pack {slug} to this conda pack {conda_pack_folder}. If this conda pack is already installed locally in a different location, pass in `conda_pack_folder` to avoid downloading it again.")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe?

Downloading a `{slug}` to the `{conda_pack_folder}`. If this conda pack is already installed locally in a different location, pass in `conda_pack_folder` to avoid downloading it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup. fixed.

self.config = config
self.oci_auth = get_signer(
self.oci_auth = create_signer(
config["execution"].get("auth"),
Copy link
Member

Choose a reason for hiding this comment

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

This is a good change :)


def predict(self) -> None:
"""
Deactivate a remote service.
Copy link
Member

Choose a reason for hiding this comment

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

Wrong description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -170,3 +172,11 @@ def watch(self) -> None:
model_deployment.watch(
log_type=log_type, interval=interval, log_filter=log_filter
)

def predict(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a description for this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

from ads.common.decorator.runtime_dependency import (OptionalDependency,
runtime_dependency)
from ads.common.oci_client import OCIClientFactory
from ads.model.datascience_model import DataScienceModel
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -54,6 +58,17 @@ def __init__(self, config: Dict) -> None:
dictionary of configurations
"""
self.config = config
self.auth_type = config["execution"].get("auth")
Copy link
Member

Choose a reason for hiding this comment

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

We are doing the same and the same steps in every backend, maybe lines 61->71 can be moved to the base class?

Copy link
Contributor Author

@z7ye z7ye May 1, 2023

Choose a reason for hiding this comment

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

actually, i realized I should not add it to localbackend since it will affect local pipeline backend and etc. So I moved it to only LocalModeDeploymentBackend.

if not compartment_id or not project_id:
raise ValueError("`compartment_id` and `project_id` must be provided.")

extra_cmd = "/opt/ds/model/deployed_model/ " + data + " " + compartment_id + " " + project_id
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the /opt/ds/model/deployed_model/ is used in many places. I would suggest to move it to the constants.

extra_cmd = f"/opt/ds/model/deployed_model/ {data} {compartment_id} {project_id}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

custom_metadata = ModelCustomMetadata._from_oci_metadata(response.data.custom_metadata_list)
conda_slug, conda_path = None, None
if "CondaEnvironmentPath" in custom_metadata:
conda_path = custom_metadata['CondaEnvironmentPath'].value
Copy link
Member

Choose a reason for hiding this comment

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

NIT: MetadataCustomKeys.CONDA_ENVIRONMENT_PATH ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed as "CondaEnvironmentPath" in custom_metadata.keys

@@ -499,7 +502,26 @@ def deactivate(**kwargs):
suppress_traceback(kwargs["debug"])(deactivate_cmd)(**kwargs)


@commands.command()
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this file needs to be formatted? Please check the other files as well. I have added a pre-commit config in this PR, but till it will be merged the manual formatting needs to be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

run black manually

ads/opctl/cli.py Outdated
@click.option("--region", nargs=1, required=False, help="The destination Object Storage bucket region. By default the value will be extracted from the `OCI_REGION_METADATA` environment variables. This is only used when the model id is passed.")
@click.option("--timeout", nargs=1, required=False, help="The connection timeout in seconds for the client. This is only used when the model id is passed.")
@click.option("--artifact-directory", nargs=1, required=False, default=None, help="The artifact directory where stores your models, score.py and etc. This is used when you have a model artifact locally and have not saved it to the model catalog yet. In this case, you dont need to pass in model ")
@click.option("--payload", nargs=1, help="The payload sent to the model for prediction.")
Copy link
Member

Choose a reason for hiding this comment

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

Should be added some details about the format of the payload?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done



class DataScienceResource(str, metaclass=ExtendedEnumMeta):
JOB = "datasciencejob"
DATAFLOW = "dataflowapplication"
PIPELINE = "datasciencepipeline"
MODEL_DEPLOYMENT = "datasciencemodeldeployment"
MODEL = "datascience"
Copy link
Member

Choose a reason for hiding this comment

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

maybe it would be better to use datasciencemodel instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@github-actions
Copy link

📌 Cov diff with develop:

Coverage-0%

📌 Overall coverage:

No success to gather report. 😿

@github-actions
Copy link

github-actions bot commented May 1, 2023

No success to gather report. 😿

📌 Overall coverage:

No success to gather report. 😿

@z7ye z7ye marked this pull request as ready for review May 1, 2023 04:47
@github-actions
Copy link

github-actions bot commented May 1, 2023

No success to gather report. 😿

📌 Overall coverage:

No success to gather report. 😿

@z7ye z7ye requested a review from mrDzurb May 1, 2023 04:55
@github-actions
Copy link

github-actions bot commented May 1, 2023

📌 Cov diff with develop:

Coverage-0%

📌 Overall coverage:

No success to gather report. 😿

@github-actions
Copy link

github-actions bot commented May 1, 2023

📌 Cov diff with develop:

Coverage-78%

📌 Overall coverage:

Coverage-70.73%

@github-actions
Copy link

github-actions bot commented May 1, 2023

📌 Cov diff with develop:

Coverage-78%

📌 Overall coverage:

Coverage-70.72%

@z7ye z7ye requested review from qiuosier, lu-ohai and mayoor May 1, 2023 17:54
mrDzurb
mrDzurb previously approved these changes May 1, 2023
@github-actions
Copy link

github-actions bot commented May 2, 2023

📌 Cov diff with develop:

No success to gather report. 😿

📌 Overall coverage:

No success to gather report. 😿

@github-actions
Copy link

github-actions bot commented May 2, 2023

📌 Cov diff with develop:

Coverage-86%

📌 Overall coverage:

Coverage-70.79%

@z7ye z7ye requested a review from mrDzurb May 2, 2023 16:59
@github-actions
Copy link

github-actions bot commented May 2, 2023

📌 Cov diff with develop:

Coverage-86%

📌 Overall coverage:

Coverage-70.77%

@z7ye z7ye merged commit 61cbba2 into develop May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants