-
Notifications
You must be signed in to change notification settings - Fork 46
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
Corrected: Improved AQUA Error Messages for Authorization and Tag-Related Uses #1141
Conversation
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.
There's one test failing in test_model_handler.py
, can you please fix it?
ads/aqua/extension/utils.py
Outdated
|
||
if operation_name: | ||
if operation_name.startswith("create"): | ||
return f"{messages['create']} Operation Name: {operation_name}." |
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.
We need to check the status code before determining if this is an auth failure. If we give wrong input parameters, create can fail. Usually the wrong param happens because user have selected a shape which is not GA in the chosen region
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.
Addressed
ads/aqua/extension/utils.py
Outdated
return tip | ||
|
||
|
||
def construct_error(status_code, **kwargs) -> tuple[dict[str, int], str, str]: |
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.
status_code missing 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.
Addressed
ads/aqua/extension/utils.py
Outdated
return f"https://github.com/oracle-samples/oci-data-science-ai-samples/blob/main/ai-quick-actions/troubleshooting-tips.md#{github_header}" | ||
|
||
|
||
def get_troubleshooting_tips(service_payload: str, |
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.
payload is Dict?
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.
Addressed
ads/aqua/extension/utils.py
Outdated
"message": message, | ||
"service_payload": service_payload, | ||
"reason": reason, | ||
"request_id": str(uuid.uuid4()), |
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.
How does the request id help here?
@mrDzurb @VipulMascarenhas we should consider tracing our APIs so that we can use this request id for checking in lumberjack.
This is for later. For this PR we can leave this as is
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.
Not sure about if this request_id is used somewhere? Maybe we use it somehow on UI side?
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.
the request id is logged in lumberjack with the request id, and we print it in the local notebook logs for errors or when CLI is used - but request id is not shown in the UI yet. We'll need a UI change to show this alongside the error message.
ads/aqua/extension/utils.py
Outdated
"message": message, | ||
"service_payload": service_payload, | ||
"reason": reason, | ||
"request_id": str(uuid.uuid4()), |
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.
Not sure about if this request_id is used somewhere? Maybe we use it somehow on UI side?
ads/aqua/constants.py
Outdated
"put_object": "Unable to access or find Object Storage Bucket. See tips for troubleshooting: ", | ||
"list_model_version_sets": "Unable to create or fetch model version set. See tips for troubleshooting:", | ||
"update_model": "Unable to update model. See tips for troubleshooting: ", | ||
"list_data_science_private_endpoints": "Unable to access specified Object Storage Bucket. See tips for troubleshooting: ", |
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.
Is it a correct message for this key?
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.
Addressed
ads/aqua/constants.py
Outdated
"list_model_version_sets": "Unable to create or fetch model version set. See tips for troubleshooting:", | ||
"update_model": "Unable to update model. See tips for troubleshooting: ", | ||
"list_data_science_private_endpoints": "Unable to access specified Object Storage Bucket. See tips for troubleshooting: ", | ||
"create_model" : "Unable to register or create model. See tips for troubleshooting: ", |
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 think we should stick with “register” here, using “create” might imply that registering and creating are two separate operations, which could be confusing.
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.
Addressed
ads/aqua/constants.py
Outdated
"create_job_run": "Unable to create job run. See tips for troubleshooting: ", | ||
} | ||
|
||
ERROR_MESSAGES = { |
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.
NIT: the ERROR_MESSAGES
might be to generic name for this. How aboutL STATUS_CODE_MESSAGE
?
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.
Addressed
ads/aqua/extension/utils.py
Outdated
return tip | ||
|
||
|
||
def construct_error(status_code: int, **kwargs) -> tuple[dict[str, int], str, str]: |
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 think we can consider to use pydantic class instad returning a tuple?
Something like:
class ErrorResponse(Serializable):
"""Structured error response returned to the client."""
status: int = Field(..., description="HTTP status code.")
message: str = Field(..., description="Error message.")
request_id: str = Field(..., description="Unique ID for tracking the error.")
reason: Optional[str] = Field(None, description="Reason for the error.")
service_payload: Optional[Dict[str, Any]] = Field(default_factory=dict)
troubleshooting_tips: Optional[List[str]] = Field(default_factory=list)
And
def construct_error(status_code: int, **kwargs) -> ErrorResponse:
....
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.
Added ReplyDetails Pydantic class
- message (str, optional): A custom error message, from error raised from failed AQUA methods calling OCI SDK methods | ||
- exc_info (tuple, optional): Exception information (e.g., from `sys.exc_info()`), used for logging. | ||
|
||
Returns: |
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.
NIT: The docstring I guess needs to be changed
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.
Addressed
Currently, the AQUA UI throws generic errors when auth issues arise- not informing the user of potential solutions and suggested policy additions to fix the error.
Added 'troubleshooting_tips' section (see example json below)
result : user will obtain a specific action to fix their auth issue
Distinguish between Auth error versus tag related error.
result: user obtains a different fix for a tag-related error vs. an auth error
Refactored write_error in used in base_handler.py and WS handlers to utils.py (basically common code between these handlers were stuck into a single file)
Passed all test cases