Skip to content

Commit

Permalink
[Enterprise Search] Disallow creating pipeline with placeholder of mo…
Browse files Browse the repository at this point in the history
…del (elastic#172988)

## Summary

This PR adds validation to model selection when creating an inference
pipeline. Pipelines cannot be created with a non-deployed placeholder of
a model (e.g. E5, ELSER) as this would break other Search UIs and
functionality. If such an item is selected, an error message is
displayed and the Continue button is disabled.

Note this feature is independent of the changing of the model selection
component.

Valid selection:
<img width="919" alt="Screenshot 2023-12-08 at 14 03 59"
src="https://github.com/elastic/kibana/assets/14224983/26f8be24-3f78-4765-9a4c-771adbb221a4">

Invalid selection:
<img width="921" alt="Screenshot 2023-12-08 at 14 04 04"
src="https://github.com/elastic/kibana/assets/14224983/a157fb69-c004-49b2-93c7-13b58e82074c">

### Checklist
- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
  • Loading branch information
demjened and kibanamachine committed Dec 12, 2023
1 parent 9502512 commit 156c8f2
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 9 deletions.
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">
<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
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.',
});
});
});
});

Expand Down
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 />);
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
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
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
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

0 comments on commit 156c8f2

Please sign in to comment.