Removing kernal messaging in aqua #1304

agrimk · 2025-11-18T11:10:52Z

No description provided.

…o removing_kernal_messaging_in_aqua

github-actions · 2025-11-18T12:18:28Z

📌 Cov diff with main:

📌 Overall coverage:

github-actions · 2025-11-19T05:51:53Z

📌 Cov diff with main:

📌 Overall coverage:

…/accelerated-data-science into removing_kernal_messaging_in_aqua

github-actions · 2025-11-19T09:06:12Z

📌 Cov diff with main:

📌 Overall coverage:

github-actions · 2025-11-21T08:26:28Z

📌 Cov diff with main:

📌 Overall coverage:

VipulMascarenhas

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

VipulMascarenhas · 2025-11-24T20:37:48Z

ads/aqua/extension/deployment_handler.py

+        if payload.get("stop") == []:
+            payload["stop"] = None
+
+        encoded_image = "NA"


why is the key named NA here? shouldn't we check if "encoded_image" is present in the payload or not?

VipulMascarenhas · 2025-11-24T20:39:30Z

ads/aqua/extension/deployment_handler.py

+                                    {"type": "text", "text": payload["prompt"]},
+                                    {
+                                        "type": "image_url",
+                                        "image_url": {"url": f"{self.encoded_image}"},


this looks incorrect - self does not have encoded_image attribute

VipulMascarenhas · 2025-11-24T20:39:55Z

ads/aqua/extension/deployment_handler.py

+
+                    response = aqua_client.chat.completions.create(**api_kwargs)
+
+                elif self.file_type.startswith("audio"):


self does not have file_type attribute, shouldn't this be just file_type?

VipulMascarenhas · 2025-11-24T20:40:40Z

ads/aqua/extension/deployment_handler.py

+                                    {"type": "text", "text": payload["prompt"]},
+                                    {
+                                        "type": "audio_url",
+                                        "audio_url": {"url": f"{self.encoded_image}"},


this looks incorrect - should encoded_image be passed when file type is audio?

VipulMascarenhas · 2025-11-24T20:40:59Z

ads/aqua/extension/deployment_handler.py

+                    for chunk in response:
+                        piece = self._extract_text_from_chunk(chunk)
+                        if piece:
+                            print(piece, end="", flush=True)


shouldn't this yield instead of printing the chunk?

VipulMascarenhas · 2025-11-24T20:50:56Z

ads/aqua/extension/deployment_handler.py

+                            yield piece 
            except ExtendedRequestError as ex:
                raise HTTPError(400, str(ex))
            except Exception as ex:


handle unknown endpoint type with something like:

else: raise HTTPError(400, f"Unsupported endpoint_type: {endpoint_type}")

VipulMascarenhas · 2025-11-24T20:55:36Z

ads/aqua/extension/deployment_handler.py

            error_message = (
                f"Error fetching data from endpoint '{endpoint}' [{error_type}]: {ex}"
            )
            logger.error(


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.

ads/aqua/extension/deployment_handler.py

VipulMascarenhas · 2025-11-24T20:58:55Z

ads/aqua/extension/deployment_handler.py


 class AquaDeploymentStreamingInferenceHandler(AquaAPIhandler):
+
+    def _extract_text_from_choice(self, choice):


nit: add docstrings, use type hinting

ads/aqua/extension/deployment_handler.py

…o removing_kernal_messaging_in_aqua

github-actions · 2025-12-05T08:00:19Z

📌 Cov diff with main:

📌 Overall coverage:

github-actions · 2025-12-08T12:02:11Z

📌 Cov diff with main:

📌 Overall coverage:

VipulMascarenhas

left a few comments. there are a few places where we pop the keys from the payload - mutating the incoming dicts can cause issues while testing or debugging.

can you also add multimodal unit tests once the functionality has been tested?

VipulMascarenhas · 2025-12-11T05:00:29Z

ads/aqua/extension/deployment_handler.py

+                    and encoded_image != "NA"
+            ):
+                file_type = payload.pop("file_type")
+                if file_type.startswith("image"):


we can be a bit defensive here - if file_type value is None, it should not fail.

file_type = payload.get("file_type") if isinstance(file_type, str) and file_type.startswith("image"):

VipulMascarenhas · 2025-12-11T05:01:50Z

ads/aqua/extension/deployment_handler.py

+                                    {"type": "text", "text": payload["prompt"]},
+                                    {
+                                        "type": "audio_url",
+                                        "audio_url": {"url": f"{encoded_image}"},


in case of audio file type, does the content comes from payload["encoded_image"] ? can we use another key for this instead of encoded_image?

also it looks like audio is currently not supported by /v1/response api. Instead of adding this code, we can return "unsupported file type" exception.
https://platform.openai.com/docs/guides/audio?example=audio-out#add-audio-to-your-existing-application

VipulMascarenhas · 2025-12-11T05:08:23Z

ads/aqua/extension/deployment_handler.py

+                            {
+                                "role": "user",
+                                "content": [
+                                    {"type": "text", "text": payload["prompt"]},


I think here type should be "input_text" and "input_image".
https://platform.openai.com/docs/guides/images-vision#giving-a-model-images-as-input

github-actions · 2025-12-11T11:03:25Z

📌 Cov diff with main:

📌 Overall coverage:

agrimk added 4 commits October 13, 2025 16:16

deployment inference using openAI client

23a87a9

Merge branch 'main' of github.com:oracle/accelerated-data-science int…

e7c18d3

…o removing_kernal_messaging_in_aqua

Merge branch 'main' of github.com:oracle/accelerated-data-science int…

ab7e984

…o removing_kernal_messaging_in_aqua

stream inference endpoint

a534b62

agrimk requested review from VipulMascarenhas, ahosler, darenr, mayoor, mrDzurb and qiuosier as code owners November 18, 2025 11:10

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Nov 18, 2025

Merge branch 'main' into removing_kernal_messaging_in_aqua

30f8e47

agrimk added 2 commits November 19, 2025 13:27

unit test fixes

f45350a

Merge branch 'removing_kernal_messaging_in_aqua' of github.com:oracle…

6e46a8c

…/accelerated-data-science into removing_kernal_messaging_in_aqua

mrDzurb previously approved these changes Nov 24, 2025

View reviewed changes

VipulMascarenhas requested changes Nov 24, 2025

View reviewed changes

agrimk added 2 commits December 4, 2025 15:59

Merge branch 'main' of github.com:oracle/accelerated-data-science int…

3c4895b

…o removing_kernal_messaging_in_aqua

added test cases and PR review comments

df27ccf

agrimk dismissed mrDzurb’s stale review via df27ccf December 5, 2025 06:49

fixing test cases

39cc70c

VipulMascarenhas reviewed Dec 11, 2025

View reviewed changes

fixed handling of encoded_image

ead77e7


		response = aqua_client.chat.completions.create(**api_kwargs)

		elif self.file_type.startswith("audio"):


		class AquaDeploymentStreamingInferenceHandler(AquaAPIhandler):

		def _extract_text_from_choice(self, choice):

Removing kernal messaging in aqua #1304

Are you sure you want to change the base?

Removing kernal messaging in aqua #1304

Uh oh!

Conversation

agrimk commented Nov 18, 2025

Uh oh!

github-actions bot commented Nov 18, 2025

Uh oh!

github-actions bot commented Nov 19, 2025

Uh oh!

github-actions bot commented Nov 19, 2025

Uh oh!

github-actions bot commented Nov 21, 2025

Uh oh!

VipulMascarenhas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VipulMascarenhas Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Dec 5, 2025

Uh oh!

github-actions bot commented Dec 8, 2025

Uh oh!

VipulMascarenhas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

VipulMascarenhas Nov 24, 2025 •

edited

Loading

VipulMascarenhas left a comment •

edited

Loading