Skip to content
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.

Sign up for GitHub

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

[Enterprise Search] Disallow creating pipeline with placeholder of model #172988

Merged
merged 5 commits into from
Dec 12, 2023
Merged

[Enterprise Search] Disallow creating pipeline with placeholder of model #172988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
15c3ad7
Set error if model is not downloaded
demjened Dec 8, 2023
7c825c2
Use title model select header instead of form row
demjened Dec 8, 2023
649380d
Add unit tests
demjened Dec 8, 2023
b18b521
Update small label tag to h6
demjened Dec 8, 2023
b4dc074
Merge branch 'main' into demjened/require-downloaded-model
kibanamachine Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions ...rise_search_content/components/search_index/pipelines/ml_inference/configure_pipeline.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
EuiTabbedContentTab,
EuiTitle,
EuiText,
EuiTextColor,
} from '@elastic/eui';

import { i18n } from '@kbn/i18n';
Expand Down Expand Up @@ -130,15 +131,27 @@ export const ConfigurePipeline: React.FC = () => {
<EuiSpacer />
</>
)}
<EuiFormRow
fullWidth
label={i18n.translate(
'xpack.enterpriseSearch.content.indices.pipelines.addInferencePipelineModal.steps.configure.titleSelectTrainedModel',
{ defaultMessage: 'Select a trained ML Model' }
)}
>
<ModelSelect />
</EuiFormRow>
<EuiSpacer size="s" />
<EuiTitle size="xxxs">
demjened marked this conversation as resolved.
Show resolved Hide resolved
<h6>
{i18n.translate(
'xpack.enterpriseSearch.content.indices.pipelines.addInferencePipelineModal.steps.configure.titleSelectTrainedModel',
{ defaultMessage: 'Select a trained ML Model' }
)}
</h6>
</EuiTitle>
{formErrors.modelStatus !== undefined && (
<>
<EuiSpacer size="xs" />
<EuiText size="xs">
<p>
<EuiTextColor color="danger">{formErrors.modelStatus}</EuiTextColor>
</p>
</EuiText>
</>
)}
<EuiSpacer size="xs" />
<ModelSelect />
</EuiForm>
</>
),
Expand Down
19 changes: 19 additions & 0 deletions ..._search_content/components/search_index/pipelines/ml_inference/ml_inference_logic.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,25 @@ describe('MlInferenceLogic', () => {
pipelineName: 'Name already used by another pipeline.',
});
});
it('has errors when non-deployed model is selected', () => {
MLInferenceLogic.actions.setInferencePipelineConfiguration({
...MLInferenceLogic.values.addInferencePipelineModal.configuration,
pipelineName: 'unit-test-pipeline',
modelID: 'unit-test-model',
existingPipeline: false,
fieldMappings: [
{
sourceField: 'body',
targetField: 'ml.inference.body',
},
],
isModelPlaceholderSelected: true,
});

expect(MLInferenceLogic.values.formErrors).toEqual({
modelStatus: 'Model must be deployed before use.',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this sufficient or should we also have a call to action here? (E.g. "Use the Deploy button to deploy the model.")
cc @1Copenut

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@demjened This seems like a good messge. It clearly explains the reason the selected model couldn't be used yet. I'll live test it too, one second.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1Copenut Are you OK going with the current message then?

});
});
});
});

Expand Down
17 changes: 17 additions & 0 deletions ...prise_search_content/components/search_index/pipelines/ml_inference/model_select.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,23 @@ describe('ModelSelect', () => {
})
);
});
it('sets placeholder flag on selecting a placeholder item', () => {
setMockValues(DEFAULT_VALUES);

const wrapper = shallow(<ModelSelect />);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are moving from enzyme to react-testing-library. Is it possible to convert these tests to ones compitible with react-testing-library?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saikatsarkar056 Thanks for the heads up, I just learned about this.
Since this is a critical usability fix that must go into 8.12, are you OK getting this PR merged as-is and deferring the rewriting of tests (possibly with a larger scope) to a tech debt ticket?

expect(wrapper.find(EuiSelectable)).toHaveLength(1);
const selectable = wrapper.find(EuiSelectable);
selectable.simulate('change', [
{ modelId: 'model_1' },
{ modelId: 'model_2', isPlaceholder: true, checked: 'on' },
]);
expect(MOCK_ACTIONS.setInferencePipelineConfiguration).toHaveBeenCalledWith(
expect.objectContaining({
modelID: 'model_2',
isModelPlaceholderSelected: true,
})
);
});
it('generates pipeline name on selecting an item', () => {
setMockValues(DEFAULT_VALUES);

Expand Down
1 change: 1 addition & 0 deletions ...enterprise_search_content/components/search_index/pipelines/ml_inference/model_select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export const ModelSelect: React.FC = () => {
...configuration,
inferenceConfig: undefined,
modelID: selectedOption?.modelId ?? '',
isModelPlaceholderSelected: selectedOption?.isPlaceholder ?? false,
fieldMappings: undefined,
pipelineName: isPipelineNameUserSupplied
? pipelineName
Expand Down
2 changes: 2 additions & 0 deletions ...cations/enterprise_search_content/components/search_index/pipelines/ml_inference/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export interface InferencePipelineConfiguration {
existingPipeline?: boolean;
inferenceConfig?: InferencePipelineInferenceConfig;
isPipelineNameUserSupplied?: boolean;
isModelPlaceholderSelected?: boolean;
modelID: string;
pipelineName: string;
fieldMappings?: FieldMapping[];
Expand All @@ -21,6 +22,7 @@ export interface InferencePipelineConfiguration {

export interface AddInferencePipelineFormErrors {
modelID?: string;
modelStatus?: string;
fieldMappings?: string;
pipelineName?: string;
}
Expand Down
8 changes: 8 additions & 0 deletions ...cations/enterprise_search_content/components/search_index/pipelines/ml_inference/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ const PIPELINE_NAME_EXISTS_ERROR = i18n.translate(
defaultMessage: 'Name already used by another pipeline.',
}
);
const MODEL_NOT_DEPLOYED_ERROR = i18n.translate(
'xpack.enterpriseSearch.content.indices.pipelines.addInferencePipelineModal.steps.configure.modelNotDeployedError',
{
defaultMessage: 'Model must be deployed before use.',
}
);

export const validateInferencePipelineConfiguration = (
config: InferencePipelineConfiguration
Expand All @@ -55,6 +61,8 @@ export const validateInferencePipelineConfiguration = (
}
if (config.modelID.trim().length === 0) {
errors.modelID = FIELD_REQUIRED_ERROR;
} else if (config.isModelPlaceholderSelected) {
errors.modelStatus = MODEL_NOT_DEPLOYED_ERROR;
}

return errors;
Expand Down
Loading