Skip to content
Snippets Groups Projects

Add MergeRequestReader to chat

Merged Gosia Ksionek requested to merge mk-mr-for-chat into master

What does this MR do and why?

Addresses one popular context the chat currently does not support: https://gitlab.com/gitlab-org/ai-powered/duo-chat/discussions/-/issues/3+

Specifically this MR addresses Support Merge Requests as context for Duo Chat ... (#464587 - closed) • Lesley Razzaghian • 17.5 • Needs attention

Evaluation results

Here are the evaluation results from a collective LLM judge on the master branch

Here are the results on this branch

Here are the stats (averages)

Master branch

Correctness: 3.65

Readability: 3.75

Comprehensiveness: 3.43

This branch

Correctness: 3.71

Readability: 3.75

Comprehensiveness: 3.53

The improved results in the existing evaluation are likely not statistically significant, but at least it proves these changes do not degrade existing questions.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

  1. In rails console enable the experiment fully
    Feature.enable(:ai_merge_request_reader_for_chat)
  2. Visit merge request and ask question, for example: summarize this Merge request

What needs to be done

Task Status Notes
Run CEF locally on this branch vs master to ensure no degradation :white_check_mark: See above in 'Evaluation results'
Create basic dataset for MR eval :white_check_mark: Here
Change Chat REST API to accept MR requests :white_check_mark:
Add seed data for merge requests to be able to test locally in CEF :o: I believe this can be a followup
Make this work with 'v2_chat_agent_integration' feature flag on :o: This MR just needs to be merged after this one has been rolled out
Ask model validation team to add this dataset to daily runs, merge this MR and monitor eval results :o: Asked them to add it discussion
Edited by Torsten Linz

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
  • @mksionek

    Here's a status update on the commit that I pushed to the branch:

    The following tests are mostly complete with test coverage:

      (use "git restore --staged <file>..." to unstage)
            modified:   app/serializers/merge_request_ai_entity.rb
            modified:   ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb
            new file:   ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb
            new file:   ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb
            modified:   ee/spec/lib/gitlab/llm/completions/chat_spec.rb
            new file:   ee/spec/models/ai/ai_resource/merge_request_spec.rb
            new file:   spec/fixtures/api/schemas/entities/merge_request_ai.json
            modified:   spec/serializers/merge_request_serializer_spec.rb

    What still needs to be accomplished:

    1. Move the original from ee/lib/gitlab/llm/templates/summarize_new_merge_request.rb to ee/lib/gitlab/llm/utils/merge_request_tool.rb
    2. Adding more specs to increase the test coverage of ee/spec/lib/gitlab/llm/completions/chat_real_requests_spec.rb for the Merge Request Reader tool.
    3. Fixing the following tests
    ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb
    ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb - # I tried using code suggestions writing this test instead of copying over the original specs.
    spec/serializers/merge_request_ai_entity_spec.rb
    
    1. Still need to add that feature flag.
    2. Still rubocop offenses and potentially more have been added.
    3. Need to do some manual testing for this.

    I'm sorry that I didn't get as far as I wanted. I spent a bit more time trying to write the test exposing the grape entity features. . I've never used this gem before.

    Edited by Nathan Weinshenker
  • added 1 commit

    • 8037691b - Added test specs to the following MR for merge request reader

    Compare with previous version

  • Nathan Weinshenker
  • Ghost User
  • Ghost User
  • Ghost User
  • Nathan Weinshenker
  • Gosia Ksionek added 2 commits

    added 2 commits

    Compare with previous version

  • Gosia Ksionek added 1 commit

    added 1 commit

    Compare with previous version

  • A deleted user added feature flag label

    added feature flag label

  • Ghost User
  • Gosia Ksionek added 1 commit

    added 1 commit

    Compare with previous version

  • Gosia Ksionek added 1 commit

    added 1 commit

    Compare with previous version

  • Gosia Ksionek added 1 commit

    added 1 commit

    • f08bcf6d - Fix specs, add additional action

    Compare with previous version

  • Author Maintainer

    @nateweinshenker what I managed to do today:

    1. Move the original from ee/lib/gitlab/llm/templates/summarize_new_merge_request.rb to ee/lib/gitlab/llm/utils/merge_request_tool.rb - I am not sure what you meant by that
    2. Adding more specs to increase the test coverage of ee/spec/lib/gitlab/llm/completions/chat_real_requests_spec.rb for the Merge Request Reader tool.
    3. Fixing the following tests
    ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb - # I tried using code suggestions writing this test instead of copying over the original specs.

    ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb - fixed spec/serializers/merge_request_ai_entity_spec.rb - fixed

    1. Still need to add that feature flag - added and specs added as well.
    2. Still rubocop offenses and potentially more have been added. - probably I fixed some :laughing:
    3. Need to do some manual testing for this. - I did some, I also found a problem I will describe in the comment below.
    Edited by Gosia Ksionek
    • Author Maintainer
      Resolved by Mark Chao

      Problem I noticed:

      When you start on main MR view and ask "summarize this MR" everything works :chefkiss:. If you click from the main MR view to diff view, still works.

      However, if you're on diff view, and reload page, and then ask "summarize this MR" - the resource is not passed to mutation properly. In MR view, it is covered by this method. On the diff view, we didn't have this concern included. I tried adding it here, but there is some problem with order of loading :weary:. I will investigate further. it is not a correct place.

      Edited by Gosia Ksionek
  • Gosia Ksionek added 2 commits

    added 2 commits

    • d7fb5f6e - Add proper assigning of resource id
    • a8174caf - Add proper assigning of resource id - move to ee

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • 1e3752b7 - Resolve test failure and migrate some test over to different class

    Compare with previous version

  • added 1 commit

    • 25347bc0 - Resolve test failure and migrate some test over to different class

    Compare with previous version

  • added groupduo chat label and removed groupai framework label

  • Gosia Ksionek added 1 commit

    added 1 commit

    Compare with previous version

  • Gosia Ksionek added 2 commits

    added 2 commits

    • 03d50e05 - Refactor to keep everything in EE directory
    • 83fe5ae7 - Prettify json

    Compare with previous version

  • Gosia Ksionek added 2 commits

    added 2 commits

    Compare with previous version

  • Gosia Ksionek added 1 commit

    added 1 commit

    Compare with previous version

  • Author Maintainer

    @lesley-r would you mind reviewing this MR?

  • Gosia Ksionek requested review from @lesley-r

    requested review from @lesley-r

    • Resolved by Mark Chao

      Hi, the code looks great! Thank you for doing this! :bow: I have been manually testing it also quite a bit and it seems to work well.

      I just wanted to share some thoughts.

      1. This has the same problem as issues/epics, where it will try to search using the MergeRequestReader, even though it can't do that. We can address this in a followup MR, but here is where I am trying to fix it for epics and issues.

      imageimage

      (Note: I didn't fix any bug with a z index problem, dunno why I used that example.)

      1. Is the model validation team ready with questions about merge requests in the daily evaluations and prompt library? It seems like we might like that to be in place as we start to roll this out.

      2. The MR seems to be in draft mode :sweat_smile:

      3. (Only blocking change): It still advises the AI to use the MergeRequestReader tool if the feature flag is off. This means that requests that ask about merge requests error out if the feature flag is off.

      This should not be included in the prompt if FF is off.

      The user is currently on a page that displays a merge request with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The title of the merge request is 'Nisi temporibus pariatur aliquid quos quasi voluptates perspiciatis.'. Remember to use the 'MergeRequestReader' tool if they ask a question about the Merge Request

      @mksionek

      Edited by Lesley Razzaghian
  • Gosia Ksionek marked this merge request as ready

    marked this merge request as ready

  • Gosia Ksionek added 2 commits

    added 2 commits

    • d98a7fec - Block by feature flag the current page part for merge requests
    • 420b2a63 - Add better serving of feature flag for merge requests

    Compare with previous version

  • Gosia Ksionek added 1843 commits

    added 1843 commits

    Compare with previous version

  • Gosia Ksionek changed the description

    changed the description

  • Lesley Razzaghian approved this merge request

    approved this merge request

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