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 presets and processors based on backend version #537

Merged
merged 9 commits into from
Dec 31, 2024

Conversation

kamahuan
Copy link

Filter presets and processors based on backend version

Description

This PR consists of 3 parts:

  1. Filter presets based on backend version. If backend version is 2.17 or 2.18, show presets which not use ML processors. Keep all the presets for version 2.19.
  2. Remove default ML processor for the enrich data section for both ingest and search pipeline if version is 2.17 or 2.18.
  3. Add ML processor and remove neural processors for version 2.19. Remove ML processors and keep neural processors for version 2.17 and 2.18.

If no datasource is configured during the first render, the create work flow page will have a loading state.

page-loading

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.

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.

Getting close! Few comments.

Only remaining piece I think we can add is integrating with the quick-configure fields for the newly-added presets. This will help set defaults for the text embedding & text/image embedding processors, such as the model IDs, fields, etc.

You can look at the quick configure components here for some reference: https://github.com/opensearch-project/dashboards-flow-framework/tree/main/public/pages/workflows/new_workflow

I wouldn't consider a blocker for this PR though. I'll follow up by pulling these changes and testing locally for another sanity test as well, once you've addressed comments. Thanks!

common/constants.ts Outdated Show resolved Hide resolved
common/constants.ts Outdated Show resolved Hide resolved
common/interfaces.ts Outdated Show resolved Hide resolved
opensearch_dashboards.json Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/new_workflow.tsx Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/new_workflow.tsx Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/new_workflow.tsx Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/new_workflow.tsx Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/new_workflow.tsx Outdated Show resolved Hide resolved
@ohltyler
Copy link
Member

@kamahuan Btw, I'm ok to close the previous PRs, quickly went through comments, nothing else to carry over.

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.

Few remaining things, getting closer. Some more items from previous review I've unresolved as well, please have a look.

In general, it is much easier from my end to add comments / details into the changes made for each comment instead of just resolving, otherwise I have to go digging for each individually. please consider for the future :)

common/constants.ts Outdated Show resolved Hide resolved
opensearch_dashboards.json Outdated Show resolved Hide resolved
@ohltyler
Copy link
Member

Cloned locally, and have a 2.19 cluster running. New workflow page is stuck in loading state. No network failures I'm seeing

Screenshot 2024-12-20 at 10 31 40 AM

@kamahuan
Copy link
Author

Cloned locally, and have a 2.19 cluster running. New workflow page is stuck in loading state. No network failures I'm seeing

Screenshot 2024-12-20 at 10 31 40 AM

Hey Tyler, I think the loading state might due to that we do not have any data source configured. Once we configured a datasource, it will show the presets. The loading state is added to indicate when the user first render the page and not data source is configured.

@ohltyler
Copy link
Member

Hey Tyler, I think the loading state might due to that we do not have any data source configured. Once we configured a datasource, it will show the presets. The loading state is added to indicate when the user first render the page and not data source is configured.

There should be a check if multiple data source (MDS) is disabled, which is the default / existing behavior, and if so, not do such checks. If it is disabled, we can assume it is 2.19+.

@kamahuan
Copy link
Author

Hey Tyler, I think the loading state might due to that we do not have any data source configured. Once we configured a datasource, it will show the presets. The loading state is added to indicate when the user first render the page and not data source is configured.

There should be a check if multiple data source (MDS) is disabled, which is the default / existing behavior, and if so, not do such checks. If it is disabled, we can assume it is 2.19+.

Hey Tyler, I think the loading state might due to that we do not have any data source configured. Once we configured a datasource, it will show the presets. The loading state is added to indicate when the user first render the page and not data source is configured.

There should be a check if multiple data source (MDS) is disabled, which is the default / existing behavior, and if so, not do such checks. If it is disabled, we can assume it is 2.19+.

Gotcha, will add this check

@kamahuan
Copy link
Author

Cloned locally, and have a 2.19 cluster running. New workflow page is stuck in loading state. No network failures I'm seeing

Hey Tyler, I add logic to filter when MDS are disabled. These are the screenshots showing:

  1. MDS disabled and version set default to 2.19. Hence all presets should show
  2. MDS not disabled and for version217, it should show a loading state when no data source is configured, and show 3 presets when data source is configured.
  3. MDS not disabled and for version218, it should show a loading state when no data source is configured, and show 3 presets when data source is configured.
  4. MDS not disabled and for version219, it should show a loading state when no data source is configured, and show all presets when data source is configured.
    mds disabled with log

loading state when version 217
version217 presets

loadingstate_version218
version218 presets
version219_loading state
version219_presets

public/pages/workflows/new_workflow/utils.ts Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/utils.ts Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/utils.ts Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/utils.ts Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/utils.ts Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/utils.ts Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/utils.ts Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/new_workflow.tsx Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/new_workflow.tsx Outdated 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.

Tested locally on a 2.19 cluster, built semantic search and RAG workflows, checked the processor lists etc. Noticed a few regressions on the existing preset values, should be one line changes a few places.

Other than that LGTM! Will approve after those changes are made.

public/pages/workflows/new_workflow/utils.ts Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/utils.ts Outdated Show resolved Hide resolved
and change enrich data section processors based on the version.

Signed-off-by: Kama Huang <[email protected]>
Kama Huang added 8 commits December 31, 2024 14:50
…ading the page. If disabled, the version should be set to 2.19+

Signed-off-by: Kama Huang <[email protected]>
Signed-off-by: Kama Huang <[email protected]>
Signed-off-by: Kama Huang <[email protected]>
…workflows file and modify processors for each preset in the fetch function

Signed-off-by: Kama Huang <[email protected]>
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.

LGTM!

@ohltyler
Copy link
Member

Please follow up by front-porting to main (3.0) branch.

@ohltyler ohltyler merged commit ea08b7d into opensearch-project:2.x Dec 31, 2024
5 of 6 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 31, 2024
Signed-off-by: Kama Huang <[email protected]>
Co-authored-by: Kama Huang <[email protected]>
(cherry picked from commit ea08b7d)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ohltyler pushed a commit that referenced this pull request Dec 31, 2024
(cherry picked from commit ea08b7d)

Signed-off-by: Kama Huang <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Kama Huang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants