-
Notifications
You must be signed in to change notification settings - Fork 7
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
Support basic semantic search with preset workflow template #121
Conversation
…ogic Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
==========================================
+ Coverage 21.92% 22.18% +0.26%
==========================================
Files 56 57 +1
Lines 812 870 +58
Branches 82 92 +10
==========================================
+ Hits 178 193 +15
- Misses 630 673 +43
Partials 4 4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LG. Added few comments
): TemplateFlows { | ||
const textEmbeddingTransformerNodeId = Object.keys(formValues).find((key) => | ||
key.includes('text_embedding') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constant for text_embedding
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this entire fn will be more generic. Just a first set of hardcoded vals to get e2e working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See TODO on line 24.
key.includes('text_embedding') | ||
) as string; | ||
const knnIndexerNodeId = Object.keys(formValues).find((key) => | ||
key.includes('knn') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above :)
space_type: 'l2', | ||
name: 'hnsw', | ||
parameters: {}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have static values defined here? Aren't we taking the inputs from the user or future plans of doing so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above. As a reminder in the description, this is all hardcoded right now for the purpose of getting an initial use case possible
componentFields={props.selectedComponent.data.fields} | ||
onFormChange={props.onFormChange} | ||
/> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplication can be avoided here
<EuiSpacer size="s" />
{selectedTabId === TAB.NEW || selectedTabId === TAB.EXISTING && (
<InputFieldList
componentId={props.selectedComponent.id}
componentFields={
selectedTabId === TAB.NEW
? props.selectedComponent.data.createFields
: props.selectedComponent.data.fields
}
onFormChange={props.onFormChange}
/>
)}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will update
onFormChange={props.onFormChange} | ||
/> | ||
<EuiSpacer size="s" /> | ||
<EuiSpacer size="m" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have ENUM constant for the size for EuiSpacer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will add
if (props.field.selectType === 'model' && models) { | ||
setOptions(Object.keys(models)); | ||
} | ||
}, [models]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need error handling here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no chance for NPEs as models
is initialized to an empty map, options
is initialized to an empty arr, and selectType
can be null/undefined, in which case the if block will fail. If there was any errors, it will be logged when fetching models, and will just show as an empty dropdown list.
We will have any errors in redux store bubbled up via toasts or static callouts at the top of the pages; there is no UX for that for now.
}; | ||
console.debug( | ||
`There is no saved UI flow for workflow: ${workflowCopy.name}. Generating a default one.` | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we just do a debug or some alert here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm keeping as just a logged debug for now. It's not considered an error as it would just indicate users loading an existing workflow with no ui metadata.
What's the difference between |
Signed-off-by: Tyler Ohlsen <[email protected]>
Just what I have as a placeholder for now, with the idea that some components may allow using existing resources/data vs. creating new. Example being creating a new knn index or using an existing one, or creating a new model or using an existing one. This can easily be removed, in the current examples I have existing entirely disabled. |
Signed-off-by: Tyler Ohlsen <[email protected]> (cherry picked from commit d5ba601)
) Signed-off-by: Tyler Ohlsen <[email protected]> (cherry picked from commit d5ba601) Co-authored-by: Tyler Ohlsen <[email protected]>
Description
This PR updates several interfaces and improves upon some of the base building blocks to support end-to-end creation of a workflow template for basic semantic search, with heavy UI guardrails. Specifically:
toWorkspaceFlow()
andtoTemplateFlows()
for a basic semantic search use case.toWorkspaceFlow()
is used to load a static set of draggable nodes/edges on the UI.toTemplateFlows()
is used to convert expected & validated field data (model ID, etc.) and populate a preset template configuration used for creating the actual workflow via create workflow API. Both of these will become more generic and flexible over time; this is to prove out an initial use caseIComponentField
schema to support more information for a particular field, including help text, links to external documentation, etc. UI components parse and render them accordinglyReactFlowProvider
to move one layer up soResizableWorkspace
can use useReactFlow() hook. This was previously downscoped too far in the previous PR.ui_metadata
field in the backend template to persist UI flow datauser_inputs
andprevious_node_inputs
provision
field increateWorkflow()
(currently not integrated into buttons yet)vh
tuning)Screenshot of updated component inputs with help text and external links. Also includes the
Existing
/New
tabs but scoped down to only allowNew
for now - this may change as scope for initial release is finalized.Demo video showing end-to-end creation of a (mostly hardcoded) template configuration, provisioning the template, and executing semantic search with the newly-created ingest pipeline and knn index.
demo-e2e.webm
Issues Resolved
Makes progress on #73
Check List
--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.