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

filter preset and enrich data section processors based on version #522

Closed
wants to merge 1 commit into from
Closed

filter preset and enrich data section processors based on version #522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions common/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ export enum PROCESSOR_TYPE {
NORMALIZATION = 'normalization-processor',
COLLAPSE = 'collapse',
RERANK = 'rerank',
TEXT_EMBEDDING = 'text_embedding-processor',
TEXT_IMAGE_EMBEDDING = 'text_image_embedding-processor',
kamahuan marked this conversation as resolved.
Show resolved Hide resolved
}

export enum MODEL_TYPE {
Expand Down
4 changes: 3 additions & 1 deletion public/pages/workflow_detail/workflow_detail.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,9 @@ export function WorkflowDetail(props: WorkflowDetailProps) {
}, [USE_NEW_HOME_PAGE, dataSourceEnabled, dataSourceId, workflowName]);

// form state
const [formValues, setFormValues] = useState<WorkflowFormValues>({});
const [formValues, setFormValues] = useState<WorkflowFormValues>(
{} as WorkflowFormValues
);
const [formSchema, setFormSchema] = useState<WorkflowSchema>(yup.object({}));

// ingest docs state. we need to persist here to update the form values.
Expand Down
14 changes: 9 additions & 5 deletions public/pages/workflow_detail/workflow_inputs/ingest_inputs/enrich_data.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,32 +3,36 @@
* SPDX-License-Identifier: Apache-2.0
*/

import React from 'react';
import React, { useState } from 'react';
import { EuiFlexGroup, EuiFlexItem } from '@elastic/eui';
import { ProcessorsList } from '../processors_list';
import { PROCESSOR_CONTEXT, WorkflowConfig } from '../../../../../common';
import { ProcessorsTitle } from '../../../../general_components';
import { SavedObject } from '../../../../../../../src/core/public';
import { DataSourceAttributes } from '../../../../../../../src/plugins/data_source/common/data_sources';

interface EnrichDataProps {
uiConfig: WorkflowConfig;
setUiConfig: (uiConfig: WorkflowConfig) => void;
dataSource?: SavedObject<DataSourceAttributes>;
}

/**
* Base component for configuring any data enrichment
*/
export function EnrichData(props: EnrichDataProps) {
const [filteredCount, setFilteredCount] = useState(0);

return (
<EuiFlexGroup direction="column">
<ProcessorsTitle
title="Enrich data"
processorCount={props.uiConfig.ingest.enrich.processors?.length || 0}
/>
<ProcessorsTitle title="Enrich data" processorCount={filteredCount} />
<EuiFlexItem>
<ProcessorsList
uiConfig={props.uiConfig}
setUiConfig={props.setUiConfig}
context={PROCESSOR_CONTEXT.INGEST}
dataSource={props.dataSource}
onProcessorsChange={setFilteredCount}
/>
</EuiFlexItem>
</EuiFlexGroup>
Expand Down
58 changes: 46 additions & 12 deletions public/pages/workflow_detail/workflow_inputs/processors_list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
*/

import React, { useEffect, useState } from 'react';
import semver from 'semver';
import { getEffectiveVersion } from '../../../pages/workflows/new_workflow/new_workflow';
import {
EuiSmallButtonEmpty,
EuiSmallButtonIcon,
Expand All @@ -20,6 +22,7 @@ import { useFormikContext } from 'formik';
import {
IProcessorConfig,
PROCESSOR_CONTEXT,
PROCESSOR_TYPE,
WorkflowConfig,
WorkflowFormValues,
} from '../../../../common';
Expand All @@ -38,11 +41,15 @@ import {
TextChunkingIngestProcessor,
} from '../../../configs';
import { ProcessorInputs } from './processor_inputs';
import { SavedObject } from '../../../../../../src/core/public';
import { DataSourceAttributes } from '../../../../../../src/plugins/data_source/common/data_sources';

interface ProcessorsListProps {
uiConfig: WorkflowConfig;
setUiConfig: (uiConfig: WorkflowConfig) => void;
context: PROCESSOR_CONTEXT;
dataSource?: SavedObject<DataSourceAttributes>;
onProcessorsChange?: (count: number) => void;
}

const PANEL_ID = 0;
Expand All @@ -57,25 +64,52 @@ export function ProcessorsList(props: ProcessorsListProps) {
// processor is added, assuming users want to immediately configure it.
const [processorAdded, setProcessorAdded] = useState<boolean>(false);

// Popover state when adding new processors
const [isPopoverOpen, setPopover] = useState(false);
const closePopover = () => {
setPopover(false);
};

// Current processors state
const [processors, setProcessors] = useState<IProcessorConfig[]>([]);

useEffect(() => {
if (props.uiConfig && props.context) {
Comment on lines -66 to -69
Copy link
Member

Choose a reason for hiding this comment

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

The processors state here represents the individual workflow config's configured processors, not the list of available processors, so we shouldn't change this existing logic or this custom hook.

In general, I don't disagree with the filtering logic, but it should live somewhere else. Note the context menu on lines 247-324 is where the available processors list exists; here is where we should have the determined final list of processors based on the version shown.

Copy link
Author

Choose a reason for hiding this comment

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

Hey Tyler, the filtering logic is updated in my second PR. Can we resolve every other comments in the PR and then proceed to the second PR to fix this?

Copy link
Member

Choose a reason for hiding this comment

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

The processor filtering should only be needed in this component, and not in the other enrich data sections. I believe most of the second PR won't be needed once the changes are made in this one.

setProcessors(
props.context === PROCESSOR_CONTEXT.INGEST
? props.uiConfig.ingest.enrich.processors
: props.context === PROCESSOR_CONTEXT.SEARCH_REQUEST
? props.uiConfig.search.enrichRequest.processors
: props.uiConfig.search.enrichResponse.processors
);
}
}, [props.context, props.uiConfig]);
const loadProcessors = async () => {
if (props.uiConfig && props.context) {
let currentProcessors =
props.context === PROCESSOR_CONTEXT.INGEST
? props.uiConfig.ingest.enrich.processors
: props.context === PROCESSOR_CONTEXT.SEARCH_REQUEST
? props.uiConfig.search.enrichRequest.processors
: props.uiConfig.search.enrichResponse.processors;

if (currentProcessors && props.dataSource?.id) {
const version = await getEffectiveVersion(props.dataSource.id);

if (semver.eq(version, '2.17.0')) {
currentProcessors = currentProcessors.filter(
(processor) =>
!processor.name.toLowerCase().includes('ml inference')
kamahuan marked this conversation as resolved.
Show resolved Hide resolved
);
} else if (semver.gte(version, '2.19.0')) {
currentProcessors = currentProcessors.filter(
(processor) =>
![
PROCESSOR_TYPE.TEXT_EMBEDDING,
PROCESSOR_TYPE.TEXT_IMAGE_EMBEDDING,
PROCESSOR_TYPE.NORMALIZATION,
kamahuan marked this conversation as resolved.
Show resolved Hide resolved
].includes(processor.type)
);
}
}

setProcessors(currentProcessors);
if (props.onProcessorsChange) {
props.onProcessorsChange(currentProcessors?.length || 0);
}
}
};

loadProcessors();
}, [props.context, props.uiConfig, props.dataSource]);

// Adding a processor to the config. Fetch the existing one
// (getting any updated/interim values along the way) and add to
Expand Down
13 changes: 9 additions & 4 deletions public/pages/workflow_detail/workflow_inputs/search_inputs/enrich_search_request.tsx
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

related to previous comments, but we shouldn't need any such idea of a filtered count. The components can just display what exists in the workflow's ui config as before.

Original file line number Diff line number Diff line change
Expand Up @@ -3,34 +3,39 @@
* SPDX-License-Identifier: Apache-2.0
*/

import React from 'react';
import React, { useState } from 'react';
import { EuiFlexGroup, EuiFlexItem } from '@elastic/eui';
import { ProcessorsTitle } from '../../../../general_components';
import { PROCESSOR_CONTEXT, WorkflowConfig } from '../../../../../common';
import { ProcessorsList } from '../processors_list';
import { SavedObject } from '../../../../../../../src/core/public';
import { DataSourceAttributes } from '../../../../../../../src/plugins/data_source/common/data_sources';

interface EnrichSearchRequestProps {
uiConfig: WorkflowConfig;
setUiConfig: (uiConfig: WorkflowConfig) => void;
dataSource?: SavedObject<DataSourceAttributes>;
}

/**
* Input component for enriching a search request (configuring search request processors, etc.)
*/
export function EnrichSearchRequest(props: EnrichSearchRequestProps) {
const [filteredCount, setFilteredCount] = useState(0);

return (
<EuiFlexGroup direction="column">
<ProcessorsTitle
title="Enhance query request"
processorCount={
props.uiConfig.search.enrichRequest.processors?.length || 0
}
processorCount={filteredCount}
/>
<EuiFlexItem>
<ProcessorsList
uiConfig={props.uiConfig}
setUiConfig={props.setUiConfig}
context={PROCESSOR_CONTEXT.SEARCH_REQUEST}
dataSource={props.dataSource}
onProcessorsChange={setFilteredCount}
/>
</EuiFlexItem>
</EuiFlexGroup>
Expand Down
13 changes: 9 additions & 4 deletions public/pages/workflow_detail/workflow_inputs/search_inputs/enrich_search_response.tsx
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

same as above comment.

Original file line number Diff line number Diff line change
Expand Up @@ -3,34 +3,39 @@
* SPDX-License-Identifier: Apache-2.0
*/

import React from 'react';
import React, { useState } from 'react';
import { EuiFlexGroup, EuiFlexItem } from '@elastic/eui';
import { ProcessorsTitle } from '../../../../general_components';
import { PROCESSOR_CONTEXT, WorkflowConfig } from '../../../../../common';
import { ProcessorsList } from '../processors_list';
import { SavedObject } from '../../../../../../../src/core/public';
import { DataSourceAttributes } from '../../../../../../../src/plugins/data_source/common/data_sources';

interface EnrichSearchResponseProps {
uiConfig: WorkflowConfig;
setUiConfig: (uiConfig: WorkflowConfig) => void;
dataSource?: SavedObject<DataSourceAttributes>;
}

/**
* Input component for enriching a search response (configuring search response processors, etc.)
*/
export function EnrichSearchResponse(props: EnrichSearchResponseProps) {
const [filteredCount, setFilteredCount] = useState(0);

return (
<EuiFlexGroup direction="column">
<ProcessorsTitle
title="Enhance query results"
processorCount={
props.uiConfig.search.enrichResponse.processors?.length || 0
}
processorCount={filteredCount}
/>
<EuiFlexItem>
<ProcessorsList
uiConfig={props.uiConfig}
setUiConfig={props.setUiConfig}
context={PROCESSOR_CONTEXT.SEARCH_RESPONSE}
dataSource={props.dataSource}
onProcessorsChange={setFilteredCount}
/>
</EuiFlexItem>
</EuiFlexGroup>
Expand Down
3 changes: 1 addition & 2 deletions public/pages/workflow_detail/workflow_inputs/workflow_inputs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ import {
getDataSourceId,
} from '../../../utils';
import { BooleanField } from './input_fields';

// styling
import '../workspace/workspace-styles.scss';
// import { getDataSourceEnabled } from '../../../services';
kamahuan marked this conversation as resolved.
Show resolved Hide resolved

interface WorkflowInputsProps {
workflow: Workflow | undefined;
Expand Down Expand Up @@ -443,7 +443,6 @@ export function WorkflowInputs(props: WorkflowInputsProps) {
};
if (Object.keys(relevantValidationResults).length > 0) {
getCore().notifications.toasts.addDanger('Missing or invalid fields');
console.error('Form invalid');
kamahuan marked this conversation as resolved.
Show resolved Hide resolved
} else {
setTouched({});
const updatedConfig = formikToUiConfig(
Expand Down
42 changes: 31 additions & 11 deletions public/pages/workflows/new_workflow/new_workflow.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const mockDispatch = jest.fn();
const renderWithRouter = () =>
render(
<Provider store={store}>
{/* <Router initialEntries={['/test?dataSourceId=123']}> */}
<Router>
<Switch>
<Route render={() => <NewWorkflow />} />
Expand All @@ -52,13 +53,27 @@ describe('NewWorkflow', () => {
jest.spyOn(ReactReduxHooks, 'useAppDispatch').mockReturnValue(mockDispatch);
});

test('renders the preset workflow names & descriptions', () => {
test('renders the preset workflow names & descriptions', async () => {
const presetWorkflows = loadPresetWorkflowTemplates();
const { getByPlaceholderText, getAllByText } = renderWithRouter();
const { getByPlaceholderText, getByText } = renderWithRouter();

// Check search box is rendered
expect(getByPlaceholderText('Search')).toBeInTheDocument();
presetWorkflows.forEach((workflow) => {
expect(getAllByText(workflow.name)).toHaveLength(1);
expect(getAllByText(workflow.description)).toHaveLength(1);

// Wait for workflows to be filtered and rendered
await waitFor(() => {
presetWorkflows.forEach((workflow) => {
// Use individual expects for better error reporting
if (
workflow.name ===
['Semantic Search', 'Multimodal Search', 'Hybrid Search'].includes(
workflow.name
)
) {
expect(getByText(workflow.name)).toBeInTheDocument();
expect(getByText(workflow.description)).toBeInTheDocument();
}
});
});
});

Expand All @@ -70,23 +85,28 @@ describe('NewWorkflow', () => {
queryByText,
} = renderWithRouter();

// Click the first "Go" button on the templates and test Quick Configure.
const goButtons = getAllByTestId('goButton');
userEvent.click(goButtons[0]);
// Wait for initial render to complete
await waitFor(() => {
const goButtons = getAllByTestId('goButton');
expect(goButtons.length).toBeGreaterThan(0);
userEvent.click(goButtons[0]);
});

// Wait for Quick Configure to appear
await waitFor(() => {
expect(getAllByText('Quick configure')).toHaveLength(1);
});

// Verify that the create button is present in the Quick Configure pop-up.
// Verify create button
expect(getByTestId('quickConfigureCreateButton')).toBeInTheDocument();

// Click the "Cancel" button in the Quick Configure pop-up.
// Click cancel
const quickConfigureCancelButton = getByTestId(
'quickConfigureCancelButton'
);
userEvent.click(quickConfigureCancelButton);

// Ensure the quick configure pop-up is closed after canceling.
// Wait for modal to close
await waitFor(() => {
expect(queryByText('quickConfigureCreateButton')).toBeNull();
});
Expand Down
Loading