Add ability to stream chat responses
What does this MR do and why?
This adds the capabilities to stream messages for GitLab Duo chat.
The streamed chunks get send over aiCompletionResponse
but the order
is not guaranteed on the client. Each chunk has a chunkId
which has a
guaranteed order, starting with 1
.
The client_subscription_id
is meant for the client to identify which
mutation belongs to which subscription response. However, chat is a
special feature where we want to sync responses between multiple chat
instances of the same user.
This means we want to have two different subscriptions for the same request: 1. User-specific to sync between multiple clients 2. Request-specific for streaming
By not streaming the response when a client_subscription_id
is not
present, we also make sure that older clients still work when the
stream_gitlab_duo
is enabled. Otherwise, they would receive a streamed
response on a subscription on user_id
and resource_id
.
NOTE: Related FE MR by @dmishunov !130347 (merged)
Screenshots or screen recordings
The following only works with the FE and graphql changes. There is an MR that combines both: !130415 (closed)
Without FF | With FF |
---|---|
streaming-example-no-ff | streaming-example |
Before | After |
---|---|
How to set up and validate locally
- Have GitLab Duo and all AI features set up
- Enable the FF
stream_gitlab_duo
- Checkout !130415 (closed) as it combines this MR with the FE changes and updates the GraphQL subscriptions
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
added groupai framework label
assigned to @nicolasdular
added devopsai-powered sectiondata-science labels
changed milestone to %16.4
- A deleted user
added backend documentation frontend labels
1 Warning This merge request is quite big (549 lines changed), please consider splitting it into multiple merge requests. 1 Message This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. Documentation review
The following files require a review from a technical writer:
-
doc/api/graphql/reference/index.md
(Link to current live version)
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Smriti Garg (
@sgarg_gitlab
) (UTC+5.5, 3.5 hours ahead of@nicolasdular
)Bojan Marjanović (
@bmarjanovic
) (UTC+2, same timezone as@nicolasdular
)Please check reviewer's status!
Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger-
- Resolved by 🤖 GitLab Bot 🤖
Proper labels assigned to this merge request. Please ignore me.
@nicolasdular - please see the following guidance and update this merge request.1 Error Please add typebug typefeature, or typemaintenance label to this merge request. Edited by 🤖 GitLab Bot 🤖
Bundle size analysis [beta]
This compares changes in bundle size for entry points between the commits 7a38200a and 581123ce
Special assetsEntrypoint / Name Size before Size after Diff Diff in percent average 4.17 MB 4.18 MB +3.23 KB 0.1 % mainChunk 3.02 MB 3.02 MB +3.23 KB 0.1 %
Note: We do not have exact data for 7a38200a. So we have used data from: a1a84dad.
The intended commit has no webpack pipeline, so we chose the last commit with one before it.Please look at the full report for more details
Read more about how this report works.
Generated by
DangerAllure report
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for c6875651expand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Plan | 51 | 0 | 0 | 0 | 51 | ✅ | | Create | 46 | 0 | 1 | 2 | 47 | ❗ | | Monitor | 4 | 0 | 0 | 0 | 4 | ✅ | | Verify | 8 | 0 | 0 | 0 | 8 | ✅ | | Govern | 36 | 0 | 0 | 0 | 36 | ✅ | | Manage | 13 | 0 | 1 | 0 | 14 | ✅ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | | Framework sanity | 0 | 0 | 1 | 0 | 1 | ➖ | | Data Stores | 22 | 0 | 0 | 0 | 22 | ✅ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 180 | 0 | 4 | 2 | 184 | ❗ | +------------------+--------+--------+---------+-------+-------+--------+
e2e-review-qa:
test report for 581123ceexpand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Create | 8 | 0 | 1 | 0 | 9 | ✅ | | Framework sanity | 0 | 0 | 1 | 0 | 1 | ➖ | | Manage | 1 | 0 | 0 | 0 | 1 | ✅ | | Data Stores | 2 | 0 | 0 | 0 | 2 | ✅ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | | Monitor | 4 | 0 | 0 | 0 | 4 | ✅ | | Plan | 3 | 0 | 1 | 0 | 4 | ✅ | | Govern | 2 | 0 | 0 | 0 | 2 | ✅ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 20 | 0 | 4 | 0 | 24 | ✅ | +------------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for c6875651expand test summary
+------------------------------------------------------------+ | suites summary | +-------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------+--------+--------+---------+-------+-------+--------+ +-------+--------+--------+---------+-------+-------+--------+ | Total | 0 | 0 | 0 | 0 | 0 | ➖ | +-------+--------+--------+---------+-------+-------+--------+
mentioned in merge request !130093 (merged)
- Resolved by Nicolas Dular
- Resolved by Nicolas Dular
requested review from @katiemacoy
requested review from @euko
- Resolved by Gosia Ksionek
@nicolasdular I can check out this MR tomorrow morning and sync with you later to ask any question.
added 649 commits
-
775a01b7...01254e6e - 645 commits from branch
master
- 2d15d07f - WIP
- 52da2799 - Fix specs
- c8e83166 - Add chunk_id
- d7a16228 - Add yet another case for receiving chat messages
Toggle commit list-
775a01b7...01254e6e - 645 commits from branch
- A deleted user
added feature flag label
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@3236aa87
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@e5af1656
- Resolved by Nicolas Dular
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@ce02244d
- Resolved by Nicolas Dular
- Resolved by Nicolas Dular
NOTE: When testing this, you will notice that the streamed response is not served in the right order.
This is due to the limitation of ActionCable.
This blog mentions the problem and the solution they're using: https://hayford.dev/implementing-order-with-actioncable-signalling-webrtc/
Rails’ ActionCable is excellent for building realtime features in applications. However, in some realtime communication applications, ActionCable is just not good enough. Especially in a stack where order matters.
The article does not describe why the order is not observed.
I peeked into ActionCable and I think this seems to be roughly what happens:
- ActionCable utilizes a thread pool (Concurrent::ThreadPoolExecutor https://ruby-concurrency.github.io/concurrent-ruby/master/Concurrent/ThreadPoolExecutor.html)
- Whenever there's a broadcasting, it's published to Redis which is the adapter we use.
- The subscriber callbacks are placed in the timer-based event loop (https://ruby-concurrency.github.io/concurrent-ruby/1.1.5/Concurrent/TimerTask.html)
Obviously we can publish messages serially but they are grabbed by different members from the threadpool.
@engwan can you check the accuracy of what I described please if you have time?
So what now?
- If the above characteristic of ActionCable is not already known widely, it's something worth being aware of equally for FEs cc. @dmishunov
- Ordering the messages in the frontend is the least frictionless route for now I think.
- Resolved by euko
- Resolved by Nicolas Dular
- Resolved by Nicolas Dular
- Resolved by Nicolas Dular
(non-blocking):
I have a question on the buffering logic in Anthropic::Client so it can be addressed as a follow up since the code wasn't added in this MR.
We have a read timeout (Gitlab::HTTP) but I wonder if we should limit the amount of the response we buffer
response_body
since we don't control the response from Anthropic to be safe? The limit can be a reasonable amount.# https://gitlab.com/gitlab-org/gitlab/-/blob/01254e6e625b09887f5372aaab1be316d88b4d1d/ee/lib/gitlab/llm/anthropic/client.rb#L34 perform_completion_request(prompt: prompt, options: options.merge(stream: true)) do |parsed_event| response_body += parsed_event["completion"] if parsed_event["completion"] yield parsed_event if block_given? end
Perhaps someone more expert in Ruby perf. can chime in later.
Edited by euko
added 175 commits
-
581123ce...a6f3ff0b - 163 commits from branch
master
- a6f3ff0b...11afcd97 - 2 earlier commits
- 6118ff92 - Add chunk_id
- 7b315a78 - Add yet another case for receiving chat messages
- 4e0e0581 - Remove unnecessary subscription
- a53f60fc - Refactor and add tests
- c3df1064 - Add more tests
- e4ab0212 - Update ID to start with 1
- 0fc91794 - Add MR to Feature Flag
- a44b6eac - Log errors from streaming
- ae2c15b8 - Rename stream method
- e61ac591 - Reset FE changes
Toggle commit list-
581123ce...a6f3ff0b - 163 commits from branch
added featureenhancement typefeature labels
mentioned in issue #423519
mentioned in issue #423520
removed review request for @euko
@euko
, thanks for approving this merge request.This is the first time the merge request has been approved. To ensure full test coverage, a new pipeline will be started shortly.
For more info, please refer to the following links:
added pipeline:mr-approved label
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@f05ea8cd
added 2 commits
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@6687ee31
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@049ea720
mentioned in merge request !130415 (closed)
requested review from @mksionek
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@a5daf1bf
mentioned in merge request !130347 (merged)
mentioned in merge request !130337 (merged)
removed review request for @katiemacoy
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@da7d6aaa
mentioned in merge request !130444 (merged)
- Resolved by Nicolas Dular
- Resolved by Gosia Ksionek
- Resolved by Nicolas Dular
- Resolved by Gosia Ksionek
I have small nitpick only. Other than that I am in awe!! Great job @nicolasdular!!!
mentioned in issue #423723
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@59e0e16b
enabled an automatic merge when the pipeline for 59e0e16b succeeds
mentioned in commit f1810bae
mentioned in issue #423756 (closed)
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowpost-deploy-db-staging label and removed workflowproduction label
added workflowpost-deploy-db-production label and removed workflowpost-deploy-db-staging label
added releasedcandidate label