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

[APP-3696] Fix streaming value and improve error messages #93

Merged
merged 3 commits into from
Jan 17, 2025

Conversation

kideh88
Copy link
Collaborator

@kideh88 kideh88 commented Jan 15, 2025

This repository is public. Do not put any private DataRobot or customer data: code, datasets, model artifacts, .etc.

Rationale

Streaming should not be enabled by default as we currently only support GPT blueprints for it.
Furthermore the stream value should not be set at all - not even as False/None

Error handling has been improved to make it more clear when the error happened during a request or within the app.
Requests will show url and code/reason/message while processing issues show Error while processing response from Chat API: ..<error message>

Summary of Changes

  • Fix stream value by not sending a value at all for False/None
  • Improve error messages (server/endpoint vs app error)
  • Improve docs with streaming limitations
  • Disable streaming as default setting
  • Fix loading message for non-streaming chat api requests
  • Re-enable Chat API default to true
  • Add new (complete) mocks for tests
  • Bump the template version

Screenshots

Improved messages:
image

image

image

@kideh88 kideh88 requested a review from a team as a code owner January 15, 2025 12:31
Copy link
Contributor

@demchukilya demchukilya left a comment

Choose a reason for hiding this comment

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

send_chat_api_request is a bit messy but in general fine.

@@ -10,7 +10,7 @@
I18N_SPLASH_TITLE, I18N_SPLASH_TEXT, I18N_LOADING_MESSAGE, I18N_ACCESSIBILITY_LABEL_LLM,
ROLE_USER, I18N_ACCESSIBILITY_LABEL_YOU, I18N_NO_DEPLOYMENT_FOUND,
I18N_NO_DEPLOYMENT_ID, LLM_AVATAR, LLM_DISPLAY_NAME, USER_AVATAR, USER_DISPLAY_NAME,
ROLE_ASSISTANT, STATUS_ERROR)
ROLE_ASSISTANT, STATUS_ERROR, ENABLE_CHAT_API_STREAMING)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it's better to have each const on separate line.
Or just import constants and use constants.FOO

@kideh88
Copy link
Collaborator Author

kideh88 commented Jan 16, 2025

send_chat_api_request is a bit messy but in general fine.

@demchukilya Thinking the same , what do you think of splitting into send_chat_api_request and separate send_chat_api_streaming_request ?

@kideh88 kideh88 merged commit 1c6e7bc into main Jan 17, 2025
3 checks passed
@kideh88 kideh88 deleted the kimd/APP-3696-fixstream branch January 17, 2025 07:53
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.

2 participants