Коммит 1ba0be6b создал по автору Marc Shaw's avatar Marc Shaw
Просмотр файлов

Automatically generate a summary for new diffs 

MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118812

Changelog: added
владелец 48b9dae0
Скрыть пробелы
Построчно Рядом
config/feature_flags/development/summarize_diff_automatically.yml 0 → 100644
Просмотр файла @ 1ba0be6b
---
name: summarize_diff_automatically
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118812
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/409135
milestone: '16.1'
type: development
group: group::code review
default_enabled: false
ee/app/models/ee/merge_request_diff.rb
Просмотр файла @ 1ba0be6b
......@@ -16,9 +16,12 @@ module MergeRequestDiff
with_replicator ::Geo::MergeRequestDiffReplicator
has_one :merge_request_diff_detail, autosave: false, inverse_of: :merge_request_diff
has_one :merge_request_diff_llm_summary, class_name: 'MergeRequest::DiffLlmSummary'
after_save :save_verification_details
after_create_commit :prepare_diff_summary, unless: :importing?
scope :has_external_diffs, -> { with_files.where(stored_externally: true) }
scope :project_id_in, ->(ids) { where(merge_request_id: ::MergeRequest.where(target_project_id: ids)) }
scope :available_replicables, -> { has_external_diffs }
......@@ -30,6 +33,22 @@ module MergeRequestDiff
def verification_state_object
merge_request_diff_detail
end
def prepare_diff_summary
llm_service_bot = ::User.llm_bot
return unless llm_service_bot
return if merge_head?
if ::Feature.enabled?(:summarize_diff_automatically, project) &&
Llm::MergeRequests::SummarizeDiffService.enabled?(group: project.root_ancestor, user: llm_service_bot)
::MergeRequests::Llm::SummarizeMergeRequestWorker.perform_async(
llm_service_bot.id,
{ type: ::MergeRequests::Llm::SummarizeMergeRequestWorker::PREPARE_DIFF_SUMMARY, diff_id: id }
)
end
end
end
class_methods do
......
app/models/merge_request/diff_llm_summary.rb → ee/app/models/merge_request/diff_llm_summary.rb
Просмотр файла @ 1ba0be6b
......@@ -8,6 +8,6 @@ class MergeRequest::DiffLlmSummary < ApplicationRecord
validates :provider, presence: true
validates :content, presence: true, length: { maximum: 2056 }
enum provider: { openai: 0 }
enum provider: { open_ai: 0 }
end
# rubocop:enable Style/ClassAndModuleChildren
ee/app/services/llm/merge_requests/summarize_diff_service.rb
Просмотр файла @ 1ba0be6b
......@@ -5,13 +5,16 @@ module MergeRequests
class SummarizeDiffService
GIT_DIFF_PREFIX_REGEX = /\A@@( -\d+,\d+ \+\d+,\d+ )@@/
def initialize(merge_request:, user:)
@merge_request = merge_request
def initialize(title:, user:, diff:)
@title = title
@user = user
@diff = diff
end
def execute
return unless enabled? && @user.can?(:read_merge_request, @merge_request) && llm_client
return unless self.class.enabled?(user: user,
group: diff.merge_request.project.root_ancestor) && user.can?(:read_merge_request,
diff.merge_request) && llm_client
response = llm_client.chat(content: summary_message, moderated: true)
......@@ -21,16 +24,19 @@ def execute
response.parsed_response["choices"].first.dig("message", "content")
end
def enabled?
Feature.enabled?(:openai_experimentation, @user) &&
Gitlab::Llm::StageCheck.available?(@merge_request.project.root_ancestor, :summarize_diff)
def self.enabled?(user:, group:)
Feature.enabled?(:openai_experimentation, user) &&
Gitlab::Llm::StageCheck.available?(group, :summarize_diff) &&
::License.feature_available?(:summarize_mr_changes)
end
private
attr_reader :title, :user, :diff
def prompt
"The above is the code diff of a merge request. The merge request's " \
"title is: '#{@merge_request.title}'\n\n" \
"title is: '#{title}'\n\n" \
"Write a summary the way an expert engineer would summarize the " \
"changes using simple - generally non-technical - terms."
end
......@@ -45,7 +51,7 @@ def summary_message
def extracted_diff
# Extract only the diff strings and discard everything else
#
diffs = @merge_request.raw_diffs.to_a.collect(&:diff)
diffs = diff.raw_diffs.to_a.collect(&:diff)
# Each diff string starts with information about the lines changed,
# bracketed by @@. Removing this saves us tokens.
......@@ -56,7 +62,7 @@ def extracted_diff
end
def llm_client
@_llm_client ||= Gitlab::Llm::OpenAi::Client.new(@user)
@_llm_client ||= Gitlab::Llm::OpenAi::Client.new(user)
end
end
end
......
ee/app/workers/merge_requests/llm/summarize_merge_request_worker.rb
Просмотр файла @ 1ba0be6b
......@@ -13,54 +13,68 @@ class SummarizeMergeRequestWorker
worker_has_external_dependencies!
idempotent!
def perform(merge_request_id, user_id, rev = nil)
@merge_request = MergeRequest.find_by_id(merge_request_id)
return unless @merge_request
@project = @merge_request.project
SUMMARIZE_QUICK_ACTION = 'summarize_quick_action'
PREPARE_DIFF_SUMMARY = 'prepare_diff_summary'
def perform(user_id, params = {})
@user = User.find_by_id(user_id)
return unless @user && @user.can?(:create_note, @project)
# If rev is not provided, use the SHA of the current head commit.
#
# Note that Llm::MergeRequests::SummarizeDiffService does not
# currently support summarizing anything except the most recent diff,
# and providing it when calling this worker aids in job deduplication.
# We will display the rev at the end of the note in order to identify
# which version what summarized.
#
rev ||= @merge_request.diff_head_sha
summary = service.execute
return unless summary
opts = {
note: note_content(summary, rev),
noteable_type: 'MergeRequest',
noteable_id: @merge_request.id
}
# Create the note, but attribute it to the LLM bot
#
Notes::CreateService.new(@project, llm_service_bot, opts).execute
# We removed a param, and added the params, so need to handle the case
# when type is not given
if params['type'] == SUMMARIZE_QUICK_ACTION || params['type'].nil?
merge_request = MergeRequest.find_by_id(params['merge_request_id'])
return unless merge_request
project = merge_request.project
return unless user.can?(:create_note, project)
summary = service(title: merge_request.title, diff: merge_request.merge_request_diff).execute
return unless summary
opts = {
note: note_content(merge_request, summary),
noteable_type: 'MergeRequest',
noteable_id: merge_request.id
}
# Create the note, but attribute it to the LLM bot
#
Notes::CreateService.new(project, llm_service_bot, opts).execute
elsif params['type'] == PREPARE_DIFF_SUMMARY
diff = MergeRequestDiff.find_by_id(params['diff_id'])
return unless diff
summary = service(title: diff.merge_request.title, diff: diff).execute
return unless summary
MergeRequest::DiffLlmSummary.create!(merge_request_diff: diff,
content: summary,
provider: :open_ai)
end
end
private
attr_accessor :user
def llm_service_bot
@_llm_service_bot ||= User.llm_bot
end
# Until we decide the best way to represent generated content
def note_content(summary, rev)
def note_content(merge_request, summary)
rev = merge_request.diff_head_sha
summary + "\n\n---\n_(AI-generated summary for revision #{rev})_"
end
def service
def service(title:, diff:)
@_service ||= ::Llm::MergeRequests::SummarizeDiffService.new(
merge_request: @merge_request,
user: @user
title: title,
user: user,
diff: diff
)
end
end
......
ee/lib/ee/gitlab/quick_actions/merge_request_actions.rb
Просмотр файла @ 1ba0be6b
......@@ -28,16 +28,16 @@ module MergeRequestActions
types MergeRequest
condition do
::Feature.enabled?(:summarize_diff_quick_action, current_user) &&
::License.feature_available?(:summarize_mr_changes) &&
::Llm::MergeRequests::SummarizeDiffService.new(
merge_request: quick_action_target,
::Llm::MergeRequests::SummarizeDiffService.enabled?(
group: quick_action_target.project.root_ancestor,
user: current_user
).enabled?
)
end
command :summarize_diff do
::MergeRequests::Llm::SummarizeMergeRequestWorker.new.perform(
quick_action_target.id,
current_user.id
current_user.id,
{ 'type' => ::MergeRequests::Llm::SummarizeMergeRequestWorker::SUMMARIZE_QUICK_ACTION,
'merge_request_id' => quick_action_target.id }
)
end
end
......
ee/spec/models/ee/merge_request_diff_spec.rb
Просмотр файла @ 1ba0be6b
......@@ -66,6 +66,108 @@
end
end
describe 'preprare_diff_summary' do
let(:merge_request) { create(:merge_request_without_merge_request_diff, target_project: project, source_project: project) }
let(:diff_with_commits) { create(:merge_request_diff, merge_request: merge_request) }
before do
stub_licensed_features(summarize_mr_changes: true)
end
context 'when openai_experimentation feature flag is off' do
before do
stub_feature_flags(openai_experimentation: false)
end
it 'does not call the summarize worker' do
expect(::MergeRequests::Llm::SummarizeMergeRequestWorker).not_to receive(:new)
diff_with_commits
end
end
context 'when summarize_diff_automatically feature flag is off' do
before do
stub_feature_flags(summarize_diff_automatically: false)
end
it 'does not call the summarize worker' do
expect(::MergeRequests::Llm::SummarizeMergeRequestWorker).not_to receive(:new)
diff_with_commits
end
end
context 'when summarize_mr_change feature not avaliable' do
before do
stub_licensed_features(summarize_mr_changes: false)
end
it 'does not call the summarize worker' do
expect(::MergeRequests::Llm::SummarizeMergeRequestWorker).not_to receive(:new)
diff_with_commits
end
end
context 'when the llm_bot does not exist' do
before do
allow(User).to receive(:llm_bot).and_return(nil)
end
it 'does not call the summarize worker' do
expect(::MergeRequests::Llm::SummarizeMergeRequestWorker).not_to receive(:new)
diff_with_commits
end
end
context 'when the diff is not merge_head' do
let(:diff_with_commits) { create(:merge_request_diff, :merge_head) }
it 'does not call the summarize worker' do
expect(::MergeRequests::Llm::SummarizeMergeRequestWorker).not_to receive(:new)
diff_with_commits
end
end
context 'when llm merge requests diff service is not enabled' do
before do
allow(Llm::MergeRequests::SummarizeDiffService)
.to receive(:enabled?)
.at_least(:once)
.with(group: project.root_ancestor, user: User.llm_bot)
.and_return(false)
end
it 'does not call the summarize worker' do
expect(::MergeRequests::Llm::SummarizeMergeRequestWorker).not_to receive(:new)
diff_with_commits
end
end
context 'when all the feature flags are on and the bot available' do
before do
allow(Llm::MergeRequests::SummarizeDiffService)
.to receive(:enabled?)
.at_least(:once)
.with(group: project.root_ancestor, user: User.llm_bot)
.and_return(true)
end
it 'calls the summarize worker' do
allow(::MergeRequests::Llm::SummarizeMergeRequestWorker).to receive(:perform_async)
diff_with_commits
expect(::MergeRequests::Llm::SummarizeMergeRequestWorker).to have_received(:perform_async)
.with(User.llm_bot.id, { type: ::MergeRequests::Llm::SummarizeMergeRequestWorker::PREPARE_DIFF_SUMMARY, diff_id: diff_with_commits.id })
end
end
end
describe '.search' do
let_it_be(:merge_request_diff1) { create(:merge_request_diff) }
let_it_be(:merge_request_diff2) { create(:merge_request_diff) }
......
ee/spec/services/llm/merge_requests/summarize_diff_service_spec.rb
Просмотр файла @ 1ba0be6b
......@@ -30,11 +30,15 @@
let(:response_double) { instance_double(HTTParty::Response, parsed_response: example_response) }
let(:errored_response_double) { instance_double(HTTParty::Response, parsed_response: { error: "true" }) }
subject(:service) { described_class.new(merge_request: merge_request, user: user) }
subject(:service) do
described_class.new(title: merge_request.title, user: user, diff:
merge_request.merge_request_diff)
end
describe "#execute" do
before do
project.add_developer(user)
stub_licensed_features(summarize_mr_changes: true)
merge_request.project.namespace.namespace_settings.update_attribute(:experiment_features_enabled, true)
merge_request.project.namespace.namespace_settings.update_attribute(:third_party_ai_features_enabled, true)
......@@ -42,7 +46,8 @@
context "when the user does not have read access to the MR" do
it "returns without attempting to summarize" do
secondary_service = described_class.new(merge_request: merge_request_2, user: user)
secondary_service = described_class.new(title: merge_request_2.title, user: user, diff:
merge_request_2.merge_request_diff)
expect(secondary_service).not_to receive(:llm_client)
expect(secondary_service.execute).to be_nil
......@@ -62,6 +67,18 @@
end
end
context 'when summarize_mr_change feature not avaliable' do
before do
stub_licensed_features(summarize_mr_changes: false)
end
it "returns without attempting to summarize" do
expect(service).not_to receive(:llm_client)
service.execute
end
end
context 'when the project experiment_features_allowed is false' do
before do
merge_request.project.namespace.namespace_settings.update_attribute(:experiment_features_enabled, false)
......
ee/spec/services/quick_actions/interpret_service_spec.rb
Просмотр файла @ 1ba0be6b
......@@ -452,27 +452,25 @@
context "summarize_diff command" do
let(:content) { "/summarize_diff" }
let(:summarize_flag) { true }
let(:summarize_diff_enabled) { true }
before do
stub_licensed_features(summarize_mr_changes: true)
end
stub_feature_flags(summarize_diff_quick_action: summarize_flag)
context "when :openai_experimentation feature flag is disabled" do
before do
stub_feature_flags(openai_experimentation: false)
end
allow(::Llm::MergeRequests::SummarizeDiffService).to receive(:enabled?).and_return(summarize_diff_enabled)
end
it "doesn't apply /summarize_diff" do
context "when the checks are enabled" do
it "applies /summarize_diff" do
_, _, msg = service.execute(content, merge_request)
expect(msg).to include("Could not apply summarize_diff command")
expect(msg).to include("Request for summary queued")
end
end
context "when :summarize_diff_quick_action feature flag is disabled" do
before do
stub_feature_flags(summarize_diff_quick_action: false)
end
let(:summarize_flag) { false }
it "doesn't apply /summarize_diff" do
_, _, msg = service.execute(content, merge_request)
......@@ -481,43 +479,13 @@
end
end
context "when :openai_experimentation feature flag is enabled" do
before do
stub_feature_flags(openai_experimentation: true)
allow_next_instance_of(::Llm::MergeRequests::SummarizeDiffService) do |service|
allow(service).to receive(:enabled?).and_return(true)
end
end
context "when summarize_mr_changes is disabled" do
before do
stub_licensed_features(summarize_mr_changes: false)
end
it "doesn't apply /summarize_diff" do
_, _, msg = service.execute(content, merge_request)
expect(msg).to include("Could not apply summarize_diff command")
end
end
context "when SummarizeDiffService is disabled" do
let(:summarize_diff_enabled) { false }
it "applies /summarize_diff" do
it "doesn't apply /summarize_diff" do
_, _, msg = service.execute(content, merge_request)
expect(msg).to include("Request for summary queued")
end
context "when :summarize_diff_quick_action feature flag is disabled" do
before do
stub_feature_flags(summarize_diff_quick_action: false)
end
it "doesn't apply /summarize_diff" do
_, _, msg = service.execute(content, merge_request)
expect(msg).to include("Could not apply summarize_diff command")
end
expect(msg).to include("Could not apply summarize_diff command")
end
end
end
......
ee/spec/workers/merge_requests/llm/summarize_merge_request_worker_spec.rb
Просмотр файла @ 1ba0be6b
......@@ -4,97 +4,134 @@
RSpec.describe MergeRequests::Llm::SummarizeMergeRequestWorker, feature_category: :code_review_workflow do
let_it_be(:user) { create(:user) }
let(:project) { create(:project, :with_namespace_settings, :repository, :public) }
let(:merge_request) { create(:merge_request, source_project: project) }
let(:example_llm_response) do
{
"id" => "chatcmpl-72mX77BBH9Hgj196u7BDhKyCTiXxL",
"object" => "chat.completion",
"created" => 1680897573,
"model" => "gpt-3.5-turbo-0301",
"usage" => { "prompt_tokens" => 3447, "completion_tokens" => 57, "total_tokens" => 3504 },
"choices" =>
[{
"message" => { "role" => "assistant", "content" => "An answer from an LLM" },
"finish_reason" => "stop",
"index" => 0
}]
}
end
let(:response_double) { instance_double(HTTParty::Response, parsed_response: example_llm_response) }
let_it_be(:merge_request) { create(:merge_request) }
let_it_be(:project) { merge_request.project }
let_it_be(:message) { 'this is a message from the llm' }
subject(:worker) { described_class.new }
before do
merge_request.project.namespace.namespace_settings.update_attribute(:experiment_features_enabled, true)
merge_request.project.namespace.namespace_settings.update_attribute(:third_party_ai_features_enabled, true)
end
context "when provided an invalid merge_request_id" do
it "returns nil" do
expect(worker.perform(non_existing_record_id, user.id)).to be_nil
end
it "does not create a new note" do
expect { worker.perform(non_existing_record_id, user.id) }.not_to change { Note.count }
allow_next_instance_of(::Llm::MergeRequests::SummarizeDiffService,
title: merge_request.title,
user: user,
diff: merge_request.merge_request_diff) do |service|
allow(service).to receive(:execute).and_return(message)
end
end
context "when provided an invalid user_id" do
let(:params) { [non_existing_record_id, { merge_request_id: merge_request.id }] }
it "returns nil" do
expect(worker.perform(merge_request.id, non_existing_record_id)).to be_nil
expect(worker.perform(*params)).to be_nil
end
it "does not create a new note" do
expect { worker.perform(merge_request.id, non_existing_record_id) }.not_to change { Note.count }
expect { worker.perform(*params) }.not_to change { Note.count }
end
end
context "when user is not able to create new notes" do
it "returns nil" do
expect(worker.perform(merge_request.id, user.id)).to be_nil
context 'when type is summarize_quick_action' do
let(:params) do
[user.id,
{ merge_request_id: merge_request.id,
type: ::MergeRequests::Llm::SummarizeMergeRequestWorker::SUMMARIZE_QUICK_ACTION }]
end
it "does not create a new note" do
expect { worker.perform(merge_request.id, user.id) }.not_to change { Note.count }
end
end
context "when provided an invalid merge_request_id" do
let(:params) do
[user.id,
{ merge_request_id: non_existing_record_id,
type: ::MergeRequests::Llm::SummarizeMergeRequestWorker::SUMMARIZE_QUICK_ACTION }]
end
context "when user can create new notes" do
before do
project.add_developer(user)
it "returns nil" do
expect(worker.perform(*params)).to be_nil
end
allow_next_instance_of(Gitlab::Llm::OpenAi::Client) do |llm_client|
allow(llm_client).to receive(:chat).and_return(response_double)
it "does not create a new note" do
expect { worker.perform(*params) }.not_to change { Note.count }
end
end
it "creates a new note" do
expect { worker.perform(merge_request.id, user.id) }
.to change { Note.count }.by(1)
context "when user is not able to create new notes" do
it "returns nil" do
expect(worker.perform(*params)).to be_nil
end
it "does not create a new note" do
expect { worker.perform(*params) }.not_to change { Note.count }
end
end
it "creates a new note by the llm_bot" do
note = worker.perform(merge_request.id, user.id)
context "when user can create new notes" do
before do
project.add_developer(user)
end
expect(note.author_id).to eq(User.llm_bot.id)
it "creates a note with the returned content" do
note = worker.perform(*params)
expect(note.note)
.to include(message)
end
it "creates a new note" do
expect { worker.perform(*params) }
.to change { Note.count }.by(1)
end
it "creates a new note by the llm_bot" do
note = worker.perform(*params)
expect(note.author_id).to eq(User.llm_bot.id)
end
it "creates a new note associated with the provided MR" do
note = worker.perform(*params)
expect(note.noteable_type).to eq("MergeRequest")
expect(note.noteable_id).to eq(merge_request.id)
end
it "creates a new note with the LLM attribution trailer" do
note = worker.perform(*params)
expect(note.note)
.to include(
"(AI-generated summary for revision #{merge_request.diff_head_sha})"
)
end
end
end
it "creates a new note associated with the provided MR" do
note = worker.perform(merge_request.id, user.id)
context 'when type is prepare_diff_summary' do
let(:params) do
[user.id,
{ type: ::MergeRequests::Llm::SummarizeMergeRequestWorker::PREPARE_DIFF_SUMMARY,
diff_id: merge_request.merge_request_diff.id }]
end
it 'creates a diff llm summary' do
expect { worker.perform(*params) }.to change { ::MergeRequest::DiffLlmSummary.count }.by(1)
expect(note.noteable_type).to eq("MergeRequest")
expect(note.noteable_id).to eq(merge_request.id)
expect(::MergeRequest::DiffLlmSummary.last)
.to have_attributes(
merge_request_diff: merge_request.merge_request_diff,
content: message,
provider: 'open_ai')
end
it "creates a new note with the LLM attribution trailer" do
note = worker.perform(merge_request.id, user.id)
context 'when the diff does not exist' do
let(:params) do
[user.id,
{ type: ::MergeRequests::Llm::SummarizeMergeRequestWorker::PREPARE_DIFF_SUMMARY,
diff_id: non_existing_record_id }]
end
expect(note.note)
.to include(
"(AI-generated summary for revision #{merge_request.diff_head_sha})"
)
it 'does not create a diff llm summary' do
expect { worker.perform(*params) }.not_to change { ::MergeRequest::DiffLlmSummary.count }
end
end
end
end
Поддерживает Markdown
0% или .
You are about to add 0 people to the discussion. Proceed with caution.
Сначала завершите редактирование этого сообщения!
Пожалуйста, зарегистрируйтесь или чтобы прокомментировать