Skip to content
Snippets Groups Projects

Add ability to stream chat responses

Merged Nicolas Dular requested to merge nd/streaming-connect-fe-be into master

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

  1. Have GitLab Duo and all AI features set up
  2. Enable the FF stream_gitlab_duo
  3. 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.

Edited by Nicolas Dular

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Nicolas Dular changed milestone to %16.4

    changed milestone to %16.4

  • A deleted user added backend documentation frontend labels
  • Contributor
    1 Warning
    :warning: This merge request is quite big (549 lines changed), please consider splitting it into multiple merge requests.
    1 Message
    :book: 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:

    The review does not need to block merging this merge request. See the:

    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 current availability (@sgarg_gitlab) (UTC+5.5, 3.5 hours ahead of @nicolasdular) Bojan Marjanović current availability (@bmarjanovic) (UTC+2, same timezone as @nicolasdular)

    Please check reviewer's status!

    • available Reviewer is available!
    • unavailable Reviewer is unavailable!

    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 :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • Contributor

    Bundle size analysis [beta]

    This compares changes in bundle size for entry points between the commits 7a38200a and 581123ce

    :sparkles: Special assets

    Entrypoint / 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 :no_entry_sign: Danger

  • Contributor

    Allure report

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :exclamation: test report for c6875651

    expand 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: :white_check_mark: test report for 581123ce

    expand 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: :heavy_minus_sign: test report for c6875651

    expand test summary
    +------------------------------------------------------------+
    |                       suites summary                       |
    +-------+--------+--------+---------+-------+-------+--------+
    |       | passed | failed | skipped | flaky | total | result |
    +-------+--------+--------+---------+-------+-------+--------+
    +-------+--------+--------+---------+-------+-------+--------+
    | Total | 0      | 0      | 0       | 0     | 0     | ➖     |
    +-------+--------+--------+---------+-------+-------+--------+
  • Nicolas Dular mentioned in merge request !130093 (merged)

    mentioned in merge request !130093 (merged)

  • Nicolas Dular added 4 commits

    added 4 commits

    Compare with previous version

  • Nicolas Dular changed the description

    changed the description

  • Katie Macoy requested review from @katiemacoy

    requested review from @katiemacoy

  • euko requested review from @euko

    requested review from @euko

  • Nicolas Dular added 649 commits

    added 649 commits

    Compare with previous version

  • Nicolas Dular added 1 commit

    added 1 commit

    • 5d9056d2 - Remove unnecessary subscription

    Compare with previous version

  • Nicolas Dular added 1 commit

    added 1 commit

    Compare with previous version

  • A deleted user added feature flag label

    added feature flag label

  • Nicolas Dular marked the checklist item Tests for the Executor as completed

    marked the checklist item Tests for the Executor as completed

  • Nicolas Dular added 1 commit

    added 1 commit

    Compare with previous version

  • Nicolas Dular changed the description

    changed the description

  • Nicolas Dular changed the description

    changed the description

  • Nicolas Dular
  • Nicolas Dular added 1 commit

    added 1 commit

    Compare with previous version

  • euko
  • euko
  • euko
  • euko
    • Contributor
      Resolved by Nicolas Dular

      @nicolasdular

      (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
  • Nicolas Dular added 175 commits

    added 175 commits

    Compare with previous version

  • Nicolas Dular mentioned in issue #423519

    mentioned in issue #423519

  • Nicolas Dular mentioned in issue #423520

    mentioned in issue #423520

  • euko approved this merge request

    approved this merge request

  • euko removed review request for @euko

    removed review request for @euko

  • :wave: @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:

  • Nicolas Dular added 2 commits

    added 2 commits

    • 2cc6a48d - Add streaming capabilities for chat
    • 6442eb9c - Only stream with a client_subscription_id

    Compare with previous version

  • Nicolas Dular marked this merge request as ready

    marked this merge request as ready

  • Nicolas Dular changed title from Draft: Add streaming to Add ability to stream chat responses

    changed title from Draft: Add streaming to Add ability to stream chat responses

  • Nicolas Dular changed the description

    changed the description

  • Nicolas Dular added 1 commit

    added 1 commit

    • 07195f09 - Update mutation and subscriptions

    Compare with previous version

  • Nicolas Dular changed the description

    changed the description

  • Nicolas Dular changed the description

    changed the description

  • Nicolas Dular mentioned in merge request !130415 (closed)

    mentioned in merge request !130415 (closed)

  • Nicolas Dular requested review from @mksionek

    requested review from @mksionek

  • Nicolas Dular mentioned in merge request !130347 (merged)

    mentioned in merge request !130347 (merged)

  • Nicolas Dular mentioned in merge request !130337 (merged)

    mentioned in merge request !130337 (merged)

  • Katie Macoy removed review request for @katiemacoy

    removed review request for @katiemacoy

  • Nicolas Dular added 1 commit

    added 1 commit

    Compare with previous version

  • Nicolas Dular mentioned in merge request !130444 (merged)

    mentioned in merge request !130444 (merged)

  • Gosia Ksionek
  • Gosia Ksionek
  • Sorry, this is not a full review, but since I have found some issues, I will finish tomorrow.

  • Gosia Ksionek
  • I have small nitpick only. Other than that I am in awe!! Great job @nicolasdular!!!

  • Gosia Ksionek approved this merge request

    approved this merge request

  • Nicolas Dular mentioned in issue #423723

    mentioned in issue #423723

  • Gosia Ksionek resolved all threads

    resolved all threads

  • Gosia Ksionek enabled an automatic merge when the pipeline for 59e0e16b succeeds

    enabled an automatic merge when the pipeline for 59e0e16b succeeds

  • merged

  • Gosia Ksionek mentioned in commit f1810bae

    mentioned in commit f1810bae

  • mentioned in issue #423756 (closed)

  • added workflowstaging label and removed workflowcanary label

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading