Added popover to display links to suggested models #616
Conversation
saimedhi
requested review from
dbwiddis,
owaiskazi19,
joshpalis,
ohltyler,
amitgalitz,
jackiehanyang and
minalsha
as code owners
common/constants.ts
Outdated
| // Large Language Models Documentation Links | ||
| export const BEDROCK_CLAUDE_3_SONNET_DOCS_LINK = | ||
| 'https://github.com/opensearch-project/ml-commons/blob/main/docs/remote_inference_blueprints/bedrock_connector_converse_blueprint.md'; | ||
|
|
||
| export const OPENAI_GPT35_DOCS_LINK = | ||
| 'https://github.com/opensearch-project/ml-commons/blob/2.x/docs/remote_inference_blueprints/open_ai_connector_chat_blueprint.md'; | ||
|
|
||
| export const DEEPSEEK_CHAT_DOCS_LINK = | ||
| 'https://github.com/opensearch-project/ml-commons/blob/main/docs/remote_inference_blueprints/deepseek_connector_chat_blueprint.md'; | ||
|
|
||
| // Embedding Models Documentation Links | ||
| export const COHERE_EMBEDDING_MODEL_DOCS_LINK = | ||
| 'https://github.com/opensearch-project/ml-commons/blob/2.x/docs/remote_inference_blueprints/cohere_connector_embedding_blueprint.md'; | ||
|
|
||
| export const BEDROCK_TITAN_EMBEDDING_DOCS_LINK = | ||
| 'https://github.com/opensearch-project/ml-commons/blob/2.x/docs/remote_inference_blueprints/bedrock_connector_titan_embedding_blueprint.md'; | ||
|
|
There was a problem hiding this comment.
These are the blueprints we want to avoid customers visiting, as they are not compatible with ML inference processors, and generally do not have model interfaces, which means they are not compatible with OpenSearch Flow. We need to point to https://github.com/opensearch-project/dashboards-flow-framework/blob/main/documentation/models.md
For any models not present in that file ^, then suggest to add.
| @@ -125,17 +133,58 @@ export function ModelField(props: ModelFieldProps) { | |||
| (props.showInvalid ?? true) && | |||
| getIn(errors, `${field.name}.id`) && | |||
| getIn(touched, `${field.name}.id`); | |||
| const renderPopover = () => { | |||
| if (props.fieldPath === "llm") { | |||
There was a problem hiding this comment.
This isn't a reliable way to determine whether or not we should suggest a model of a certain type; field path names are internal and may change. Suggest to add a prop to ModelField that passes an optional model type, for example. Maybe make an enum of different model types, which is currently just LLM/generative models, and embedding models.
| // Embedding model popover state | ||
| const [embeddingModelPopoverOpen, setEmbeddingModelPopoverOpen] = useState<boolean>(false); | ||
|
|
||
| // Large Language model popover state | ||
| const [largeLaguageModelPopoverOpen, setLargeLaguageModelPopoverOpen] = useState<boolean>(false); |
There was a problem hiding this comment.
How about just have a single generic popover state?
There was a problem hiding this comment.
These components are very repetitive. How about we have a single component, that takes in a single prop determining the model type, and then dynamically render? They seem identical besides the list of models to display, and maybe one-word differences, like "embedding" vs. "large language".
Additionally, this component should just live next to model_field where it is logically coupled, instead of nested under new_workflow.
saimedhi
force-pushed
the
add/model_suggestions
branch
from
7c84740 to
e15bc44
Compare
saimedhi
force-pushed
the
add/model_suggestions
branch
from
e15bc44 to
e5104c6
Compare
Description
screen-capture.41.webm
Check List
--signoffBy 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.