From 1ebcd3f32f1eeba0d95221ed86d45177ac888de1 Mon Sep 17 00:00:00 2001 From: Eduardo Bonet <ebonet@gitlab.com> Date: Thu, 12 Jan 2023 15:51:36 +0100 Subject: [PATCH] Adds Candidate Search to Experiment page Moves fetching code to CandidateFinder and adds a search box to ml_experiments.vue component, reusing RegistrySearch component. Using GlFilteredSearch is a bit of overkill here since we are not using Filters, but this is planned to come soon Changelog: added FeatureFlag: ml_experiment_tracking --- .../components/ml_experiment.vue | 102 ++- .../ml/experiment_tracking/constants.js | 16 + .../projects/ml/experiments_controller.rb | 25 +- app/finders/projects/ml/candidate_finder.rb | 63 ++ app/helpers/projects/ml/experiments_helper.rb | 17 + app/models/ml/candidate.rb | 8 + .../projects/ml/experiments/show.html.haml | 2 +- locale/gitlab.pot | 36 +- .../projects/ml/candidate_finder_spec.rb | 79 ++ .../__snapshots__/ml_experiment_spec.js.snap | 761 ------------------ .../components/ml_experiment_spec.js | 372 +++++++-- .../projects/ml/experiments_helper_spec.rb | 64 ++ .../api/entities/ml/mlflow/run_info_spec.rb | 2 +- spec/models/ml/candidate_spec.rb | 69 +- .../ml/experiments_controller_spec.rb | 28 +- 15 files changed, 758 insertions(+), 886 deletions(-) create mode 100644 app/assets/javascripts/ml/experiment_tracking/constants.js create mode 100644 app/finders/projects/ml/candidate_finder.rb create mode 100644 spec/finders/projects/ml/candidate_finder_spec.rb delete mode 100644 spec/frontend/ml/experiment_tracking/components/__snapshots__/ml_experiment_spec.js.snap diff --git a/app/assets/javascripts/ml/experiment_tracking/components/ml_experiment.vue b/app/assets/javascripts/ml/experiment_tracking/components/ml_experiment.vue index 5d13122765af2..61278f521120e 100644 --- a/app/assets/javascripts/ml/experiment_tracking/components/ml_experiment.vue +++ b/app/assets/javascripts/ml/experiment_tracking/components/ml_experiment.vue @@ -1,8 +1,16 @@ <script> import { GlTable, GlLink, GlPagination, GlTooltipDirective } from '@gitlab/ui'; -import { __ } from '~/locale'; -import { getParameterValues, setUrlParams } from '~/lib/utils/url_utility'; import TimeAgo from '~/vue_shared/components/time_ago_tooltip.vue'; +import RegistrySearch from '~/vue_shared/components/registry/registry_search.vue'; +import { FILTERED_SEARCH_TERM } from '~/vue_shared/components/filtered_search_bar/constants'; +import { + LIST_KEY_CREATED_AT, + BASE_SORT_FIELDS, + METRIC_KEY_PREFIX, +} from '~/ml/experiment_tracking/constants'; +import { s__ } from '~/locale'; +import { queryToObject, setUrlParams, visitUrl } from '~/lib/utils/url_utility'; +import { capitalizeFirstCharacter } from '~/lib/utils/text_utility'; import IncubationAlert from './incubation_alert.vue'; export default { @@ -13,18 +21,36 @@ export default { TimeAgo, IncubationAlert, GlPagination, + RegistrySearch, }, directives: { GlTooltip: GlTooltipDirective, }, inject: ['candidates', 'metricNames', 'paramNames', 'pagination'], data() { + const query = queryToObject(window.location.search); + + const filter = query.name ? [{ value: { data: query.name }, type: FILTERED_SEARCH_TERM }] : []; + + let orderBy = query.orderBy || LIST_KEY_CREATED_AT; + + if (query.orderByType === 'metric') { + orderBy = `${METRIC_KEY_PREFIX}${orderBy}`; + } + return { - page: parseInt(getParameterValues('page')[0], 10) || 1, + page: parseInt(query.page, 10) || 1, + filters: filter, + sorting: { + orderBy, + sort: (query.sort || 'desc').toLowerCase(), + }, }; }, computed: { fields() { + if (this.candidates.length === 0) return []; + return [ { key: 'name', label: this.$options.i18n.nameLabel }, { key: 'created_at', label: this.$options.i18n.createdAtLabel }, @@ -44,21 +70,63 @@ export default { nextPage() { return !this.pagination.isLastPage ? this.pagination.page + 1 : null; }, + sortableFields() { + return [ + ...BASE_SORT_FIELDS, + ...this.metricNames.map((name) => ({ + orderBy: `${METRIC_KEY_PREFIX}${name}`, + label: capitalizeFirstCharacter(name), + })), + ]; + }, + parsedQuery() { + const name = this.filters + .map((f) => f.value.data) + .join(' ') + .trim(); + + const filterByQuery = name === '' ? {} : { name }; + + let orderByType = 'column'; + let { orderBy } = this.sorting; + const { sort } = this.sorting; + + if (orderBy.startsWith(METRIC_KEY_PREFIX)) { + orderBy = this.sorting.orderBy.slice(METRIC_KEY_PREFIX.length); + orderByType = 'metric'; + } + + return { ...filterByQuery, orderBy, orderByType, sort }; + }, }, methods: { generateLink(page) { - return setUrlParams({ page }); + return setUrlParams({ ...this.parsedQuery, page }); + }, + submitFilters() { + return visitUrl(setUrlParams({ ...this.parsedQuery, page: this.page })); + }, + updateFilters(newValue) { + this.filters = newValue; + }, + updateSorting(newValue) { + this.sorting = { ...this.sorting, ...newValue }; + }, + updateSortingAndEmitUpdate(newValue) { + this.updateSorting(newValue); + this.submitFilters(); }, }, i18n: { - titleLabel: __('Experiment candidates'), - emptyStateLabel: __('This experiment has no logged candidates'), - artifactsLabel: __('Artifacts'), - detailsLabel: __('Details'), - userLabel: __('User'), - createdAtLabel: __('Created at'), - nameLabel: __('Name'), - noDataContent: __('-'), + titleLabel: s__('MlExperimentTracking|Experiment candidates'), + emptyStateLabel: s__('MlExperimentTracking|No candidates to display'), + artifactsLabel: s__('MlExperimentTracking|Artifacts'), + detailsLabel: s__('MlExperimentTracking|Details'), + userLabel: s__('MlExperimentTracking|User'), + createdAtLabel: s__('MlExperimentTracking|Created at'), + nameLabel: s__('MlExperimentTracking|Name'), + noDataContent: s__('MlExperimentTracking|-'), + filterCandidatesLabel: s__('MlExperimentTracking|Filter candidates'), }, }; </script> @@ -71,6 +139,16 @@ export default { {{ $options.i18n.titleLabel }} </h3> + <registry-search + :filters="filters" + :sorting="sorting" + :sortable-fields="sortableFields" + @sorting:changed="updateSortingAndEmitUpdate" + @filter:changed="updateFilters" + @filter:submit="submitFilters" + @filter:clear="filters = []" + /> + <gl-table :fields="fields" :items="candidates" diff --git a/app/assets/javascripts/ml/experiment_tracking/constants.js b/app/assets/javascripts/ml/experiment_tracking/constants.js new file mode 100644 index 0000000000000..c9bdc78b69a29 --- /dev/null +++ b/app/assets/javascripts/ml/experiment_tracking/constants.js @@ -0,0 +1,16 @@ +import { __ } from '~/locale'; + +export const METRIC_KEY_PREFIX = 'metric.'; + +export const LIST_KEY_CREATED_AT = 'created_at'; + +export const BASE_SORT_FIELDS = Object.freeze([ + { + orderBy: 'name', + label: __('Name'), + }, + { + orderBy: LIST_KEY_CREATED_AT, + label: __('Created at'), + }, +]); diff --git a/app/controllers/projects/ml/experiments_controller.rb b/app/controllers/projects/ml/experiments_controller.rb index 1e1c4b1587cdc..5e7e6828a3b1f 100644 --- a/app/controllers/projects/ml/experiments_controller.rb +++ b/app/controllers/projects/ml/experiments_controller.rb @@ -3,6 +3,8 @@ module Projects module Ml class ExperimentsController < ::Projects::ApplicationController + include Projects::Ml::ExperimentsHelper + before_action :check_feature_flag feature_category :mlops @@ -19,24 +21,19 @@ def show return redirect_to project_ml_experiments_path(@project) unless @experiment.present? - page = params[:page].to_i - page = 1 if page == 0 + find_params = params + .transform_keys(&:underscore) + .permit(:name, :order_by, :sort, :order_by_type) + + @candidates = CandidateFinder.new(@experiment, find_params).execute - @candidates = @experiment.candidates - .including_relationships - .page(page) - .per(MAX_CANDIDATES_PER_PAGE) + page = [params[:page].to_i, 1].max - return unless @candidates + @candidates, @pagination_info = paginate_candidates(@candidates, page, MAX_CANDIDATES_PER_PAGE) - return redirect_to(url_for(page: @candidates.total_pages)) if @candidates.out_of_range? + return if @pagination_info[:total_pages] == 0 - @pagination = { - page: page, - is_last_page: @candidates.last_page?, - per_page: MAX_CANDIDATES_PER_PAGE, - total_items: @candidates.total_count - } + redirect_to(url_for(safe_params.merge(page: @pagination_info[:total_pages]))) if @pagination_info[:out_of_range] @candidates.each(&:artifact_lazy) end diff --git a/app/finders/projects/ml/candidate_finder.rb b/app/finders/projects/ml/candidate_finder.rb new file mode 100644 index 0000000000000..a543abc2c99a7 --- /dev/null +++ b/app/finders/projects/ml/candidate_finder.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +module Projects + module Ml + class CandidateFinder + VALID_ORDER_BY_TYPES = %w[column metric].freeze + VALID_ORDER_BY_COLUMNS = %w[name created_at id].freeze + VALID_SORT = %w[asc desc].freeze + + def initialize(experiment, params = {}) + @experiment = experiment + @params = params + end + + def execute + candidates = @experiment.candidates.including_relationships + + candidates = by_name(candidates) + order(candidates) + end + + private + + def by_name(candidates) + return candidates unless @params[:name].present? + + candidates.by_name(@params[:name]) + end + + def order(candidates) + return candidates.order_by_metric(metric_order_by, sort) if order_by_metric? + + candidates.order_by("#{column_order_by}_#{sort}").with_order_id_desc + end + + def order_by_metric? + order_by_type == 'metric' + end + + def order_by_type + valid_or_default(@params[:order_by_type], VALID_ORDER_BY_TYPES, 'column') + end + + def column_order_by + valid_or_default(@params[:order_by], VALID_ORDER_BY_COLUMNS, 'created_at') + end + + def metric_order_by + @params[:order_by] || '' + end + + def sort + valid_or_default(@params[:sort]&.downcase, VALID_SORT, 'desc') + end + + def valid_or_default(value, valid_values, default) + return value if valid_values.include?(value) + + default + end + end + end +end diff --git a/app/helpers/projects/ml/experiments_helper.rb b/app/helpers/projects/ml/experiments_helper.rb index b9a219b3021e5..afb6f9c99e31c 100644 --- a/app/helpers/projects/ml/experiments_helper.rb +++ b/app/helpers/projects/ml/experiments_helper.rb @@ -42,6 +42,23 @@ def candidate_as_data(candidate) Gitlab::Json.generate(data) end + def paginate_candidates(candidates, page, max_per_page) + return [candidates, nil] unless candidates + + candidates = candidates.page(page).per(max_per_page) + + pagination_info = { + page: page, + is_last_page: candidates.last_page?, + per_page: max_per_page, + total_items: candidates.total_count, + total_pages: candidates.total_pages, + out_of_range: candidates.out_of_range? + } + + [candidates, pagination_info] + end + private def link_to_artifact(candidate) diff --git a/app/models/ml/candidate.rb b/app/models/ml/candidate.rb index 3ea46a8b70376..16e49305fded1 100644 --- a/app/models/ml/candidate.rb +++ b/app/models/ml/candidate.rb @@ -2,6 +2,8 @@ module Ml class Candidate < ApplicationRecord + include Sortable + PACKAGE_PREFIX = 'ml_candidate_' enum status: { running: 0, scheduled: 1, finished: 2, failed: 3, killed: 4 } @@ -19,6 +21,12 @@ class Candidate < ApplicationRecord attribute :iid, default: -> { SecureRandom.uuid } scope :including_relationships, -> { includes(:latest_metrics, :params, :user) } + scope :by_name, ->(name) { where("ml_candidates.name LIKE ?", "%#{sanitize_sql_like(name)}%") } # rubocop:disable GitlabSecurity/SqlInjection + scope :order_by_metric, ->(metric, direction) do + subquery = Ml::CandidateMetric.latest.where(name: metric) + joins("INNER JOIN (#{subquery.to_sql}) latest ON latest.candidate_id = ml_candidates.id") + .order("latest.value #{direction}, ml_candidates.id DESC") + end delegate :project_id, :project, to: :experiment diff --git a/app/views/projects/ml/experiments/show.html.haml b/app/views/projects/ml/experiments/show.html.haml index 143981eebe6ae..c938c804dcff4 100644 --- a/app/views/projects/ml/experiments/show.html.haml +++ b/app/views/projects/ml/experiments/show.html.haml @@ -12,5 +12,5 @@ candidates: items, metrics: metrics, params: params, - pagination: @pagination.to_json + pagination: @pagination_info.to_json } } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 91de67a861135..bc68f42dd9460 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1375,9 +1375,6 @@ msgstr "" msgid ", or " msgstr "" -msgid "-" -msgstr "" - msgid "- %{policy_name} (notifying after %{elapsed_time} minutes unless %{status})" msgstr "" @@ -16626,9 +16623,6 @@ msgstr "" msgid "Experiment" msgstr "" -msgid "Experiment candidates" -msgstr "" - msgid "Experiments" msgstr "" @@ -27032,6 +27026,33 @@ msgstr "" msgid "MissingSSHKeyWarningLink|You won't be able to pull or push repositories via SSH until you add an SSH key to your profile" msgstr "" +msgid "MlExperimentTracking|-" +msgstr "" + +msgid "MlExperimentTracking|Artifacts" +msgstr "" + +msgid "MlExperimentTracking|Created at" +msgstr "" + +msgid "MlExperimentTracking|Details" +msgstr "" + +msgid "MlExperimentTracking|Experiment candidates" +msgstr "" + +msgid "MlExperimentTracking|Filter candidates" +msgstr "" + +msgid "MlExperimentTracking|Name" +msgstr "" + +msgid "MlExperimentTracking|No candidates to display" +msgstr "" + +msgid "MlExperimentTracking|User" +msgstr "" + msgid "MlExperimentsEmptyState|No Experiments to Show" msgstr "" @@ -42891,9 +42912,6 @@ msgstr "" msgid "This epic would exceed maximum number of related epics." msgstr "" -msgid "This experiment has no logged candidates" -msgstr "" - msgid "This feature requires local storage to be enabled" msgstr "" diff --git a/spec/finders/projects/ml/candidate_finder_spec.rb b/spec/finders/projects/ml/candidate_finder_spec.rb new file mode 100644 index 0000000000000..967d563c09046 --- /dev/null +++ b/spec/finders/projects/ml/candidate_finder_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::Ml::CandidateFinder, feature_category: :mlops do + let_it_be(:experiment) { create(:ml_experiments, user: nil) } + + let_it_be(:candidates) do + %w[c a da b].zip([3, 2, 4, 1]).map do |name, auc| + make_candidate_and_metric(name, auc, experiment) + end + end + + let_it_be(:another_candidate) { create(:ml_candidates) } + let_it_be(:first_candidate) { candidates.first } + + let(:finder) { described_class.new(experiment, params) } + let(:page) { 1 } + let(:default_params) { { page: page } } + let(:params) { default_params } + + subject { finder.execute } + + describe '.execute' do + describe 'by name' do + context 'when params has no name' do + it 'fetches all candidates in the experiment' do + expect(subject).to match_array(candidates) + end + + it 'does not fetch candidate not in experiment' do + expect(subject).not_to include(another_candidate) + end + end + + context 'when name is included in params' do + let(:params) { { name: 'a' } } + + it 'fetches the correct candidates' do + expect(subject).to match_array(candidates.values_at(2, 1)) + end + end + end + + describe 'sorting' do + using RSpec::Parameterized::TableSyntax + + where(:test_case, :order_by, :order_by_type, :direction, :expected_order) do + 'default params' | nil | nil | nil | [3, 2, 1, 0] + 'ascending order' | nil | nil | 'ASC' | [0, 1, 2, 3] + 'column is passed' | 'name' | 'column' | 'ASC' | [1, 3, 0, 2] + 'column is a metric' | 'auc' | 'metric' | nil | [2, 0, 1, 3] + 'invalid sort' | nil | nil | 'INVALID' | [3, 2, 1, 0] + 'invalid order by' | 'INVALID' | 'column' | 'desc' | [3, 2, 1, 0] + 'invalid order by metric' | nil | 'metric' | 'desc' | [] + end + with_them do + let(:params) { { order_by: order_by, order_by_type: order_by_type, sort: direction } } + + it { expect(subject).to eq(candidates.values_at(*expected_order)) } + end + end + + context 'when name and sort by metric is passed' do + let(:params) { { order_by: 'auc', order_by_type: 'metric', sort: 'DESC', name: 'a' } } + + it { expect(subject).to eq(candidates.values_at(2, 1)) } + end + end + + private + + def make_candidate_and_metric(name, auc_value, experiment) + create(:ml_candidates, name: name, experiment: experiment, user: nil).tap do |c| + create(:ml_candidate_metrics, name: 'auc', candidate_id: c.id, value: 10) + create(:ml_candidate_metrics, name: 'auc', candidate_id: c.id, value: auc_value) + end + end +end diff --git a/spec/frontend/ml/experiment_tracking/components/__snapshots__/ml_experiment_spec.js.snap b/spec/frontend/ml/experiment_tracking/components/__snapshots__/ml_experiment_spec.js.snap deleted file mode 100644 index 3ee2c1cc075c8..0000000000000 --- a/spec/frontend/ml/experiment_tracking/components/__snapshots__/ml_experiment_spec.js.snap +++ /dev/null @@ -1,761 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`MlExperiment with candidates renders correctly 1`] = ` -<div> - <div - class="gl-alert gl-alert-warning" - > - <svg - aria-hidden="true" - class="gl-icon s16 gl-alert-icon" - data-testid="warning-icon" - role="img" - > - <use - href="#warning" - /> - </svg> - - <div - aria-live="assertive" - class="gl-alert-content" - role="alert" - > - <h2 - class="gl-alert-title" - > - Machine Learning Experiment Tracking is in Incubating Phase - </h2> - - <div - class="gl-alert-body" - > - - GitLab incubates features to explore new use cases. These features are updated regularly, and support is limited - - <a - class="gl-link" - href="https://about.gitlab.com/handbook/engineering/incubation/" - rel="noopener noreferrer" - target="_blank" - > - Learn more - </a> - </div> - - <div - class="gl-alert-actions" - > - <a - class="btn gl-alert-action btn-confirm btn-md gl-button" - href="https://gitlab.com/gitlab-org/gitlab/-/issues/381660" - > - <!----> - - <!----> - - <span - class="gl-button-text" - > - - Feedback - - </span> - </a> - </div> - </div> - - <button - aria-label="Dismiss" - class="btn gl-dismiss-btn btn-default btn-sm gl-button btn-default-tertiary btn-icon" - type="button" - > - <!----> - - <svg - aria-hidden="true" - class="gl-button-icon gl-icon s16" - data-testid="close-icon" - role="img" - > - <use - href="#close" - /> - </svg> - - <!----> - </button> - </div> - - <h3> - - Experiment candidates - - </h3> - - <table - aria-busy="false" - aria-colcount="9" - class="table b-table gl-table gl-mt-0! ml-candidate-table table-sm" - role="table" - > - <!----> - <!----> - <thead - class="" - role="rowgroup" - > - <!----> - <tr - class="" - role="row" - > - <th - aria-colindex="1" - class="" - role="columnheader" - scope="col" - > - <div> - Name - </div> - </th> - <th - aria-colindex="2" - class="" - role="columnheader" - scope="col" - > - <div> - Created at - </div> - </th> - <th - aria-colindex="3" - class="" - role="columnheader" - scope="col" - > - <div> - User - </div> - </th> - <th - aria-colindex="4" - class="" - role="columnheader" - scope="col" - > - <div> - L1 Ratio - </div> - </th> - <th - aria-colindex="5" - class="" - role="columnheader" - scope="col" - > - <div> - Rmse - </div> - </th> - <th - aria-colindex="6" - class="" - role="columnheader" - scope="col" - > - <div> - Auc - </div> - </th> - <th - aria-colindex="7" - class="" - role="columnheader" - scope="col" - > - <div> - Mae - </div> - </th> - <th - aria-colindex="8" - aria-label="Details" - class="" - role="columnheader" - scope="col" - > - <div /> - </th> - <th - aria-colindex="9" - aria-label="Artifact" - class="" - role="columnheader" - scope="col" - > - <div /> - </th> - </tr> - </thead> - <tbody - role="rowgroup" - > - <!----> - <tr - class="" - role="row" - > - <td - aria-colindex="1" - class="" - role="cell" - > - <div - title="aCandidate" - > - aCandidate - </div> - </td> - <td - aria-colindex="2" - class="" - role="cell" - > - <time - class="" - datetime="2023-01-05T14:07:02.975Z" - title="2023-01-05T14:07:02.975Z" - > - in 2 years - </time> - </td> - <td - aria-colindex="3" - class="" - role="cell" - > - <a - class="gl-link" - href="/root" - title="root" - > - @root - </a> - </td> - <td - aria-colindex="4" - class="" - role="cell" - > - <div - title="0.4" - > - 0.4 - </div> - </td> - <td - aria-colindex="5" - class="" - role="cell" - > - <div - title="1" - > - 1 - </div> - </td> - <td - aria-colindex="6" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="7" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="8" - class="" - role="cell" - > - <a - class="gl-link" - href="link_to_candidate1" - title="Details" - > - Details - </a> - </td> - <td - aria-colindex="9" - class="" - role="cell" - > - <a - class="gl-link" - href="link_to_artifact" - rel="noopener" - target="_blank" - title="Artifacts" - > - Artifacts - </a> - </td> - </tr> - <tr - class="" - role="row" - > - <td - aria-colindex="1" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="2" - class="" - role="cell" - > - <time - class="" - datetime="2023-01-05T14:07:02.975Z" - title="2023-01-05T14:07:02.975Z" - > - in 2 years - </time> - </td> - <td - aria-colindex="3" - class="" - role="cell" - > - <div> - - - </div> - </td> - <td - aria-colindex="4" - class="" - role="cell" - > - <div - title="0.5" - > - 0.5 - </div> - </td> - <td - aria-colindex="5" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="6" - class="" - role="cell" - > - <div - title="0.3" - > - 0.3 - </div> - </td> - <td - aria-colindex="7" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="8" - class="" - role="cell" - > - <a - class="gl-link" - href="link_to_candidate2" - title="Details" - > - Details - </a> - </td> - <td - aria-colindex="9" - class="" - role="cell" - > - <div - title="Artifacts" - > - - - - - </div> - </td> - </tr> - <tr - class="" - role="row" - > - <td - aria-colindex="1" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="2" - class="" - role="cell" - > - <time - class="" - datetime="2023-01-05T14:07:02.975Z" - title="2023-01-05T14:07:02.975Z" - > - in 2 years - </time> - </td> - <td - aria-colindex="3" - class="" - role="cell" - > - <div> - - - </div> - </td> - <td - aria-colindex="4" - class="" - role="cell" - > - <div - title="0.5" - > - 0.5 - </div> - </td> - <td - aria-colindex="5" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="6" - class="" - role="cell" - > - <div - title="0.3" - > - 0.3 - </div> - </td> - <td - aria-colindex="7" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="8" - class="" - role="cell" - > - <a - class="gl-link" - href="link_to_candidate3" - title="Details" - > - Details - </a> - </td> - <td - aria-colindex="9" - class="" - role="cell" - > - <div - title="Artifacts" - > - - - - - </div> - </td> - </tr> - <tr - class="" - role="row" - > - <td - aria-colindex="1" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="2" - class="" - role="cell" - > - <time - class="" - datetime="2023-01-05T14:07:02.975Z" - title="2023-01-05T14:07:02.975Z" - > - in 2 years - </time> - </td> - <td - aria-colindex="3" - class="" - role="cell" - > - <div> - - - </div> - </td> - <td - aria-colindex="4" - class="" - role="cell" - > - <div - title="0.5" - > - 0.5 - </div> - </td> - <td - aria-colindex="5" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="6" - class="" - role="cell" - > - <div - title="0.3" - > - 0.3 - </div> - </td> - <td - aria-colindex="7" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="8" - class="" - role="cell" - > - <a - class="gl-link" - href="link_to_candidate4" - title="Details" - > - Details - </a> - </td> - <td - aria-colindex="9" - class="" - role="cell" - > - <div - title="Artifacts" - > - - - - - </div> - </td> - </tr> - <tr - class="" - role="row" - > - <td - aria-colindex="1" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="2" - class="" - role="cell" - > - <time - class="" - datetime="2023-01-05T14:07:02.975Z" - title="2023-01-05T14:07:02.975Z" - > - in 2 years - </time> - </td> - <td - aria-colindex="3" - class="" - role="cell" - > - <div> - - - </div> - </td> - <td - aria-colindex="4" - class="" - role="cell" - > - <div - title="0.5" - > - 0.5 - </div> - </td> - <td - aria-colindex="5" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="6" - class="" - role="cell" - > - <div - title="0.3" - > - 0.3 - </div> - </td> - <td - aria-colindex="7" - class="" - role="cell" - > - <div - title="" - > - - </div> - </td> - <td - aria-colindex="8" - class="" - role="cell" - > - <a - class="gl-link" - href="link_to_candidate5" - title="Details" - > - Details - </a> - </td> - <td - aria-colindex="9" - class="" - role="cell" - > - <div - title="Artifacts" - > - - - - - </div> - </td> - </tr> - <!----> - <!----> - </tbody> - <!----> - </table> - - <!----> -</div> -`; diff --git a/spec/frontend/ml/experiment_tracking/components/ml_experiment_spec.js b/spec/frontend/ml/experiment_tracking/components/ml_experiment_spec.js index abcaf17303fd1..a8637bc35ce0a 100644 --- a/spec/frontend/ml/experiment_tracking/components/ml_experiment_spec.js +++ b/spec/frontend/ml/experiment_tracking/components/ml_experiment_spec.js @@ -1,6 +1,10 @@ -import { GlAlert, GlPagination } from '@gitlab/ui'; +import { GlAlert, GlPagination, GlTable, GlLink } from '@gitlab/ui'; +import { nextTick } from 'vue'; import { mountExtended } from 'helpers/vue_test_utils_helper'; import MlExperiment from '~/ml/experiment_tracking/components/ml_experiment.vue'; +import RegistrySearch from '~/vue_shared/components/registry/registry_search.vue'; +import setWindowLocation from 'helpers/set_window_location_helper'; +import * as urlHelpers from '~/lib/utils/url_utility'; describe('MlExperiment', () => { let wrapper; @@ -11,130 +15,340 @@ describe('MlExperiment', () => { paramNames = [], pagination = { page: 1, isLastPage: false, per_page: 2, totalItems: 0 }, ) => { - return mountExtended(MlExperiment, { + wrapper = mountExtended(MlExperiment, { provide: { candidates, metricNames, paramNames, pagination }, }); }; - const findAlert = () => wrapper.findComponent(GlAlert); + const defaultPagination = { page: 1, isLastPage: false, per_page: 2, totalItems: 5 }; + + const candidates = [ + { + rmse: 1, + l1_ratio: 0.4, + details: 'link_to_candidate1', + artifact: 'link_to_artifact', + name: 'aCandidate', + created_at: '2023-01-05T14:07:02.975Z', + user: { username: 'root', path: '/root' }, + }, + { + auc: 0.3, + l1_ratio: 0.5, + details: 'link_to_candidate2', + created_at: '2023-01-05T14:07:02.975Z', + name: null, + user: null, + }, + { + auc: 0.3, + l1_ratio: 0.5, + details: 'link_to_candidate3', + created_at: '2023-01-05T14:07:02.975Z', + name: null, + user: null, + }, + { + auc: 0.3, + l1_ratio: 0.5, + details: 'link_to_candidate4', + created_at: '2023-01-05T14:07:02.975Z', + name: null, + user: null, + }, + { + auc: 0.3, + l1_ratio: 0.5, + details: 'link_to_candidate5', + created_at: '2023-01-05T14:07:02.975Z', + name: null, + user: null, + }, + ]; + + const createWrapperWithCandidates = (pagination = defaultPagination) => { + createWrapper(candidates, ['rmse', 'auc', 'mae'], ['l1_ratio'], pagination); + }; - const findEmptyState = () => wrapper.findByText('This experiment has no logged candidates'); + const findAlert = () => wrapper.findComponent(GlAlert); + const findPagination = () => wrapper.findComponent(GlPagination); + const findEmptyState = () => wrapper.findByText('No candidates to display'); + const findRegistrySearch = () => wrapper.findComponent(RegistrySearch); + const findTable = () => wrapper.findComponent(GlTable); + const findTableHeaders = () => findTable().findAll('th'); + const findTableRows = () => findTable().findAll('tbody > tr'); + const findNthTableRow = (idx) => findTableRows().at(idx); + const findColumnInRow = (row, col) => findNthTableRow(row).findAll('td').at(col); + const hrefInRowAndColumn = (row, col) => + findColumnInRow(row, col).findComponent(GlLink).attributes().href; it('shows incubation warning', () => { - wrapper = createWrapper(); + createWrapper(); expect(findAlert().exists()).toBe(true); }); - describe('no candidates', () => { - it('shows empty state', () => { - wrapper = createWrapper(); + describe('default inputs', () => { + beforeEach(async () => { + createWrapper(); + await nextTick(); + }); + + it('shows empty state', () => { expect(findEmptyState().exists()).toBe(true); }); it('does not show pagination', () => { - wrapper = createWrapper(); + expect(findPagination().exists()).toBe(false); + }); + + it('there are no columns', () => { + expect(findTable().findAll('th')).toHaveLength(0); + }); - expect(wrapper.findComponent(GlPagination).exists()).toBe(false); + it('initializes sorting correctly', () => { + expect(findRegistrySearch().props('sorting')).toMatchObject({ + orderBy: 'created_at', + sort: 'desc', + }); + }); + + it('initializes filters correctly', () => { + expect(findRegistrySearch().props('filters')).toMatchObject([{ value: { data: '' } }]); }); }); - describe('with candidates', () => { - const defaultPagination = { page: 1, isLastPage: false, per_page: 2, totalItems: 5 }; - - const createWrapperWithCandidates = (pagination = defaultPagination) => { - return createWrapper( - [ - { - rmse: 1, - l1_ratio: 0.4, - details: 'link_to_candidate1', - artifact: 'link_to_artifact', - name: 'aCandidate', - created_at: '2023-01-05T14:07:02.975Z', - user: { username: 'root', path: '/root' }, - }, - { - auc: 0.3, - l1_ratio: 0.5, - details: 'link_to_candidate2', - created_at: '2023-01-05T14:07:02.975Z', - name: null, - user: null, - }, - { - auc: 0.3, - l1_ratio: 0.5, - details: 'link_to_candidate3', - created_at: '2023-01-05T14:07:02.975Z', - name: null, - user: null, - }, - { - auc: 0.3, - l1_ratio: 0.5, - details: 'link_to_candidate4', - created_at: '2023-01-05T14:07:02.975Z', - name: null, - user: null, - }, - { - auc: 0.3, - l1_ratio: 0.5, - details: 'link_to_candidate5', - created_at: '2023-01-05T14:07:02.975Z', - name: null, - user: null, - }, - ], - ['rmse', 'auc', 'mae'], - ['l1_ratio'], - pagination, + describe('generateLink', () => { + it('generates the correct url', () => { + setWindowLocation( + 'https://blah.com/?name=query&orderBy=name&orderByType=column&sort=asc&page=1', + ); + + createWrapperWithCandidates(); + + expect(findPagination().props('linkGen')(2)).toBe( + 'https://blah.com/?name=query&orderBy=name&orderByType=column&sort=asc&page=2', + ); + }); + + it('generates the correct url when no name', () => { + setWindowLocation('https://blah.com/?orderBy=auc&orderByType=metric&sort=asc'); + + createWrapperWithCandidates(); + + expect(findPagination().props('linkGen')(2)).toBe( + 'https://blah.com/?orderBy=auc&orderByType=metric&sort=asc&page=2', ); - }; + }); + }); + + describe('Search', () => { + it('shows search box', () => { + createWrapper(); + + expect(findRegistrySearch().exists()).toBe(true); + }); + + it('metrics are added as options for sorting', () => { + createWrapper([], ['bar']); + + const labels = findRegistrySearch() + .props('sortableFields') + .map((e) => e.orderBy); + expect(labels).toContain('metric.bar'); + }); + + it('sets the component filters based on the querystring', () => { + setWindowLocation('https://blah?name=A&orderBy=B&sort=C'); + + createWrapper(); + + expect(findRegistrySearch().props('filters')).toMatchObject([{ value: { data: 'A' } }]); + }); + + it('sets the component sort based on the querystring', () => { + setWindowLocation('https://blah?name=A&orderBy=B&sort=C'); + + createWrapper(); + + expect(findRegistrySearch().props('sorting')).toMatchObject({ orderBy: 'B', sort: 'c' }); + }); + + it('sets the component sort based on the querystring, when order by is a metric', () => { + setWindowLocation('https://blah?name=A&orderBy=B&sort=C&orderByType=metric'); + + createWrapper(); + + expect(findRegistrySearch().props('sorting')).toMatchObject({ + orderBy: 'metric.B', + sort: 'c', + }); + }); + + describe('Search submit', () => { + beforeEach(() => { + setWindowLocation('https://blah.com/?name=query&orderBy=name&orderByType=column&sort=asc'); + jest.spyOn(urlHelpers, 'visitUrl').mockImplementation(() => {}); + + createWrapper(); + }); - it('renders correctly', () => { - wrapper = createWrapperWithCandidates(); + it('On submit, reloads to correct page', () => { + findRegistrySearch().vm.$emit('filter:submit'); - expect(wrapper.element).toMatchSnapshot(); + expect(urlHelpers.visitUrl).toHaveBeenCalledTimes(1); + expect(urlHelpers.visitUrl).toHaveBeenCalledWith( + 'https://blah.com/?name=query&orderBy=name&orderByType=column&sort=asc&page=1', + ); + }); + + it('On sorting changed, reloads to correct page', () => { + findRegistrySearch().vm.$emit('sorting:changed', { orderBy: 'created_at' }); + + expect(urlHelpers.visitUrl).toHaveBeenCalledTimes(1); + expect(urlHelpers.visitUrl).toHaveBeenCalledWith( + 'https://blah.com/?name=query&orderBy=created_at&orderByType=column&sort=asc&page=1', + ); + }); + + it('On sorting changed and is metric, reloads to correct page', () => { + findRegistrySearch().vm.$emit('sorting:changed', { orderBy: 'metric.auc' }); + + expect(urlHelpers.visitUrl).toHaveBeenCalledTimes(1); + expect(urlHelpers.visitUrl).toHaveBeenCalledWith( + 'https://blah.com/?name=query&orderBy=auc&orderByType=metric&sort=asc&page=1', + ); + }); + + it('On direction changed, reloads to correct page', () => { + findRegistrySearch().vm.$emit('sorting:changed', { sort: 'desc' }); + + expect(urlHelpers.visitUrl).toHaveBeenCalledTimes(1); + expect(urlHelpers.visitUrl).toHaveBeenCalledWith( + 'https://blah.com/?name=query&orderBy=name&orderByType=column&sort=desc&page=1', + ); + }); }); + }); + describe('with candidates', () => { describe('Pagination behaviour', () => { - it('should show', () => { - wrapper = createWrapperWithCandidates(); + beforeEach(() => { + createWrapperWithCandidates(); + }); - expect(wrapper.findComponent(GlPagination).exists()).toBe(true); + it('should show', () => { + expect(findPagination().exists()).toBe(true); }); it('should get the page number from the URL', () => { - wrapper = createWrapperWithCandidates({ ...defaultPagination, page: 2 }); + createWrapperWithCandidates({ ...defaultPagination, page: 2 }); - expect(wrapper.findComponent(GlPagination).props().value).toBe(2); + expect(findPagination().props().value).toBe(2); }); it('should not have a prevPage if the page is 1', () => { - wrapper = createWrapperWithCandidates(); - - expect(wrapper.findComponent(GlPagination).props().prevPage).toBe(null); + expect(findPagination().props().prevPage).toBe(null); }); it('should set the prevPage to 1 if the page is 2', () => { - wrapper = createWrapperWithCandidates({ ...defaultPagination, page: 2 }); + createWrapperWithCandidates({ ...defaultPagination, page: 2 }); - expect(wrapper.findComponent(GlPagination).props().prevPage).toBe(1); + expect(findPagination().props().prevPage).toBe(1); }); it('should not have a nextPage if isLastPage is true', async () => { - wrapper = createWrapperWithCandidates({ ...defaultPagination, isLastPage: true }); + createWrapperWithCandidates({ ...defaultPagination, isLastPage: true }); - expect(wrapper.findComponent(GlPagination).props().nextPage).toBe(null); + expect(findPagination().props().nextPage).toBe(null); }); it('should set the nextPage to 2 if the page is 1', () => { - wrapper = createWrapperWithCandidates(); + expect(findPagination().props().nextPage).toBe(2); + }); + }); + }); + + describe('Candidate table', () => { + const firstCandidateIndex = 0; + const secondCandidateIndex = 1; + const firstCandidate = candidates[firstCandidateIndex]; + + beforeEach(() => { + createWrapperWithCandidates(); + }); + + it('renders all rows', () => { + expect(findTableRows()).toHaveLength(candidates.length); + }); + + it('sets the correct columns in the table', () => { + const expectedColumnNames = [ + 'Name', + 'Created at', + 'User', + 'L1 Ratio', + 'Rmse', + 'Auc', + 'Mae', + '', + '', + ]; + + expect(findTableHeaders().wrappers.map((h) => h.text())).toEqual(expectedColumnNames); + }); + + describe('Artifact column', () => { + const artifactColumnIndex = -1; + + it('shows the a link to the artifact', () => { + expect(hrefInRowAndColumn(firstCandidateIndex, artifactColumnIndex)).toBe( + firstCandidate.artifact, + ); + }); + + it('shows empty state when no artifact', () => { + expect(findColumnInRow(secondCandidateIndex, artifactColumnIndex).text()).toBe('-'); + }); + }); + + describe('User column', () => { + const userColumn = 2; + + it('creates a link to the user', () => { + const column = findColumnInRow(firstCandidateIndex, userColumn).findComponent(GlLink); + + expect(column.attributes().href).toBe(firstCandidate.user.path); + expect(column.text()).toBe(`@${firstCandidate.user.username}`); + }); + + it('when there is no user shows empty state', () => { + createWrapperWithCandidates(); + + expect(findColumnInRow(secondCandidateIndex, userColumn).text()).toBe('-'); + }); + }); + + describe('Candidate name column', () => { + const nameColumnIndex = 0; + + it('Sets the name', () => { + expect(findColumnInRow(firstCandidateIndex, nameColumnIndex).text()).toBe( + firstCandidate.name, + ); + }); + + it('when there is no user shows nothing', () => { + expect(findColumnInRow(secondCandidateIndex, nameColumnIndex).text()).toBe(''); + }); + }); + + describe('Detail column', () => { + const detailColumn = -2; - expect(wrapper.findComponent(GlPagination).props().nextPage).toBe(2); + it('is a link to details', () => { + expect(hrefInRowAndColumn(firstCandidateIndex, detailColumn)).toBe(firstCandidate.details); }); }); }); diff --git a/spec/helpers/projects/ml/experiments_helper_spec.rb b/spec/helpers/projects/ml/experiments_helper_spec.rb index 2b70201456abd..283a3f7323878 100644 --- a/spec/helpers/projects/ml/experiments_helper_spec.rb +++ b/spec/helpers/projects/ml/experiments_helper_spec.rb @@ -109,4 +109,68 @@ expect(subject['info']).to include(expected_info) end end + + describe '#paginate_candidates' do + let(:page) { 1 } + let(:max_per_page) { 1 } + + subject { helper.paginate_candidates(experiment.candidates.order(:id), page, max_per_page) } + + it 'paginates' do + expect(subject[1]).not_to be_nil + end + + it 'only returns max_per_page elements' do + expect(subject[0].size).to eq(1) + end + + it 'fetches the items on the first page' do + expect(subject[0]).to eq([candidate0]) + end + + it 'creates the pagination info' do + expect(subject[1]).to eq({ + page: 1, + is_last_page: false, + per_page: 1, + total_items: 2, + total_pages: 2, + out_of_range: false + }) + end + + context 'when not the first page' do + let(:page) { 2 } + + it 'fetches the right page' do + expect(subject[0]).to eq([candidate1]) + end + + it 'creates the pagination info' do + expect(subject[1]).to eq({ + page: 2, + is_last_page: true, + per_page: 1, + total_items: 2, + total_pages: 2, + out_of_range: false + }) + end + end + + context 'when out of bounds' do + let(:page) { 3 } + + it 'creates the pagination info' do + expect(subject[1]).to eq({ + page: page, + is_last_page: false, + per_page: 1, + total_items: 2, + total_pages: 2, + out_of_range: true + }) + end + end + end end diff --git a/spec/lib/api/entities/ml/mlflow/run_info_spec.rb b/spec/lib/api/entities/ml/mlflow/run_info_spec.rb index db8f106c9fedc..b64a15553321d 100644 --- a/spec/lib/api/entities/ml/mlflow/run_info_spec.rb +++ b/spec/lib/api/entities/ml/mlflow/run_info_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe API::Entities::Ml::Mlflow::RunInfo do +RSpec.describe API::Entities::Ml::Mlflow::RunInfo, feature_category: :mlops do let_it_be(:candidate) { create(:ml_candidates) } subject { described_class.new(candidate, packages_url: 'http://example.com').as_json } diff --git a/spec/models/ml/candidate_spec.rb b/spec/models/ml/candidate_spec.rb index fa8952dc0f446..374e49aea01ed 100644 --- a/spec/models/ml/candidate_spec.rb +++ b/spec/models/ml/candidate_spec.rb @@ -2,9 +2,11 @@ require 'spec_helper' -RSpec.describe Ml::Candidate, factory_default: :keep do - let_it_be(:candidate) { create(:ml_candidates, :with_metrics_and_params) } - let_it_be(:candidate2) { create(:ml_candidates, experiment: candidate.experiment) } +RSpec.describe Ml::Candidate, factory_default: :keep, feature_category: :mlops do + let_it_be(:candidate) { create(:ml_candidates, :with_metrics_and_params, name: 'candidate0') } + let_it_be(:candidate2) do + create(:ml_candidates, experiment: candidate.experiment, user: create(:user), name: 'candidate2') + end let_it_be(:candidate_artifact) do FactoryBot.create(:generic_package, @@ -109,12 +111,12 @@ end describe "#latest_metrics" do - let_it_be(:candidate2) { create(:ml_candidates, experiment: candidate.experiment) } - let!(:metric1) { create(:ml_candidate_metrics, candidate: candidate2) } - let!(:metric2) { create(:ml_candidate_metrics, candidate: candidate2 ) } - let!(:metric3) { create(:ml_candidate_metrics, name: metric1.name, candidate: candidate2) } + let_it_be(:candidate3) { create(:ml_candidates, experiment: candidate.experiment) } + let_it_be(:metric1) { create(:ml_candidate_metrics, candidate: candidate3) } + let_it_be(:metric2) { create(:ml_candidate_metrics, candidate: candidate3 ) } + let_it_be(:metric3) { create(:ml_candidate_metrics, name: metric1.name, candidate: candidate3) } - subject { candidate2.latest_metrics } + subject { candidate3.latest_metrics } it 'fetches only the last metric for the name' do expect(subject).to match_array([metric2, metric3] ) @@ -130,4 +132,55 @@ expect(subject.association_cached?(:user)).to be(true) end end + + describe '#by_name' do + let(:name) { candidate.name } + + subject { described_class.by_name(name) } + + context 'when name matches' do + it 'gets the correct candidates' do + expect(subject).to match_array([candidate]) + end + end + + context 'when name matches partially' do + let(:name) { 'andidate' } + + it 'gets the correct candidates' do + expect(subject).to match_array([candidate, candidate2]) + end + end + + context 'when name does not match' do + let(:name) { non_existing_record_id.to_s } + + it 'does not fetch any candidate' do + expect(subject).to match_array([]) + end + end + end + + describe '#order_by_metric' do + let_it_be(:auc_metrics) do + create(:ml_candidate_metrics, name: 'auc', value: 0.4, candidate: candidate) + create(:ml_candidate_metrics, name: 'auc', value: 0.8, candidate: candidate2) + end + + let(:direction) { 'desc' } + + subject { described_class.order_by_metric('auc', direction) } + + it 'orders correctly' do + expect(subject).to eq([candidate2, candidate]) + end + + context 'when direction is asc' do + let(:direction) { 'asc' } + + it 'orders correctly' do + expect(subject).to eq([candidate, candidate2]) + end + end + end end diff --git a/spec/requests/projects/ml/experiments_controller_spec.rb b/spec/requests/projects/ml/experiments_controller_spec.rb index e8b6f80625194..a1fd07b0a65ea 100644 --- a/spec/requests/projects/ml/experiments_controller_spec.rb +++ b/spec/requests/projects/ml/experiments_controller_spec.rb @@ -98,7 +98,7 @@ let(:params) { basic_params.merge(id: experiment.iid, page: 's') } it 'uses first page' do - expect(assigns(:pagination)).to include( + expect(assigns(:pagination_info)).to include( page: 1, is_last_page: false, per_page: 2, @@ -108,6 +108,32 @@ end end + describe 'search' do + let(:params) do + basic_params.merge( + id: experiment.iid, + name: 'some_name', + orderBy: 'name', + orderByType: 'metric', + sort: 'asc', + invalid: 'invalid' + ) + end + + it 'formats and filters the parameters' do + expect(Projects::Ml::CandidateFinder).to receive(:new).and_call_original do |exp, params| + expect(params.to_h).to include({ + name: 'some_name', + order_by: 'name', + order_by_type: 'metric', + sort: 'asc' + }) + end + + show_experiment + end + end + it 'does not perform N+1 sql queries' do control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { show_experiment } -- GitLab