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.

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 processors for the add processor section #524

Closed

Conversation

kamahuan
Copy link

Description

Filter processors for the add processor list.

Upon discussion, not matter which version we use for the backend, the search pipeline returns an empty list for now.

For the ingest pipeline, if the backend version is 2.17.0, disable the ML processor and enable neural processors. if backend version is 2.19.0, enable the ML processor and disable neural processors.

I use text_embedding_ingest processor and text_image_embedding_ingest_processors as placeholders until more details are acknowledged.

Issues Resolved

NONE

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Kama Huang added 2 commits December 9, 2024 23:43
and change enrich data section processors based on the version.

Add processor list change is out of scope of this CR.
Signed-off-by: Kama Huang <[email protected]>
Copy link
Member

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

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

This PR seems to include a lot of duplicate changes as the ongoing PR #522. Suggest to start with addressing comments in that PR, and I think consequentially many of the changes made here can be ignored. Essentially the processors filtering can be scoped to just the reusable ProcessorsList component, and the EnrichSearchRequest/EnrichSearchResponse/EnrichData components can be left alone, as these are just pass-through components displaying the existing configured processors and do not need any filtering done on them.

Comment on lines +134 to +135
TEXT_EMBEDDING = 'text_embedding-processor',
TEXT_IMAGE_EMBEDDING = 'text_image_embedding-processor',
Copy link
Member

Choose a reason for hiding this comment

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

@@ -1,25 +1,17 @@
{
"id": "flowFrameworkDashboards",
"version": "2.19.0.0",
"opensearchDashboardsVersion": "2.19.0",
"opensearchDashboardsVersion": "2.18.0",
Copy link
Member

Choose a reason for hiding this comment

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

Should leave as 2.19

"navigation",
"opensearchDashboardsUtils"
],
"requiredPlugins": ["navigation", "opensearchDashboardsUtils"],
"optionalPlugins": [
"dataSource",
"dataSourceManagement",
"contentManagement"
],
"supportedOSDataSourceVersions": ">=2.18.0 <4.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

similar to comment in prev PR, let's change this to >=2.17.0 <4.0.0

"navigation",
"opensearchDashboardsUtils"
],
"requiredPlugins": ["navigation", "opensearchDashboardsUtils"],
"optionalPlugins": [
"dataSource",
"dataSourceManagement",
"contentManagement"
],
"supportedOSDataSourceVersions": ">=2.18.0 <4.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

similar to comment in prev PR, let's change this to >=2.17.0 <4.0.0

Comment on lines -55 to -66

// Processor added state. Used to automatically open accordion when a new
// 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
Copy link
Member

Choose a reason for hiding this comment

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

prefer to leave these comments

Comment on lines +24 to +30
const [filteredCount, setFilteredCount] = useState(0);

return (
<EuiFlexGroup direction="column">
<ProcessorsTitle
title="Enhance query request"
processorCount={
props.uiConfig.search.enrichRequest.processors?.length || 0
}
processorCount={filteredCount}
Copy link
Member

Choose a reason for hiding this comment

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

Similar comments as #522; we should not have to make any changes in these files. We do not want to filter on the configured processors, only the available ones users can select to add & configure.

}

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

Choose a reason for hiding this comment

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

same comment as previous.

// styling
import '../workspace/workspace-styles.scss';
// import { getDataSourceEnabled } from '../../../services';
Copy link
Member

Choose a reason for hiding this comment

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

leftover comment

@@ -38,6 +38,7 @@ const mockDispatch = jest.fn();
const renderWithRouter = () =>
render(
<Provider store={store}>
{/* <Router initialEntries={['/test?dataSourceId=123']}> */}
Copy link
Member

Choose a reason for hiding this comment

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

leftover comment

Copy link
Member

Choose a reason for hiding this comment

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

seems this is duplicate from #522, refer to any comments on that PR.

@ohltyler
Copy link
Member

Closed per #537

@ohltyler ohltyler closed this Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants