[DRAFT] /streaming_chat endpoint PoC #1527

TamiTakamiya · 2025-02-10T21:11:22Z

Jira Issue: https://issues.redhat.com/browse/AAP-39044

Description

A PoC for /streaming_chat endpoint

Testing

Steps to test

Pull down the PR
...
...

Scenarios tested

Production deployment

This code change is ready for production on its own
This code change requires the following considerations before going to production:

manstis

I had a peek @TamiTakamiya

I don't think we need the async_invoke function as you've now moved streaming support to a new type of pipeline (ModelPipelineStreamingChatBot).

manstis · 2025-02-19T09:37:19Z

ansible_ai_connect/main/settings/legacy.py

+            "inference_url": chatbot_service_url or "http://localhost:8000",
+            "model_id": chatbot_service_model_id or "granite3-8b",
+            "verify_ssl": model_service_verify_ssl,
+            "stream": False,


IMO use of environment variables is deprecated.

ANSIBLE_AI_MODEL_MESH_CONFIG should be considered the master configuration.

I'd therefore propose:

Add a new line after L#182: chatbot_streaming = os.getenv("CHATBOT_STREAMING")

Change L#201 to "stream": chatbot_streaming

Revert this change.

Thanks for the comment.

With this streaming support, AI Connect Service will have two chat endpoints: /chat and /streaming_chat and natually ANSIBLE_AI_MODEL_MESH_CONFIG is going to have two pipeline definitions: ModelPipelineChatBot and ModelPipelineStreamingChatBot.

I started thinking that the stream property in config of ModelPipelineChatBot should always be false and the stream property in config of ModelPipelineStreamingChatBot should always be true because that's how each pipeline is written.

The CHATBOT_STREAMING envvar sets the stream hidden form field that is used for letting Chatbot UI know which endpoint (streaming/non-streaming) to be called. That's my intention.

Realistically, when ANSIBLE_AI_MODEL_MESH_CONFIG contains ModelPipelineStreamingChatBot, I think we can assume the streaming mode is used and we can eliminate that envvar. I will implement in that way and when we see a requirement to override the default, add an envvar or some other option.

manstis · 2025-02-19T09:54:53Z

ansible_ai_connect/ai/api/model_pipelines/pipelines.py

+        )
+
+
+StreamingChatBotResponse = Any


IMO this should be StreamingChatBotResponse = AsyncGenerator

manstis · 2025-02-19T09:55:45Z

ansible_ai_connect/ai/api/model_pipelines/pipelines.py

@@ -274,6 +301,9 @@ def alias() -> str:
    def invoke(self, params: PIPELINE_PARAMETERS) -> PIPELINE_RETURN:
        raise NotImplementedError

+    def async_invoke(self, params: PIPELINE_PARAMETERS) -> AsyncGenerator:


This method isn't needed (yes, yes, I know I proposed it.. but I found a better way.. I think)

I will update my code today :-) Thanks.

manstis · 2025-02-19T09:58:18Z

ansible_ai_connect/ai/api/model_pipelines/http/pipelines.py

+    def __init__(self, config: HttpConfiguration):
+        super().__init__(config=config)
+
+    def invoke(self, params: StreamingChatBotParameters) -> StreamingHttpResponse:


Change L#222-L#225 to simply be:

async def invoke(self, params: StreamingChatBotParameters) -> StreamingChatBotResponse:

i.e. there is no need for an invoke AND async_invoke function; just an invoke function.

@TamiTakamiya it looks like you misunderstood some of my proposals in this latest commit.

Happy to go through with you tomorrow.

manstis · 2025-02-19T09:59:30Z

ansible_ai_connect/ai/api/views.py

+        },
+        summary="Streaming chat request",
+    )
+    def post(self, request) -> Response:


Can the return type be StreamingHttpResponse?

manstis · 2025-02-19T09:59:55Z

ansible_ai_connect/ai/api/views.py

+        self.event.modelName = self.req_model_id or self.llm.config.model_id
+
+        return StreamingHttpResponse(
+            self.llm.async_invoke(


If my other changes are integrated; this can simply self.llm.invoke(....)

manstis · 2025-02-19T10:01:51Z

ansible_ai_connect/healthcheck/tests/test_healthcheck.py

@@ -159,7 +159,7 @@ def assert_basic_data(
        self.assert_common_data(data, expected_status, deployed_region)
        timestamp = data["timestamp"]
        dependencies = data.get("dependencies", [])
-        self.assertEqual(10, len(dependencies))
+        self.assertEqual(11, len(dependencies))


JFYI, there's also a test in ansible-wisdom-testing that will break.

Thanks for the information.

I thought I should introduce a constant for this instead of replacing 10 with 11 (long time ago I was told immediate values other than 0 should not appear in source codes), but I took the easy way :-)

sonarqubecloud · 2025-02-21T03:21:47Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
83.3% Coverage on New Code
2.1% Duplication on New Code

See analysis details on SonarQube Cloud

TamiTakamiya changed the base branch from main to TamiTakamiya/AAP-39043/chatbot-streaming-ui February 10, 2025 21:12

Base automatically changed from TamiTakamiya/AAP-39043/chatbot-streaming-ui to main February 10, 2025 21:21

TamiTakamiya force-pushed the TamiTakamiya/AAP-39044/streaming-chat-endpoint-poc branch 6 times, most recently from dae29f3 to 91c8daa Compare February 14, 2025 21:35

TamiTakamiya force-pushed the TamiTakamiya/AAP-39044/streaming-chat-endpoint-poc branch from e833b89 to d251b56 Compare February 18, 2025 18:28

TamiTakamiya added 2 commits February 18, 2025 13:54

Chat Streaming endpoint

0113adf

Add CHATBOT_STREAMING envvar

7e950a1

TamiTakamiya force-pushed the TamiTakamiya/AAP-39044/streaming-chat-endpoint-poc branch 2 times, most recently from 2d9bdbf to eed802b Compare February 18, 2025 20:06

manstis reviewed Feb 19, 2025

View reviewed changes

TamiTakamiya added 3 commits February 19, 2025 16:20

Fix CI error

5f058ff

Redesign model pipeline

61f025e

Remove CHATBOT_STREAMING envvar

da5c188

TamiTakamiya force-pushed the TamiTakamiya/AAP-39044/streaming-chat-endpoint-poc branch from c661a8d to da5c188 Compare February 19, 2025 21:48

TamiTakamiya added 7 commits February 19, 2025 19:57

Test cases (initial commit)

512e1df

test_chat_not_enabled_exception

b382d27

update api schema and more test changes

b7c7cba

Pipeline changes

d8cc3a9

Pipeline changes

512b15a

Test case update

df33e26

Test case update

4b1519c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] /streaming_chat endpoint PoC #1527

[DRAFT] /streaming_chat endpoint PoC #1527

TamiTakamiya commented Feb 10, 2025

manstis left a comment

manstis Feb 19, 2025

TamiTakamiya Feb 19, 2025

manstis Feb 19, 2025

manstis Feb 19, 2025

TamiTakamiya Feb 19, 2025

manstis Feb 19, 2025

manstis Feb 19, 2025

manstis Feb 19, 2025

manstis Feb 19, 2025

manstis Feb 19, 2025

TamiTakamiya Feb 19, 2025

sonarqubecloud bot commented Feb 21, 2025

		)


		StreamingChatBotResponse = Any

[DRAFT] /streaming_chat endpoint PoC #1527

Are you sure you want to change the base?

[DRAFT] /streaming_chat endpoint PoC #1527

Conversation

TamiTakamiya commented Feb 10, 2025

Description

Testing

Steps to test

Scenarios tested

Production deployment

manstis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 21, 2025

Quality Gate passed