[AQUA Telemetry] Update MD Tracking #1193
Conversation
agrimk
requested review from
darenr,
mayoor,
mrDzurb,
VipulMascarenhas,
qiuosier and
ahosler
as code owners
oracle-contributor-agreement
bot
added
the
OCA Verified
|
📌 Cov diff with main: 📌 Overall coverage: |
|
📌 Cov diff with main: 📌 Overall coverage: |
| deployment_id = deployment.id | ||
|
|
||
|
|
||
| deployment_id = deployment.id() |
There was a problem hiding this comment.
id is a property, keep it as .id instead else it will result in TypeError
ads/common/work_request.py
Outdated
| @@ -38,6 +38,7 @@ def __init__( | |||
| config: dict = None, | |||
| signer: Signer = None, | |||
| client_kwargs: dict = None, | |||
| _error_message: str = None, | |||
There was a problem hiding this comment.
why do we need a parameter called _error_message here? We can have it inside init directly:
self._error_message = None
|
|
||
|
|
||
| def get_deployment_status(self,model_deployment_id: str, work_request_id : str, model_type : str) : |
ads/aqua/constants.py
Outdated
| @@ -54,6 +54,8 @@ | |||
| SERVICE_MANAGED_CONTAINER_URI_SCHEME = "dsmc://" | |||
| SUPPORTED_FILE_FORMATS = ["jsonl"] | |||
| MODEL_BY_REFERENCE_OSS_PATH_KEY = "artifact_location" | |||
| DEFAULT_WAIT_TIME = 1200 | |||
There was a problem hiding this comment.
since these constants are specific to model deployment status, we could have it in the ads.aqua.modeldeployment.constants.py.
| @@ -80,6 +84,9 @@ | |||
| from ads.model.model_metadata import ModelCustomMetadataItem | |||
| from ads.telemetry import telemetry | |||
|
|
|||
| THREAD_POOL_SIZE = 16 | |||
There was a problem hiding this comment.
we can move this constant in ads.aqua.modeldeployment.constants.py
| @@ -80,6 +84,9 @@ | |||
| from ads.model.model_metadata import ModelCustomMetadataItem | |||
| from ads.telemetry import telemetry | |||
|
|
|||
| THREAD_POOL_SIZE = 16 | |||
| thread_pool = concurrent.futures.ThreadPoolExecutor(max_workers=THREAD_POOL_SIZE) | |||
There was a problem hiding this comment.
I think we should use the common telemetry or the common decorator threadpool instead of creating one here.
| category=f"aqua/{model_type}/deployment/status", | ||
| action="FAILED", | ||
| detail="Error creating model deployment" |
There was a problem hiding this comment.
we should log the error message (_error_message) coming from work request here instead of a static message. This will be used to track the specific reasons why the deployment failed.
| ) | ||
|
|
||
| self.telemetry.record_event_async( |
There was a problem hiding this comment.
shouldn't this be within else block? We can use try-except-else here.
| category=f"aqua/{model_type}/deployment/status", | ||
| action="SUCCEEDED", | ||
| detail=" Create model deployment successful", |
There was a problem hiding this comment.
we can skip this detail, action "SUCCEEDED" implies the same thing.
…elerated-data-science into ODSC-70841_update_md_tracking
…o ODSC-70841_update_md_tracking
| self.telemetry.record_event_async( | ||
| category=f"aqua/{model_type}/deployment/status", | ||
| action="FAILED", | ||
| detail=data_science_work_request._error_message |
There was a problem hiding this comment.
can _error_message be None for any reason? Might be good to do detail=data_science_work_request._error_message or UNKNOWN to avoid unforeseen issues in telemetry logging.
| @@ -78,6 +79,7 @@ def _sync(self): | |||
| self._percentage= work_request.percent_complete | |||
| self._status = work_request.status | |||
| self._description = work_request_logs[-1].message if work_request_logs else "Processing" | |||
| if work_request.status == 'FAILED' : self._error_message = self.client.list_work_request_errors | |||
There was a problem hiding this comment.
might be good to show an example output for failed and successful MD in the PR description.
mrDzurb
changed the title
https://jira.oci.oraclecorp.com/browse/ODSC-70841