-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML] Trained models list: disables 'View training data' action if data frame analytics job no longer exists #171061
Changes from 5 commits
a407ee2
7c06abd
6d8a4e5
12f1a01
50ffcfd
8c7094d
3bc2c8d
73216d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,9 +7,10 @@ | |
|
||
import { Action } from '@elastic/eui/src/components/basic_table/action_types'; | ||
import { i18n } from '@kbn/i18n'; | ||
import { FormattedMessage } from '@kbn/i18n-react'; | ||
import { isPopulatedObject } from '@kbn/ml-is-populated-object'; | ||
import { EuiToolTip } from '@elastic/eui'; | ||
import React, { useCallback, useMemo, useEffect, useState } from 'react'; | ||
import { EuiIcon, EuiLink, EuiText, EuiToolTip } from '@elastic/eui'; | ||
import React, { FC, useCallback, useMemo, useEffect, useState } from 'react'; | ||
import { | ||
BUILT_IN_MODEL_TAG, | ||
DEPLOYMENT_STATE, | ||
|
@@ -33,6 +34,80 @@ import { isTestable, isDfaTrainedModel } from './test_models'; | |
import { ModelItem } from './models_list'; | ||
import { usePermissionCheck } from '../capabilities/check_capabilities'; | ||
|
||
const ViewTrainingDataAction: FC<{ | ||
item: ModelItem; | ||
}> = ({ item }) => { | ||
const urlLocator = useMlLocator()!; | ||
const { | ||
services: { | ||
application: { navigateToUrl }, | ||
}, | ||
} = useMlKibana(); | ||
|
||
const handleClick = async () => { | ||
if (item.metadata?.analytics_config === undefined) return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Could this be a check probably with a type guard so we can avoid the |
||
|
||
const analysisType = getAnalysisType( | ||
item.metadata?.analytics_config.analysis | ||
) as DataFrameAnalysisConfigType; | ||
|
||
const url = await urlLocator.getUrl({ | ||
page: ML_PAGES.DATA_FRAME_ANALYTICS_EXPLORATION, | ||
pageState: { | ||
jobId: item.metadata?.analytics_config.id as string, | ||
analysisType, | ||
...(analysisType === 'classification' || analysisType === 'regression' | ||
? { | ||
queryText: `${item.metadata?.analytics_config.dest.results_field}.is_training : true`, | ||
} | ||
: {}), | ||
}, | ||
}); | ||
|
||
await navigateToUrl(url); | ||
}; | ||
|
||
const buttonContent = ( | ||
peteharverson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<> | ||
<EuiIcon type={'visTable'} css={{ marginRight: '8px' }} /> | ||
{i18n.translate('xpack.ml.trainedModels.modelsList.viewTrainingDataActionLabel', { | ||
defaultMessage: 'View training data', | ||
})} | ||
</> | ||
); | ||
|
||
const isEnabled = item.origin_job_exists; | ||
const button = isEnabled ? ( | ||
<EuiLink onClick={handleClick} color={'text'}> | ||
<EuiText size="s" color={'text'}> | ||
{buttonContent} | ||
</EuiText> | ||
</EuiLink> | ||
) : ( | ||
<EuiText size="s" color={'subdued'} css={{ fontWeight: 300 }}> | ||
{buttonContent} | ||
</EuiText> | ||
); | ||
|
||
if (isEnabled) { | ||
return button; | ||
} else { | ||
return ( | ||
<EuiToolTip | ||
position="top" | ||
content={ | ||
<FormattedMessage | ||
id="xpack.ml.trainedModels.modelsList.viewTrainingDataActionTooltip" | ||
defaultMessage="Related data frame analytics job was not found." | ||
/> | ||
} | ||
> | ||
{button} | ||
</EuiToolTip> | ||
); | ||
} | ||
}; | ||
|
||
export function useModelActions({ | ||
onDfaTestAction, | ||
onTestAction, | ||
|
@@ -54,7 +129,6 @@ export function useModelActions({ | |
}): Array<Action<ModelItem>> { | ||
const { | ||
services: { | ||
application: { navigateToUrl }, | ||
overlays, | ||
theme, | ||
i18n: i18nStart, | ||
|
@@ -130,40 +204,11 @@ export function useModelActions({ | |
return useMemo( | ||
() => [ | ||
{ | ||
name: i18n.translate('xpack.ml.trainedModels.modelsList.viewTrainingDataActionLabel', { | ||
defaultMessage: 'View training data', | ||
}), | ||
description: i18n.translate( | ||
'xpack.ml.trainedModels.modelsList.viewTrainingDataActionLabel', | ||
{ | ||
defaultMessage: 'View training data', | ||
} | ||
), | ||
icon: 'visTable', | ||
type: 'icon', | ||
available: (item) => !!item.metadata?.analytics_config?.id, | ||
onClick: async (item) => { | ||
if (item.metadata?.analytics_config === undefined) return; | ||
|
||
const analysisType = getAnalysisType( | ||
item.metadata?.analytics_config.analysis | ||
) as DataFrameAnalysisConfigType; | ||
|
||
const url = await urlLocator.getUrl({ | ||
page: ML_PAGES.DATA_FRAME_ANALYTICS_EXPLORATION, | ||
pageState: { | ||
jobId: item.metadata?.analytics_config.id as string, | ||
analysisType, | ||
...(analysisType === 'classification' || analysisType === 'regression' | ||
? { | ||
queryText: `${item.metadata?.analytics_config.dest.results_field}.is_training : true`, | ||
} | ||
: {}), | ||
}, | ||
}); | ||
|
||
await navigateToUrl(url); | ||
render: (item) => { | ||
return <ViewTrainingDataAction item={item} />; | ||
}, | ||
available: (item) => !!item.metadata?.analytics_config?.id, | ||
enabled: (item) => item.origin_job_exists === true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a tooltip to show why the action is disabled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 6d8a4e5 |
||
isPrimary: true, | ||
}, | ||
{ | ||
|
@@ -606,7 +651,6 @@ export function useModelActions({ | |
isLoading, | ||
modelAndDeploymentIds, | ||
navigateToPath, | ||
navigateToUrl, | ||
onDfaTestAction, | ||
onLoading, | ||
onModelDeployRequest, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,9 +29,10 @@ import { | |
createIngestPipelineSchema, | ||
modelDownloadsQuery, | ||
} from './schemas/inference_schema'; | ||
import type { | ||
import { | ||
isTrainedModelConfigResponse, | ||
PipelineDefinition, | ||
TrainedModelConfigResponse, | ||
type TrainedModelConfigResponse, | ||
} from '../../common/types/trained_models'; | ||
import { mlLog } from '../lib/log'; | ||
import { forceQuerySchema } from './schemas/anomaly_detectors_schema'; | ||
|
@@ -191,10 +192,50 @@ export function trainedModelsRoutes( | |
mlLog.debug(e); | ||
} | ||
|
||
const body = filterForEnabledFeatureModels(result, getEnabledFeatures()); | ||
const filteredModels = filterForEnabledFeatureModels(result, getEnabledFeatures()); | ||
|
||
try { | ||
// @ts-ignore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The types can be fixed by updating export function filterForEnabledFeatureModels<
T extends TrainedModelConfigResponse | estypes.MlTrainedModelConfig
>(models: T[], enabledFeatures: MlFeatures) {
... This also means you don't need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep - good catch - updated in 8c7094d |
||
const jobIdsString = filteredModels.reduce( | ||
( | ||
jobIdsStr: string, | ||
currentModel: TrainedModelConfigResponse | estypes.MlTrainedModelConfig, | ||
idx: number | ||
) => { | ||
if (isTrainedModelConfigResponse(currentModel)) { | ||
let id = currentModel.metadata?.analytics_config?.id ?? ''; | ||
if (id !== '') { | ||
id = `${idx > 0 ? ',' : ''}${id}*`; | ||
} | ||
return `${jobIdsStr}${id}`; | ||
} | ||
return jobIdsStr; | ||
}, | ||
'' | ||
); | ||
|
||
if (jobIdsString !== '') { | ||
const { data_frame_analytics: jobs } = await mlClient.getDataFrameAnalytics({ | ||
jgowdyelastic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
id: jobIdsString, | ||
allow_no_match: true, | ||
}); | ||
|
||
filteredModels.forEach((model) => { | ||
if (isTrainedModelConfigResponse(model)) { | ||
const dfaId = model?.metadata?.analytics_config?.id; | ||
if (dfaId !== undefined) { | ||
// if this is a dfa model, set origin_job_exists | ||
model.origin_job_exists = jobs.find((job) => job.id === dfaId) !== undefined; | ||
} | ||
} | ||
}); | ||
} | ||
} catch (e) { | ||
// Swallow error to prevent blocking trained models result | ||
jgowdyelastic marked this conversation as resolved.
Show resolved
Hide resolved
peteharverson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
return response.ok({ | ||
body, | ||
body: filteredModels, | ||
}); | ||
} catch (e) { | ||
return response.customError(wrapError(e)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs to be optional route param. We should always perform this check when retrieving the models as it's useful information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 7c06abd