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

Add initial component interfaces & basic node rendering #32

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

ohltyler
Copy link
Member

Description

This PR accomplishes 3 main things:

  1. Sets up the base interfaces that will represent all of the different building blocks / UI components users will select to construct workflows.
  2. Implements interfaces for 2 components: 1/ a text embedding processor, and 2/ a k-NN index.
  3. Sets up a basic WorkflowComponent React component that's purpose is to take in a component instance (a particular k-NN index, for example), and dynamically render based on its configuration. An example with more details is shown below.

Example:

  • shows the same WorkflowComponent building out instances of a text embedding processor and a k-NN index.
  • k-NN index component supports creation, so it has a tabbed portion to toggle the inputs based on if user wants to create a new index, or use existing
  • based on the input types, renders differently (text vs. select vs. JSON, etc.)
  • inputs/outputs displayed there for readability & debugging purposes
  • dummy values used for the indices and the created components
screen-capture.15.webm

Adding a rapid label to indicate this PR may not be perfectly clean or have tests. It is for rapid iteration.

Issues Resolved

Makes progress on #4.

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.

switch (field.type) {
case 'string': {
el = (
<EuiFlexItem key={idx}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

using index as a key isn't always a problem. But if the list can change over time, will it cause issues and even potentially introduce bugs? Do we have other unique identifier in IComponentField that we can use and the key here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Using the index as a key for items in a list is a common JS pattern since it is calculated on-the-fly for each render. This will likely change a lot as we iterate so prefer to leave for now.

<EuiFlexItem>
{props.inputFields?.map((field, idx) => {
let el;
switch (field.type) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: instead of using a switch statement, consider refactoring it to use a mapping object for better clarity and scalability. This also makes adding new input types in the future easier

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Likely this will be moved into more complex helper fns as the number of object types will grow, and the complexity to render them will grow. Prefer to just leave as a simple switch for now.

@ohltyler ohltyler merged commit d3bb6bb into opensearch-project:main Sep 21, 2023
4 checks passed
@ohltyler ohltyler deleted the interfaces-1 branch September 21, 2023 22:39
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 21, 2023
Signed-off-by: Tyler Ohlsen <[email protected]>
(cherry picked from commit d3bb6bb)
ohltyler added a commit that referenced this pull request Sep 21, 2023
Signed-off-by: Tyler Ohlsen <[email protected]>
(cherry picked from commit d3bb6bb)

Co-authored-by: Tyler Ohlsen <[email protected]>
@ohltyler ohltyler mentioned this pull request Oct 9, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants