Removing kernal messaging in aqua #1304
Conversation
…o removing_kernal_messaging_in_aqua
…o removing_kernal_messaging_in_aqua
agrimk
requested review from
VipulMascarenhas,
ahosler,
darenr,
mayoor,
mrDzurb and
qiuosier
as code owners
oracle-contributor-agreement
bot
added
the
OCA Verified
|
📌 Cov diff with main: 📌 Overall coverage: |
|
📌 Cov diff with main: 📌 Overall coverage: |
…/accelerated-data-science into removing_kernal_messaging_in_aqua
|
📌 Cov diff with main: 📌 Overall coverage: |
1 similar comment
|
📌 Cov diff with main: 📌 Overall coverage: |
VipulMascarenhas
left a comment
VipulMascarenhas
left a comment
There was a problem hiding this comment.
the PR needs some work, also please add tests to validate the new helper methods added, and the _get_model_deployment_response function to cover:
- chat completions with prompt only
- chat completions with messages
- multimodal with image, with audio
- text completions
- responses
- missing/invalid endpoint_type -> HTTP 400
| if payload.get("stop") == []: | ||
| payload["stop"] = None | ||
|
|
||
| encoded_image = "NA" |
There was a problem hiding this comment.
why is the key named NA here? shouldn't we check if "encoded_image" is present in the payload or not?
| {"type": "text", "text": payload["prompt"]}, | ||
| { | ||
| "type": "image_url", | ||
| "image_url": {"url": f"{self.encoded_image}"}, |
There was a problem hiding this comment.
this looks incorrect - self does not have encoded_image attribute
|
|
||
| response = aqua_client.chat.completions.create(**api_kwargs) | ||
|
|
||
| elif self.file_type.startswith("audio"): |
There was a problem hiding this comment.
self does not have file_type attribute, shouldn't this be just file_type?
| {"type": "text", "text": payload["prompt"]}, | ||
| { | ||
| "type": "audio_url", | ||
| "audio_url": {"url": f"{self.encoded_image}"}, |
There was a problem hiding this comment.
this looks incorrect - should encoded_image be passed when file type is audio?
| for chunk in response: | ||
| piece = self._extract_text_from_chunk(chunk) | ||
| if piece: | ||
| print(piece, end="", flush=True) |
There was a problem hiding this comment.
shouldn't this yield instead of printing the chunk?
| yield piece | ||
| except ExtendedRequestError as ex: | ||
| raise HTTPError(400, str(ex)) | ||
| except Exception as ex: |
There was a problem hiding this comment.
handle unknown endpoint type with something like:
else:
raise HTTPError(400, f"Unsupported endpoint_type: {endpoint_type}")
| error_message = ( | ||
| f"Error fetching data from endpoint '{endpoint}' [{error_type}]: {ex}" | ||
| ) | ||
| logger.error( |
There was a problem hiding this comment.
this is old code, but has a breaking change. Can we add from ads.aqua import logger import at the top of the file? Else, if this handler is not used at all then we can remove this.
| ) | ||
| api_kwargs = { | ||
| "model": model, | ||
| "messages": [{"role": "user", "content": payload["prompt"]}], |
There was a problem hiding this comment.
payload["prompt"] can error out if prompt key isn't present. We can do some key validation at the beginning of the function for all attributes fetched from payload.
|
|
||
| class AquaDeploymentStreamingInferenceHandler(AquaAPIhandler): | ||
|
|
||
| def _extract_text_from_choice(self, choice): |
There was a problem hiding this comment.
nit: add docstrings, use type hinting
| return getattr(choice, "text", None) or getattr(choice, "content", None) | ||
|
|
||
| def _extract_text_from_chunk(self, chunk): | ||
| if chunk : |
There was a problem hiding this comment.
nit: add docstrings, use type hinting
3 participants
No description provided.