Skip to content
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

Option to enable chat streaming #1516

Merged

Conversation

TamiTakamiya
Copy link
Contributor

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

Description

Add an option to enable chat streaming to our AI Connect service Django application. The new option is

CHATBOT_STREAM

and its default value is set to False. Unless CHATBOT_STREAM is set to True, the chat streaming support code will
not be enabled.

Testing

Steps to test

  1. Pull down the PR
  2. Run unit tests

Scenarios tested

Unit tests are provided with this PR.

Production deployment

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

Copy link
Contributor

@manstis manstis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2c about how new configuration options should be handled...

ansible_ai_connect/main/settings/base.py Outdated Show resolved Hide resolved
ansible_ai_connect/main/views.py Outdated Show resolved Hide resolved
@TamiTakamiya TamiTakamiya force-pushed the TamiTakamiya/AAP-39043/option-to-enable-chat-streaming branch from 3cfc3c8 to aadf74e Compare January 30, 2025 19:42
@TamiTakamiya TamiTakamiya marked this pull request as ready for review January 30, 2025 20:15
@manstis
Copy link
Contributor

manstis commented Jan 30, 2025

@TamiTakamiya I'll look again tomorrow.

If it's urgent perhaps ask someone else to look.

@TamiTakamiya
Copy link
Contributor Author

@manstis I have pushed the changes based on your comments. A new environment variable is no longer included. Please take a look. Thank you.

Notes:

  1. I have added the stream parameter to the HttpConfiguration class considering the streaming support might be added to other pipelines.
  2. I named the parameter as stream instead of streaming simply because OpenAI uses stream on their API.

@TamiTakamiya
Copy link
Contributor Author

@manstis It's not urgent. Pls take time :-)

@@ -140,6 +140,11 @@ def get_context_data(self, **kwargs):
context["user_name"] = user.username
context["debug"] = "true" if settings.CHATBOT_DEBUG_UI else "false"

llm: ModelPipelineChatBot = apps.get_app_config("ai").get_model_pipeline(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hint for optimisation (not that what you have written is slow/expensive to call).

You could have got the instance in a __init__ function...

... and used this in L#146 and L#126-L#130.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manstis I have update the code based on your comment. Pls re-review this when you have time. Thanks!

Copy link
Contributor

@manstis manstis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TamiTakamiya LGTM 👍 but I am confused as to whether stream is a static configuration parameter or a dynamic runtime variable.. Please see my comments.

ansible_ai_connect/main/tests/test_views.py Show resolved Hide resolved
manstis
manstis previously approved these changes Jan 30, 2025
Copy link
Contributor

@manstis manstis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank-you for the explanation @TamiTakamiya

LGTM 👍

Copy link
Contributor

@manstis manstis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Thank-you.

@TamiTakamiya TamiTakamiya merged commit 0a0d99d into main Feb 2, 2025
11 checks passed
@TamiTakamiya TamiTakamiya deleted the TamiTakamiya/AAP-39043/option-to-enable-chat-streaming branch February 2, 2025 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants