Summarization model #19
Conversation
|
|
||
| export const OpenAISummarizationModel: React.FC<OpenAISummarizationModelProps> = ({ model, onSelect, openAIFlyOutOpen }) => { | ||
|
|
||
| const [selectedModel, setSelectedModel] = useState(model ?? SummarizationModelName.gpt3_5_turbo_1106); |
There was a problem hiding this comment.
useState is redundant here
useForm is taking a role of useState for us
| gpt3_5_turbo = 'gpt-3.5-turbo-instruct', | ||
| } | ||
|
|
||
| const Summarization_Model: EuiSuperSelectOption<string>[] = [ |
There was a problem hiding this comment.
We can simplify all of this things.
All summarization models are an array, and just can be mapped before rendering. Will looks more native and simpe
| } | ||
| > | ||
| <EuiSuperSelect | ||
| options={Summarization_Model} |
There was a problem hiding this comment.
here you can use models.map((model) => ({value: model, inputDisplay: <EuiFlexGroup...}))
There was a problem hiding this comment.
Before I make the changes, I want to clarify if this is something is encourage in Elastic?
The reason is that, under the hood, it is essentially the same thing. By creating a string array and render each element in the options or I can utilize typescript to create the array and plug in within the component.
Please let me know if you still feels strongly against it, I will push the changes or will change it in my next PR.
There was a problem hiding this comment.
It mostly to keep thing simple and avoid copypasting things and make it more complex and specific to our needs.
Anyway this solution is working so it up to you to apply or not this changes :)
Imagine we gonna add few more model, it will be just a new elements of array. If this elements will have another icon we don't need to create another render for it, we just will put icon as a new param and so on.
There was a problem hiding this comment.
got it. My idea was based on a single icon but not with multiple icons. I will do some research in the code base and update in my next PR if needed
| gpt3_5_turbo = 'gpt-3.5-turbo-instruct', | ||
| } | ||
|
|
||
| const Summarization_Model: EuiSuperSelectOption<string>[] = [ |
There was a problem hiding this comment.
And small things the naming of const and values suppose to follow CamelCase or camelCase format
| <EuiSuperSelect | ||
| options={Summarization_Model} | ||
| valueOfSelected={selectedModel} | ||
| onChange={(value) => onChange(value)} |
There was a problem hiding this comment.
nitpick: not necessary to create new function here
onChange={onChange} is enough
7072aa8
into
yansavitski:ai-playground
Summary
Adding Summarization model
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers