diff --git a/app/models/ml/experiment.rb b/app/models/ml/experiment.rb index cae4a71fcb4736775e38e15131b3d8f74e2511f5..1d94b51387252be2690af2ff8b7120f062d189ec 100644 --- a/app/models/ml/experiment.rb +++ b/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 diff --git a/app/services/ml/destroy_experiment_service.rb b/app/services/ml/destroy_experiment_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..fc7f7cb3c20b1b1696fe0d6253cb9c899c3dd9e9 --- /dev/null +++ b/app/services/ml/destroy_experiment_service.rb @@ -0,0 +1,23 @@ +# 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 diff --git a/doc/user/project/ml/experiment_tracking/mlflow_client.md b/doc/user/project/ml/experiment_tracking/mlflow_client.md index c39d3fe16ff846db8be2d172b13df9bf74a68149..a52013ec67cfbfd824a6a12b6e6fafb0a1b67e52 100644 --- a/doc/user/project/ml/experiment_tracking/mlflow_client.md +++ b/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. | diff --git a/lib/api/ml/mlflow/experiments.rb b/lib/api/ml/mlflow/experiments.rb index 511922782e8156418c576955d8fac2837fd621e7..1854af4721eb5cb9bf4b35e90e26f8180a9588bb 100644 --- a/lib/api/ml/mlflow/experiments.rb +++ b/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 diff --git a/spec/models/ml/experiment_spec.rb b/spec/models/ml/experiment_spec.rb index b28b1e058260ca22bb3d444d00d419ae507a480d..80aad5a02df10766960e92bf4365f9c0b5d0c1c0 100644 --- a/spec/models/ml/experiment_spec.rb +++ b/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 diff --git a/spec/requests/api/ml/mlflow/experiments_spec.rb b/spec/requests/api/ml/mlflow/experiments_spec.rb index af59fedefd00782d874f8cf011cacad524b5ec53..3b077b0dba1bcff4aab38dc59b194c3eca6175ad 100644 --- a/spec/requests/api/ml/mlflow/experiments_spec.rb +++ b/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 diff --git a/spec/services/ml/destroy_experiment_service_spec.rb b/spec/services/ml/destroy_experiment_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..f5aa0335340cc511369244aac31c2fb105e5e9bd --- /dev/null +++ b/spec/services/ml/destroy_experiment_service_spec.rb @@ -0,0 +1,51 @@ +# 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