Always create a new experiment for a model

To increase the bounds between Model experiments and model registry,
and reduce conflicts when migrating model experiments into model
registry later on, we are forcing the creation of a new experiment when
creating a new model, instead of reusing an existing one with the same
in case it exists. The new experiment also clearly states it belongs to
a model in its name.

iAlso updates services to use ServiceResponse, and deletes
FindOrCreateModelService that wasn't being used anywhere.

app/graphql/mutations/ml/models/create.rb

@@ -19,12 +19,20 @@ class Create < Base
         def resolve(**args)
           project = authorized_find!(args[:project_path])
 
-          model = ::Ml::CreateModelService.new(project, args[:name], current_user, args[:description]).execute
+          service_response = ::Ml::CreateModelService.new(project, args[:name], current_user,
+            args[:description]).execute
 
-          {
-            model: model.persisted? ? model : nil,
-            errors: errors_on_object(model)
-          }
+          if service_response.success?
+            {
+              model: service_response.payload,
+              errors: []
+            }
+          else
+            {
+              model: nil,
+              errors: service_response.errors
+            }
+          end
         end
       end
     end

app/models/ml/model.rb

@@ -5,6 +5,8 @@ class Model < ApplicationRecord
     include Presentable
     include Sortable
 
+    EXPERIMENT_NAME_PREFIX = '[model]'
+
     validates :project, :default_experiment, presence: true
     validates :name,
       format: Gitlab::Regex.ml_model_name_regex,
@@ -39,7 +41,7 @@ def all_packages
     def valid_default_experiment?
       return unless default_experiment
 
-      errors.add(:default_experiment) unless default_experiment.name == name
+      errors.add(:default_experiment) unless default_experiment.name == self.class.prefixed_experiment(name)
       errors.add(:default_experiment) unless default_experiment.project_id == project_id
     end
 
@@ -50,5 +52,9 @@ def self.by_project_id_and_id(project_id, id)
     def self.by_project_id_and_name(project_id, name)
       find_by(project_id: project_id, name: name)
     end
+
+    def self.prefixed_experiment(string)
+      "#{EXPERIMENT_NAME_PREFIX}#{string}"
+    end
   end
 end

app/services/ml/find_or_create_experiment_service.rb → app/services/ml/create_experiment_service.rb

 # frozen_string_literal: true
 
 module Ml
-  class FindOrCreateExperimentService
+  class CreateExperimentService
     def initialize(project, experiment_name, user = nil)
       @project = project
       @name = experiment_name
@@ -9,11 +9,24 @@ def initialize(project, experiment_name, user = nil)
     end
 
     def execute
-      Ml::Experiment.find_or_create(project, name, user)
+      experiment = Ml::Experiment.new(project: project, name: name, user: user)
+      experiment.save
+
+      return error(experiment.errors.full_messages) unless experiment.persisted?
+
+      success(experiment)
     end
 
     private
 
+    def success(model)
+      ServiceResponse.success(payload: model)
+    end
+
+    def error(reason)
+      ServiceResponse.error(message: reason)
+    end
+
     attr_reader :project, :name, :user
   end
 end

app/services/ml/create_model_service.rb

@@ -12,34 +12,42 @@ def initialize(project, name, user = nil, description = nil, metadata = [])
 
     def execute
       ApplicationRecord.transaction do
+        experiment_result = Ml::CreateExperimentService.new(@project, experiment_name, @user).execute
+
+        next experiment_result if experiment_result.error?
+
         model = Ml::Model.new(
           project: @project,
           name: @name,
           user: @user,
           description: @description,
-          default_experiment: default_experiment
+          default_experiment: experiment_result.payload
         )
 
         model.save
 
-        if model.persisted?
-          add_metadata(model, @metadata)
+        next error(model.errors.full_messages) unless model.persisted?
+
+        Gitlab::InternalEvents.track_event(
+          'model_registry_ml_model_created',
+          project: @project,
+          user: @user
+        )
 
-          Gitlab::InternalEvents.track_event(
-            'model_registry_ml_model_created',
-            project: @project,
-            user: @user
-          )
-        end
+        add_metadata(model, @metadata)
 
-        model
+        success(model)
       end
     end
 
     private
 
-    def default_experiment
-      @default_experiment ||= Ml::FindOrCreateExperimentService.new(@project, @name).execute
+    def success(model)
+      ServiceResponse.success(payload: model)
+    end
+
+    def error(reason)
+      ServiceResponse.error(message: reason)
     end
 
     def add_metadata(model, metadata_key_value)
@@ -57,5 +65,9 @@ def add_metadata(model, metadata_key_value)
         ::Ml::ModelMetadata.create!(entry)
       end
     end
+
+    def experiment_name
+      Ml::Model.prefixed_experiment(@name)
+    end
   end
 end

app/services/ml/find_or_create_model_service.rb

-# frozen_string_literal: true
-
-module Ml
-  class FindOrCreateModelService
-    def initialize(project, name, user = nil, description = nil, metadata = [])
-      @project = project
-      @name = name
-      @description = description
-      @metadata = metadata
-      @user = user
-    end
-
-    def execute
-      FindModelService.new(@project, @name).execute ||
-        CreateModelService.new(@project, @name, @user, @description, @metadata).execute
-    end
-  end
-end

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

@@ -128,13 +128,13 @@ client.delete_registered_model(model_name)
 
 ### Logging candidates to a model
 
-Every model has an associated experiment with the same name. To log a candidate/run to the model,
-use the experiment with the name of the model:
+Every model has an associated experiment with the same name prefixed by `[model]`.
+To log a candidate/run to the model, use the experiment passing the correct name:
 
 ```python
 client = MlflowClient()
 model_name = '<your_model_name>'
-exp = client.get_experiment_by_name(model_name)
+exp = client.get_experiment_by_name(f"[model]{model_name}")
 run = client.create_run(exp.experiment_id)
 ```
 

lib/api/ml/mlflow/registered_models.rb

@@ -31,7 +31,7 @@ class RegisteredModels < ::API::Base
             optional :tags, type: Array, desc: 'Additional metadata for registered model.'
           end
           post 'create', urgency: :low do
-            model = ::Ml::CreateModelService.new(
+            service_result = ::Ml::CreateModelService.new(
               user_project,
               params[:name],
               current_user,
@@ -39,9 +39,9 @@ class RegisteredModels < ::API::Base
               params[:tags]
             ).execute
 
-            resource_already_exists! unless model.persisted?
+            resource_already_exists! unless service_result.success?
 
-            present model,
+            present service_result.payload,
               with: Entities::Ml::Mlflow::RegisteredModel,
               root: :registered_model
           rescue ActiveRecord::RecordInvalid

spec/factories/ml/models.rb

@@ -5,7 +5,7 @@
     sequence(:name) { |n| "model#{n}" }
 
     project
-    default_experiment { association :ml_experiments, project_id: project.id, name: name }
+    default_experiment { association :ml_experiments, project_id: project.id, name: "[model]#{name}" }
 
     trait :with_versions do
       versions { Array.new(2) { association(:ml_model_versions, model: instance) } }

spec/models/ml/model_spec.rb

@@ -8,7 +8,7 @@
   let_it_be(:existing_model) { create(:ml_models, name: 'an_existing_model', project: project1) }
   let_it_be(:another_existing_model) { create(:ml_models, name: 'an_existing_model', project: project2) }
   let_it_be(:valid_name) { 'a_valid_name' }
-  let_it_be(:default_experiment) { create(:ml_experiments, name: valid_name, project: project1) }
+  let_it_be(:default_experiment) { create(:ml_experiments, name: "[model]#{valid_name}", project: project1) }
 
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
@@ -164,4 +164,10 @@
       expect(all_packages).to match_array([version1.package, version2.package])
     end
   end
+
+  describe '.prefixed_experiment' do
+    it 'returns the given string prefixed with "[model]"' do
+      expect(described_class.prefixed_experiment('a_valid_name')).to eq('[model]a_valid_name')
+    end
+  end
 end

spec/requests/api/graphql/mutations/ml/models/create_spec.rb

@@ -28,7 +28,7 @@
   end
 
   context 'when user is allowed write changes' do
-    it 'creates a models' do
+    it 'creates a model' do
       post_graphql_mutation(mutation, current_user: current_user)
 
       expect(response).to have_gitlab_http_status(:success)
@@ -39,7 +39,7 @@
     end
 
     context 'when name already exists' do
-      err_msg = "Name has already been taken"
+      err_msg = "Name should be unique in the project"
       let(:name) { model.name }
 
       it_behaves_like 'a mutation that returns errors in the response', errors: [err_msg]

spec/services/ml/find_or_create_experiment_service_spec.rb → spec/services/ml/create_experiment_service_spec.rb

@@ -2,31 +2,32 @@
 
 require 'spec_helper'
 
-RSpec.describe ::Ml::FindOrCreateExperimentService, feature_category: :mlops do
+RSpec.describe ::Ml::CreateExperimentService, feature_category: :mlops do
   let_it_be(:project) { create(:project) }
   let_it_be(:user) { project.first_owner }
   let_it_be(:existing_experiment) { create(:ml_experiments, project: project, user: user) }
 
   let(:name) { 'new_experiment' }
 
-  subject(:new_experiment) { described_class.new(project, name, user).execute }
+  subject(:create_experiment) { described_class.new(project, name, user).execute }
 
   describe '#execute' do
-    it 'creates an experiment using Ml::Experiment.find_or_create', :aggregate_failures do
-      expect(Ml::Experiment).to receive(:find_or_create).and_call_original
+    subject(:created_experiment) { create_experiment.payload }
 
-      expect(new_experiment.name).to eq('new_experiment')
-      expect(new_experiment.project).to eq(project)
-      expect(new_experiment.user).to eq(user)
+    it 'creates an experiment', :aggregate_failures do
+      expect(create_experiment).to be_success
+      expect(created_experiment.name).to eq('new_experiment')
+      expect(created_experiment.project).to eq(project)
+      expect(created_experiment.user).to eq(user)
     end
 
     context 'when experiment already exists' do
       let(:name) { existing_experiment.name }
 
-      it 'fetches existing experiment', :aggregate_failures do
-        expect { new_experiment }.not_to change { Ml::Experiment.count }
+      it 'returns an error', :aggregate_failures do
+        expect { create_experiment }.not_to change { Ml::Experiment.count }
 
-        expect(new_experiment).to eq(existing_experiment)
+        expect(create_experiment).to be_error
       end
     end
   end

spec/services/ml/create_model_service_spec.rb

@@ -16,6 +16,20 @@
   subject(:create_model) { described_class.new(project, name, user, description, metadata).execute }
 
   describe '#execute' do
+    subject(:model_payload) { create_model.payload }
+
+    context 'when model name is not supplied' do
+      let(:name) { nil }
+      let(:project) { existing_model.project }
+
+      it 'returns a model with errors', :aggregate_failures do
+        expect { create_model }.not_to change { Ml::Model.count }
+        expect(create_model).to be_error
+        expect(Gitlab::InternalEvents).not_to have_received(:track_event)
+        expect(create_model.message).to include("Name can't be blank")
+      end
+    end
+
     context 'when model name does not exist in the project' do
       let(:name) { 'new_model' }
       let(:project) { existing_model.project }
@@ -27,7 +41,8 @@
           { project: project, user: user }
         )
 
-        expect(create_model.name).to eq(name)
+        expect(model_payload.name).to eq('new_model')
+        expect(model_payload.default_experiment.name).to eq('[model]new_model')
       end
     end
 
@@ -42,7 +57,7 @@
           { project: project, user: user }
         )
 
-        expect(create_model.name).to eq(name)
+        expect(model_payload.name).to eq(name)
       end
     end
 
@@ -51,9 +66,10 @@
       let(:project) { existing_model.project }
 
       it 'returns a model with errors', :aggregate_failures do
-        expect(create_model).not_to be_persisted
+        expect { create_model }.not_to change { Ml::Model.count }
+        expect(create_model).to be_error
         expect(Gitlab::InternalEvents).not_to have_received(:track_event)
-        expect(create_model.errors.full_messages).to eq(["Name has already been taken"])
+        expect(create_model.message).to eq(["Name should be unique in the project"])
       end
     end
 
@@ -65,8 +81,8 @@
       it 'creates metadata records', :aggregate_failures do
         expect { create_model }.to change { Ml::Model.count }.by(1)
 
-        expect(create_model.name).to eq(name)
-        expect(create_model.metadata.count).to be 2
+        expect(model_payload.name).to eq(name)
+        expect(model_payload.metadata.count).to be 2
       end
     end
 

spec/services/ml/find_or_create_model_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe ::Ml::FindOrCreateModelService, feature_category: :mlops do
-  let_it_be(:user) { create(:user) }
-  let_it_be(:existing_model) { create(:ml_models) }
-  let_it_be(:another_project) { create(:project) }
-  let_it_be(:description) { 'description' }
-  let_it_be(:metadata) { [] }
-
-  subject(:create_model) { described_class.new(project, name, user, description, metadata).execute }
-
-  describe '#execute' do
-    context 'when model name does not exist in the project' do
-      let(:name) { 'new_model' }
-      let(:project) { existing_model.project }
-
-      it 'creates a model', :aggregate_failures do
-        expect { create_model }.to change { Ml::Model.count }.by(1)
-
-        expect(create_model.name).to eq(name)
-      end
-    end
-
-    context 'when model name exists but project is different' do
-      let(:name) { existing_model.name }
-      let(:project) { another_project }
-
-      it 'creates a model', :aggregate_failures do
-        expect { create_model }.to change { Ml::Model.count }.by(1)
-
-        expect(create_model.name).to eq(name)
-      end
-    end
-
-    context 'when model with name exists' do
-      let(:name) { existing_model.name }
-      let(:project) { existing_model.project }
-
-      it 'fetches existing model', :aggregate_failures do
-        expect { create_model }.to change { Ml::Model.count }.by(0)
-
-        expect(create_model).to eq(existing_model)
-      end
-    end
-  end
-end