Merge branch '482876-mlflow-compatibility-candidate-and-experiment-deletion' into 'master'

Model Registry MLFlow compatibility: Experiment deletion

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/167414



Merged-by: Adam Hegyi <ahegyi@gitlab.com>
Approved-by: Suraj Tripathi <stripathi@gitlab.com>
Approved-by: Rutger Wessels <rwessels@gitlab.com>
Approved-by: Adam Hegyi <ahegyi@gitlab.com>
Reviewed-by: Rutger Wessels <rwessels@gitlab.com>
Co-authored-by: fdegier <fdegier@gitlab.com>

app/models/ml/experiment.rb

@@ -42,7 +42,7 @@ def for_model?
     def stop_destroy
       return unless model_id
 
-      errors[:base] << "Cannot delete an experiment associated to a model"
+      errors.add(:base, "Cannot delete an experiment associated to a model")
       # According to docs, throw is the correct way to stop on a callback
       # https://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html#module-ActiveRecord::Callbacks-label-Canceling+callbacks
       throw :abort # rubocop:disable Cop/BanCatchThrow

app/services/ml/destroy_experiment_service.rb

+# frozen_string_literal: true
+
+module Ml
+  class DestroyExperimentService
+    def initialize(experiment)
+      @experiment = experiment
+    end
+
+    def execute
+      if @experiment.destroy
+        ServiceResponse.success(payload: payload)
+      else
+        ServiceResponse.error(message: @experiment.errors.full_messages, payload: payload)
+      end
+    end
+
+    private
+
+    def payload
+      { experiment: @experiment }
+    end
+  end
+end

doc/user/project/ml/experiment_tracking/mlflow_client.md

@@ -285,6 +285,7 @@ of the methods below are also supported with the same caveats.
 |--------------------------|-----------------|---------------|----------------------------------------------------------------------------------------------|
 | `get_experiment`         | Yes             | 15.11         |                                                                                              |
 | `get_experiment_by_name` | Yes             | 15.11         |                                                                                              |
+| `delete_experiment`      | Yes             | 17.5          |                                                                                              |
 | `set_experiment`         | Yes             | 15.11         |                                                                                              |
 | `get_run`                | Yes             | 15.11         |                                                                                              |
 | `start_run`              | Yes             | 15.11         | (16.3) If a name is not provided, the candidate receives a random nickname.                  |

lib/api/ml/mlflow/experiments.rb

@@ -114,6 +114,23 @@ class Experiments < ::API::Base
 
             {}
           end
+
+          desc 'Delete an experiment.' do
+            summary 'Delete an experiment.'
+
+            detail 'https://mlflow.org/docs/latest/rest-api.html#delete-experiment'
+          end
+          params do
+            requires :experiment_id, type: String, desc: 'ID of the experiment.'
+          end
+          post 'delete', urgency: :low do
+            destroy = ::Ml::DestroyExperimentService.new(experiment).execute
+            if destroy.success?
+              present({})
+            else
+              render_api_error!(destroy.message.first, 400)
+            end
+          end
         end
       end
     end

spec/models/ml/experiment_spec.rb

@@ -30,6 +30,7 @@
       experiment = create(:ml_models, project: exp.project).default_experiment
 
       expect { experiment.destroy! }.to raise_error(ActiveRecord::ActiveRecordError)
+      expect(experiment.errors.full_messages).to include('Cannot delete an experiment associated to a model')
     end
   end
 

spec/requests/api/ml/mlflow/experiments_spec.rb

@@ -284,4 +284,45 @@
       it_behaves_like 'MLflow|Requires api scope and write permission'
     end
   end
+
+  describe 'POST /projects/:id/ml/mlflow/api/2.0/mlflow/experiments/delete' do
+    let(:route) { "/projects/#{project_id}/ml/mlflow/api/2.0/mlflow/experiments/delete" }
+    let(:default_params) { { experiment_id: experiment.iid.to_s } }
+    let(:params) { default_params }
+    let(:request) { post api(route), params: params, headers: headers }
+
+    it 'deletes the experiment', :aggregate_failures do
+      is_expected.to have_gitlab_http_status(:ok)
+      expect { experiment.reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+
+    describe 'Error States' do
+      context 'when experiment does not exist' do
+        let(:params) { default_params.merge(experiment_id: non_existing_record_iid.to_s) }
+
+        it_behaves_like 'MLflow|Not Found - Resource Does Not Exist'
+      end
+
+      context 'when experiment has a model_id' do
+        let(:model) { create(:ml_models, project: project) }
+        let(:experiment) { create(:ml_experiments, :with_metadata, project: project, model_id: model.id) }
+
+        it 'returns an error' do
+          is_expected.to have_gitlab_http_status(:bad_request)
+          expect(json_response).to include({ 'message' => 'Cannot delete an experiment associated to a model' })
+        end
+
+        it_behaves_like 'MLflow|Bad Request'
+      end
+
+      context 'when experiment_id is not passed' do
+        let(:params) { {} }
+
+        it_behaves_like 'MLflow|Bad Request'
+      end
+
+      it_behaves_like 'MLflow|shared error cases'
+      it_behaves_like 'MLflow|Requires api scope and write permission'
+    end
+  end
 end

spec/services/ml/destroy_experiment_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Ml::DestroyExperimentService, feature_category: :mlops do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:model) { create(:ml_models, project: project) }
+
+  let(:experiment) { create(:ml_experiments, project: project) }
+  let(:experiment_with_model) { create(:ml_experiments, project: project, model_id: model.id) }
+  let(:service) { described_class.new(experiment) }
+
+  describe '#execute' do
+    subject(:destroy_result) { service.execute }
+
+    context 'when experiment is successfully destroyed' do
+      it 'returns a success response' do
+        expect(destroy_result).to be_success
+      end
+
+      it 'destroys the experiment' do
+        expect(destroy_result).to be_success
+        expect(destroy_result.payload[:experiment]).to eq(experiment)
+        expect(Ml::Experiment.find_by(id: experiment.id)).to be_nil
+      end
+    end
+
+    context 'when experiment fails to destroy' do
+      before do
+        allow(experiment).to receive(:destroy).and_return(false)
+      end
+
+      it 'returns an error response' do
+        expect(destroy_result).to be_error
+      end
+    end
+
+    context 'when experiment is associated with a model' do
+      let(:experiment) { experiment_with_model }
+
+      it 'returns an error response' do
+        expect(destroy_result).to be_error
+        expect(destroy_result.message[0]).to eq('Cannot delete an experiment associated to a model')
+      end
+
+      it 'does not destroy the experiment' do
+        expect(Ml::Experiment.find_by(id: experiment.id)).to eq(experiment)
+      end
+    end
+  end
+end