Add MergeRequestReader to chat
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.
- In rails console enable the experiment fully
Feature.enable(:ai_merge_request_reader_for_chat)
- 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 | ![]() |
See above in 'Evaluation results' |
Create basic dataset for MR eval | ![]() |
Here |
Change Chat REST API to accept MR requests | ![]() |
|
Add seed data for merge requests to be able to test locally in CEF | ![]() |
I believe this can be a followup |
Make this work with 'v2_chat_agent_integration' feature flag on | ![]() |
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 | ![]() |
Asked them to add it discussion |
Merge request reports
Activity
assigned to @mksionek
added pipelinetier-1 label
- A deleted user
added backend label
16 Warnings This merge request is quite big (928 lines changed), please consider splitting it into multiple merge requests. ee4a078c: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 243c347f: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 3052f065: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. db535cdc: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 8bd728ca: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. f69746bc: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 5242a74c: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. e18eaad3: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 299d7d9a: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 8ba46873: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. d0a95c83: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 7d9dac32: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. This merge request has more than 20 commits which may cause issues in some of the jobs. If you see errors like missing commits, please consider squashing some commits so it is within 20 commits. The master pipeline status page reported failures in - rspec unit pg14 single-redis 7/44
- rspec-ee unit pg14 single-redis 36/39
- rspec integration pg14 single-redis 15/20
- rspec integration pg14 single-redis 13/20
- rspec unit pg14 single-redis 42/44
- rspec unit pg14 single-redis 40/44
- rspec unit pg14 single-redis 20/44
- rspec unit pg14 single-redis 19/44
If these jobs fail in your merge request with the same errors, then they are not caused by your changes.
Please check for any on-going incidents in the incident issue tracker or in the#master-broken
Slack channel.This merge request does not refer to an existing milestone. 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/user/gitlab_duo_chat/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
Category Reviewer Maintainer backend @tigerwnz
(UTC+12, 10 hours ahead of author)
@DylanGriffith
(UTC+10, 8 hours ahead of author)
Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger- Resolved by Mark Chao
@mksionek - please see the following guidance and update this merge request.1 Error Please add typebug typefeature, or typemaintenance label to this merge request.
added groupai framework label
added devopsai-powered sectiondata-science labels
@mksionek Thanks for kickstarting this.
@tlinz the way for Chat to pick this up would be for you to create an issue that outlines the requirements for this capability. You can link to this MR as a reference with the notes from Gosia about what the work entails:
it requires tests, rubocop is failing, and of course a lot of quality checks
This issue should then follow the process to go to Planning Breakdown, Ready for Development and be planned for a future iteration. Thanks.
- Resolved by Mark Chao
@nateweinshenker what this MR is missing ATM:
- there are rubocop failures, probably not that hard to fix
- it lacks specs for new classes
- I moved some methods from
ee/lib/gitlab/llm/templates/summarize_new_merge_request.rb
toee/lib/gitlab/llm/utils/merge_request_tool.rb
, so those specs should be in the old class tests
- I moved some methods from
- we could add more specs to
ee/spec/lib/gitlab/llm/completions/chat_real_requests_spec.rb
- we should guard the changes in the zero-shot agent prompt with feature flag.
- we need to run it locally against bigger MRs to make sure it works, particularly how it handles diff
- resource name in merge request reader is weird, but it's used in one method and it needs to be weird. Unfortunately, it's also used in zero shot prompt, that requires refactoring.
Nathan, let me know what you can do, I will cover the rest tomorrow!
CC @oregand
Edited by Gosia Ksionek
- Resolved by Mark Chao
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:
- Move the original from
ee/lib/gitlab/llm/templates/summarize_new_merge_request.rb
toee/lib/gitlab/llm/utils/merge_request_tool.rb
- 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. - 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
- Still need to add that feature flag.
- Still rubocop offenses and potentially more have been added.
- 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- Move the original from
added 1 commit
- 8037691b - Added test specs to the following MR for merge request reader
- Resolved by Mark Chao
- Resolved by Mark Chao
- Resolved by Mark Chao
- Resolved by Mark Chao
- Resolved by Mark Chao
added 2 commits
- A deleted user
added feature flag label
- Resolved by Mark Chao
@nateweinshenker what I managed to do today:
- Move the original from
ee/lib/gitlab/llm/templates/summarize_new_merge_request.rb
toee/lib/gitlab/llm/utils/merge_request_tool.rb
- I am not sure what you meant by that - 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. - 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
- Still need to add that feature flag - added and specs added as well.
- Still rubocop offenses and potentially more have been added. - probably I fixed some
- 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- Move the original from
- Resolved by Mark Chao
Problem I noticed:
When you start on main MR view and ask "summarize this MR" everything works
. 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 loadingit is not a correct place. . I will investigate further.Edited by Gosia Ksionek
added 1 commit
- 1e3752b7 - Resolve test failure and migrate some test over to different class
added 1 commit
- 25347bc0 - Resolve test failure and migrate some test over to different class
added groupduo chat label and removed groupai framework label
added Category:Duo Chat label
added 2 commits
added 2 commits
@lesley-r would you mind reviewing this MR?
requested review from @lesley-r
- Resolved by Mark Chao
Hi, the code looks great! Thank you for doing this!
I have been manually testing it also quite a bit and it seems to work well.I just wanted to share some thoughts.
- 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.
(Note: I didn't fix any bug with a z index problem, dunno why I used that example.)
-
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.
-
The MR seems to be in draft mode
-
(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
Edited by Lesley Razzaghian
added 1843 commits
-
420b2a63...ee6fa469 - 1820 commits from branch
master
- ee6fa469...fe980848 - 13 earlier commits
- 27050663 - Resolve test failure and migrate some test over to different class
- cb07262f - Fix failing specs
- ed1f2405 - Refactor to keep everything in EE directory
- 198f80bc - Prettify json
- c34e64ef - Fix failing specs
- cc3f4ba5 - Simplify specs
- 077680d9 - Remove obsolete change
- 027598d5 - Block by feature flag the current page part for merge requests
- a58a4e35 - Add better serving of feature flag for merge requests
- ce8abf87 - Fix bad rebase
Toggle commit list-
420b2a63...ee6fa469 - 1820 commits from branch
added pipeline:mr-approved label
- Resolved by Mark Chao
@lesley-r
, thanks for approving this merge request.This is the first time the merge request has been approved. To ensure we don't only run predictive pipelines, and we don't break
master
, a new pipeline will be started shortly.Please wait for the pipeline to start before resolving this discussion and set auto-merge for the new pipeline. See merging a merge request for more details.