From 5d66b4ac91c0f740be33a077440a9e86775e48bd Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Tue, 11 Jul 2023 10:08:26 +0200 Subject: [PATCH] Adds write policy for Model experiments Model experiments now requires at least user access to the repository to be able to modify or delete experiments and candidates. Changelog: fixed --- .../projects/ml/candidates_controller.rb | 10 +++++-- .../projects/ml/experiments_controller.rb | 9 +++++-- app/policies/project_policy.rb | 4 +++ .../project/ml/experiment_tracking/index.md | 10 ++++--- spec/policies/project_policy_spec.rb | 25 +++++++++++++++++ .../projects/ml/candidates_controller_spec.rb | 23 +++++++++++----- .../ml/experiments_controller_spec.rb | 27 +++++++++++++------ 7 files changed, 86 insertions(+), 22 deletions(-) diff --git a/app/controllers/projects/ml/candidates_controller.rb b/app/controllers/projects/ml/candidates_controller.rb index ed7155fc5f48a1..9905e454acb819 100644 --- a/app/controllers/projects/ml/candidates_controller.rb +++ b/app/controllers/projects/ml/candidates_controller.rb @@ -3,7 +3,9 @@ module Projects module Ml class CandidatesController < ApplicationController - before_action :check_feature_enabled, :set_candidate + before_action :set_candidate + before_action :check_read, only: [:show] + before_action :check_write, only: [:destroy] feature_category :mlops @@ -26,9 +28,13 @@ def set_candidate render_404 unless @candidate.present? end - def check_feature_enabled + def check_read render_404 unless can?(current_user, :read_model_experiments, @project) end + + def check_write + render_404 unless can?(current_user, :write_model_experiments, @project) + end end end end diff --git a/app/controllers/projects/ml/experiments_controller.rb b/app/controllers/projects/ml/experiments_controller.rb index a620e9919e7452..85e7f63779c53e 100644 --- a/app/controllers/projects/ml/experiments_controller.rb +++ b/app/controllers/projects/ml/experiments_controller.rb @@ -5,7 +5,8 @@ module Ml class ExperimentsController < ::Projects::ApplicationController include Projects::Ml::ExperimentsHelper - before_action :check_feature_enabled + before_action :check_read, only: [:show, :index] + before_action :check_write, only: [:destroy] before_action :set_experiment, only: [:show, :destroy] feature_category :mlops @@ -55,10 +56,14 @@ def destroy private - def check_feature_enabled + def check_read render_404 unless can?(current_user, :read_model_experiments, @project) end + def check_write + render_404 unless can?(current_user, :write_model_experiments, @project) + end + def set_experiment @experiment = ::Ml::Experiment.by_project_id_and_iid(@project.id, params[:iid]) diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 0891c66853e367..ad6155258abe86 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -905,6 +905,10 @@ class ProjectPolicy < BasePolicy enable :read_model_experiments end + rule { can?(:reporter_access) & model_experiments_enabled }.policy do + enable :write_model_experiments + end + rule { ~admin & created_and_owned_by_banned_user }.policy do prevent :read_project end diff --git a/doc/user/project/ml/experiment_tracking/index.md b/doc/user/project/ml/experiment_tracking/index.md index 584a6c0df59714..98359911c06877 100644 --- a/doc/user/project/ml/experiment_tracking/index.md +++ b/doc/user/project/ml/experiment_tracking/index.md @@ -14,6 +14,12 @@ On GitLab.com, this feature is enabled on all projects. NOTE: Model experiment tracking is an [experimental feature](../../../../policy/experiment-beta-support.md). Refer to for feedback and feature requests. +ACCESS LEVEL: +Model experiments [visibility level](../../../public_access.md) can be set to public, private or disabled. This options can +be configured under `Settings > General > Visibility, project features, permissions > Model experiments`. Users must have +at least [Reporter role](../../../permissions.md#roles) to modify or delete experiments +and candidate data. + When creating machine learning models, data scientists often experiment with different parameters, configurations, and feature engineering to improve the performance of the model. Keeping track of all this metadata and the associated artifacts so that the data scientist can later replicate the experiment is not trivial. Machine learning experiment @@ -64,10 +70,6 @@ on how to use GitLab as a backend for the MLflow Client. ## Explore model candidates -Prerequisites: - -- You must have at least the Developer role to view experiment data. - To list the current active experiments, either go to `https/-/ml/experiments` or: 1. On the left sidebar, at the top, select **Search GitLab** (**{search}**) to find your project. diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 42fe301ea9eba1..602b7148d0e527 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -3217,6 +3217,31 @@ def permissions_abilities(role) end end + describe ':write_model_experiments' do + using RSpec::Parameterized::TableSyntax + + where(:ff_ml_experiment_tracking, :current_user, :access_level, :allowed) do + false | ref(:owner) | Featurable::ENABLED | false + true | ref(:reporter) | Featurable::ENABLED | true + true | ref(:reporter) | Featurable::PRIVATE | true + true | ref(:reporter) | Featurable::DISABLED | false + true | ref(:guest) | Featurable::ENABLED | false + true | ref(:non_member) | Featurable::ENABLED | false + end + with_them do + before do + stub_feature_flags(ml_experiment_tracking: ff_ml_experiment_tracking) + project.project_feature.update!(model_experiments_access_level: access_level) + end + + if params[:allowed] + it { is_expected.to be_allowed(:write_model_experiments) } + else + it { is_expected.not_to be_allowed(:write_model_experiments) } + end + end + end + describe 'when project is created and owned by a banned user' do let_it_be(:project) { create(:project, :public) } diff --git a/spec/requests/projects/ml/candidates_controller_spec.rb b/spec/requests/projects/ml/candidates_controller_spec.rb index eec7af990637dc..4c7491970e1255 100644 --- a/spec/requests/projects/ml/candidates_controller_spec.rb +++ b/spec/requests/projects/ml/candidates_controller_spec.rb @@ -10,13 +10,17 @@ let(:ff_value) { true } let(:candidate_iid) { candidate.iid } - let(:model_experiments_enabled) { true } + let(:read_model_experiments) { true } + let(:write_model_experiments) { true } before do allow(Ability).to receive(:allowed?).and_call_original allow(Ability).to receive(:allowed?) .with(user, :read_model_experiments, project) - .and_return(model_experiments_enabled) + .and_return(read_model_experiments) + allow(Ability).to receive(:allowed?) + .with(user, :write_model_experiments, project) + .and_return(write_model_experiments) sign_in(user) end @@ -34,9 +38,9 @@ end end - shared_examples '404 when model experiments is unavailable' do + shared_examples 'requires read_model_experiments' do context 'when user does not have access' do - let(:model_experiments_enabled) { false } + let(:read_model_experiments) { false } it_behaves_like 'renders 404' end @@ -61,7 +65,7 @@ end it_behaves_like '404 if candidate does not exist' - it_behaves_like '404 when model experiments is unavailable' + it_behaves_like 'requires read_model_experiments' end describe 'DELETE #destroy' do @@ -83,7 +87,14 @@ end it_behaves_like '404 if candidate does not exist' - it_behaves_like '404 when model experiments is unavailable' + + describe 'requires write_model_experiments' do + let(:write_model_experiments) { false } + + it 'is 404' do + expect(response).to have_gitlab_http_status(:not_found) + end + end end private diff --git a/spec/requests/projects/ml/experiments_controller_spec.rb b/spec/requests/projects/ml/experiments_controller_spec.rb index e2d26e84f752d8..9440c716640f6b 100644 --- a/spec/requests/projects/ml/experiments_controller_spec.rb +++ b/spec/requests/projects/ml/experiments_controller_spec.rb @@ -15,13 +15,17 @@ let(:ff_value) { true } let(:basic_params) { { namespace_id: project.namespace.to_param, project_id: project } } let(:experiment_iid) { experiment.iid } - let(:model_experiments_enabled) { true } + let(:read_model_experiments) { true } + let(:write_model_experiments) { true } before do allow(Ability).to receive(:allowed?).and_call_original allow(Ability).to receive(:allowed?) .with(user, :read_model_experiments, project) - .and_return(model_experiments_enabled) + .and_return(read_model_experiments) + allow(Ability).to receive(:allowed?) + .with(user, :write_model_experiments, project) + .and_return(write_model_experiments) sign_in(user) end @@ -40,9 +44,9 @@ end end - shared_examples '404 when model experiments is unavailable' do + shared_examples 'requires read_model_experiments' do context 'when user does not have access' do - let(:model_experiments_enabled) { false } + let(:read_model_experiments) { false } it_behaves_like 'renders 404' end @@ -100,7 +104,7 @@ end end - it_behaves_like '404 when model experiments is unavailable' do + it_behaves_like 'requires read_model_experiments' do before do list_experiments end @@ -211,7 +215,7 @@ end it_behaves_like '404 if experiment does not exist' - it_behaves_like '404 when model experiments is unavailable' + it_behaves_like 'requires read_model_experiments' end end @@ -243,7 +247,7 @@ end it_behaves_like '404 if experiment does not exist' - it_behaves_like '404 when model experiments is unavailable' + it_behaves_like 'requires read_model_experiments' end end end @@ -268,7 +272,14 @@ end it_behaves_like '404 if experiment does not exist' - it_behaves_like '404 when model experiments is unavailable' + + describe 'requires write_model_experiments' do + let(:write_model_experiments) { false } + + it 'is 404' do + expect(response).to have_gitlab_http_status(:not_found) + end + end end private -- GitLab