diff --git a/app/graphql/mutations/ml/models/create.rb b/app/graphql/mutations/ml/models/create.rb
index 21570fc34b8f9724b4f6b6bef0bd81ee1dcbd249..c0f22fa7904ca3870b943d6a4afa21674d335a9f 100644
--- a/app/graphql/mutations/ml/models/create.rb
+++ b/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
diff --git a/app/models/ml/model.rb b/app/models/ml/model.rb
index 1d5625ec8de2689d7020b0e5093dddd6da6490b0..c00c57538dacabb8d9d21a557b28db9181f022cb 100644
--- a/app/models/ml/model.rb
+++ b/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
diff --git a/app/services/ml/create_experiment_service.rb b/app/services/ml/create_experiment_service.rb
new file mode 100644
index 0000000000000000000000000000000000000000..ed312b6e9650458349c597f18b264916103aff38
--- /dev/null
+++ b/app/services/ml/create_experiment_service.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+module Ml
+ class CreateExperimentService
+ def initialize(project, experiment_name, user = nil)
+ @project = project
+ @name = experiment_name
+ @user = user
+ end
+
+ def execute
+ 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
diff --git a/app/services/ml/create_model_service.rb b/app/services/ml/create_model_service.rb
index 7ac9c2a27371b42118e13dc00f6db6887fe6a560..46035194a12f5b4dfa1894bad66141c4dfbfb13e 100644
--- a/app/services/ml/create_model_service.rb
+++ b/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
diff --git a/app/services/ml/find_or_create_experiment_service.rb b/app/services/ml/find_or_create_experiment_service.rb
deleted file mode 100644
index 1fe10c7f8562e3f44372c34ae3538341759e69ff..0000000000000000000000000000000000000000
--- a/app/services/ml/find_or_create_experiment_service.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-# frozen_string_literal: true
-
-module Ml
- class FindOrCreateExperimentService
- def initialize(project, experiment_name, user = nil)
- @project = project
- @name = experiment_name
- @user = user
- end
-
- def execute
- Ml::Experiment.find_or_create(project, name, user)
- end
-
- private
-
- attr_reader :project, :name, :user
- end
-end
diff --git a/app/services/ml/find_or_create_model_service.rb b/app/services/ml/find_or_create_model_service.rb
deleted file mode 100644
index 9199730e84bfc0ec85f364b1e4918ee6b5255dff..0000000000000000000000000000000000000000
--- a/app/services/ml/find_or_create_model_service.rb
+++ /dev/null
@@ -1,18 +0,0 @@
-# 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
diff --git a/doc/user/project/ml/experiment_tracking/mlflow_client.md b/doc/user/project/ml/experiment_tracking/mlflow_client.md
index d52e2c2fae17446a3a31c046bda9a1b9a9450020..0fc139cfc23a67c7a88bd1f915a36ecb99656fad 100644
--- a/doc/user/project/ml/experiment_tracking/mlflow_client.md
+++ b/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)
```
diff --git a/lib/api/ml/mlflow/registered_models.rb b/lib/api/ml/mlflow/registered_models.rb
index 3f4996a94c07f6c8b05e4922d63c215fb7e89c39..8a9434d24471b4d509e92590609a8f8e6ad20968 100644
--- a/lib/api/ml/mlflow/registered_models.rb
+++ b/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
diff --git a/spec/factories/ml/models.rb b/spec/factories/ml/models.rb
index ae00ade9054957164f1c2935c5a7425716c65726..cf7322bd62a20622d683f5b44ed0c56b72489092 100644
--- a/spec/factories/ml/models.rb
+++ b/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) } }
diff --git a/spec/models/ml/model_spec.rb b/spec/models/ml/model_spec.rb
index e1de44b00304c9e48b7bc83585c538376c5b0790..38d44938b7428ea407f62f47355084002f195a2f 100644
--- a/spec/models/ml/model_spec.rb
+++ b/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
diff --git a/spec/requests/api/graphql/mutations/ml/models/create_spec.rb b/spec/requests/api/graphql/mutations/ml/models/create_spec.rb
index 0daabeab0d18035e29e80db01e13a0315010ff30..959e67d63fb5582a3012d38aa66286d329cd7b8a 100644
--- a/spec/requests/api/graphql/mutations/ml/models/create_spec.rb
+++ b/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]
diff --git a/spec/services/ml/create_experiment_service_spec.rb b/spec/services/ml/create_experiment_service_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..5fdca79cb33dda90174be194343a3c05ab2ce34f
--- /dev/null
+++ b/spec/services/ml/create_experiment_service_spec.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+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(:create_experiment) { described_class.new(project, name, user).execute }
+
+ describe '#execute' do
+ subject(:created_experiment) { create_experiment.payload }
+
+ 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 'returns an error', :aggregate_failures do
+ expect { create_experiment }.not_to change { Ml::Experiment.count }
+
+ expect(create_experiment).to be_error
+ end
+ end
+ end
+end
diff --git a/spec/services/ml/create_model_service_spec.rb b/spec/services/ml/create_model_service_spec.rb
index 88e7c00d1f9668f48c9ef9726abab1136eeac5aa..dd4d41b49955e7465e4098227325aac15d917373 100644
--- a/spec/services/ml/create_model_service_spec.rb
+++ b/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
diff --git a/spec/services/ml/find_or_create_experiment_service_spec.rb b/spec/services/ml/find_or_create_experiment_service_spec.rb
deleted file mode 100644
index a8c533d13203532180cd16e564f1230a759f3580..0000000000000000000000000000000000000000
--- a/spec/services/ml/find_or_create_experiment_service_spec.rb
+++ /dev/null
@@ -1,33 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe ::Ml::FindOrCreateExperimentService, 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 }
-
- 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
-
- expect(new_experiment.name).to eq('new_experiment')
- expect(new_experiment.project).to eq(project)
- expect(new_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 }
-
- expect(new_experiment).to eq(existing_experiment)
- end
- end
- end
-end
diff --git a/spec/services/ml/find_or_create_model_service_spec.rb b/spec/services/ml/find_or_create_model_service_spec.rb
deleted file mode 100644
index 5d5eaea0a72d2358562e968edcecd4ebd999ea87..0000000000000000000000000000000000000000
--- a/spec/services/ml/find_or_create_model_service_spec.rb
+++ /dev/null
@@ -1,48 +0,0 @@
-# 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