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 preset and enrich data section processors based on version #522

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

kamahuan
Copy link

@kamahuan kamahuan commented Dec 10, 2024

Signed-off-by: Kama Huang kamahuan@amazon.com

Description

Use datasource to get backend version.
If open search backend version is 2.17.0:

  1. filter the presets using ML processors, and keep only 3 presets
  2. in the enrich data section, disable all the ML processors in both ingest and search pipeline

if open search backend version is 2.19.0:

  1. filter all the presets using neural processors, in this case, keep all the presets as they are.
  2. in the enrich data section, keep all the ML processors and disable the neural processors in both ingest and search pipeline.

Upon checking this PR only handle version before 2.17.0 and 2.19.0. 2.18.0 is not handled.

Add processor list change is out of scope of this CR.

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.

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 <kamahuan@amazon.com>
public/plugin.ts Show resolved Hide resolved
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.

Really great start! A few main things I see to be updated.

common/constants.ts Show resolved Hide resolved
Comment on lines -66 to -69
// Current processors state
const [processors, setProcessors] = useState<IProcessorConfig[]>([]);

useEffect(() => {
if (props.uiConfig && props.context) {
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.

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.

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.

3 participants