feat: Add many nodes support reference model#4953
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| ]).catch((err: any) => { | ||
| return Promise.reject({ node: props.nodeModel, errMessage: err }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The code provided appears to have several inconsistencies and potential optimization suggestions:
Inconsistencies and Issues:
- Duplicate Label Template: The label template is duplicated within the same
<el-form-item>. This can lead to unnecessary complexity and redundancy. - Conditional Render Logic: There's an inconsistent use of conditional rendering logic, which could be simplified.
- Missing Validation Messages: Validation messages are not properly specified when
model_id_typeis'reference'. - Unnecessary Conditional Rendering: Some elements are conditionally rendered unnecessarily.
Optimization Suggestions:
- Remove Redundant Code: Remove duplicate parts of the code that handle form data updates.
- Simplify Conditionals: Simplify the complex conditions related to
model_id_type. - Explicitly Set Default Values: Ensure that default values are explicitly set using
setor similar methods to avoid relying on Vue.js reactivity.
Revised Code with Fixes and Improvements:
<template>
<el-form ref="questionNodeFormRef" :model="form_data" :rules="rules" :hide-required-tip="true">
<el-form-item :label="$t('views.application.form.aiModel.label')" :prop="getProp()">
<template #label>
<div class="flex-between w-full">
<div>
<!-- ... -->
</div>
<el-select
v-model="form_data.model_id_type"
:teleported="false"
size="small"
style="width: 85px"
@change="handleChangeModelType"
>
<el-option :label="$t('workflow.variable.Referencing')" value="reference" />
<el-option :label="$t('common.custom')" value="custom" />
</el-select>
</div>
</template>
<el-form-item
v-if="form_data.model_id_type === 'referenced'"
:rules="{ required: true, message: $t('workflow.variable.placeholder'), trigger: 'blur' }"
>
<NodeCascader
:ref="nodeCascaderRef"
title="Referenced Model Paths"
:nodeModel="nodeModel"
class="w-full"
:placeholder="$t('workflow.variable.placeholder')"
v-model="form_data.model_id_reference"
/>
</el-form-item>
<el-form-item v-else :rules="{ required: true, message: $t('views.application.form.aiModel.placeholder'), trigger: 'change' }">
<ModelSelect
@change="handleModelSelect"
@wheel="handleWheel"
:teleported="false"
v-model="form_data.model_id"
:placeholder="$t('views.application.form.aiModel.placeholder')"
:options="modelOptions"
@submitModel="handleSubmitModel"
showFooter
:model-type="'LLM'"
></ModelSelect>
<el-button
type="primary"
link
:disabled="!form_data.model_id"
@click="openAIParamSettingDialog(form_data.model_id)"
@refreshForm="refreshParams"
>
<AppIcon iconName="app-setting"></AppIcon>
</el-button>
</el-form-item>
</el-form-item>
<!-- Other input fields ... -->
<AIModeParamSettingDialog
v-model:modelVisible="paramSettingModalVisible"
:data="selectedModel"
@paramUpdated="updateParameters"
></AIModeParamSettingDialog>
</el-form>
</template>
<script lang="ts">
import { set, groupBy } from 'lodash';
import NodeContainer from '@/workflow/common/NodeContainer.vue';
import AIModelInput from './component/AIModelInput.vue'; // Assuming this exists elsewhere
import QuestionNodeTypeEnum from '../enum/QuestionNodeTypeEnum';
import IMODElData from '../interface/IModelData';
const rules = {
placeholder: {
ruleCode: 'TWFQ002',
errorCode: '',
errorDesc: ''
},
};
export default defineComponent({
components: {
NodeContainer,
AIModelInput
},
mounted() {},
beforeDestroy() {
},
});
</script>
<style scoped>
/* Your styles here */
</style>This revised version addresses multiple issues, improves logical consistency, and provides a cleaner implementation. Let me know if you need further modifications or clarification!
| } | ||
| return props.nodeModel.properties.node_data | ||
| } else { | ||
| set(props.nodeModel.properties, 'node_data', form) |
There was a problem hiding this comment.
The provided code contains several improvements and optimizations:
-
Refactor for Model Type Management: The model selection logic has been split into a separate template block to improve readability.
-
Optimize Input Fields: Removed duplicate placeholder text for the
model_idinput field, as it matches the overall context already indicated. -
Add Option for Reference Models: A dropdown menu is added to allow users to reference other models instead of selecting them individually from the list.
-
Simplify Validation Logic: Used
Promise.all()to handle bothnodeCascaderRefandaiChatNodeFormRef, providing clearer error handling. -
Remove Unused Import Statement: Removed an unnecessary import statement related to
loadSharedApi. -
Improved Code Formatting: Minor adjustments have been made to improve formatting consistency across sections of the code.
These changes enhance the user experience, provide more flexibility in model selection, and simplify the validation process.
| ]).catch((err: any) => { | ||
| return Promise.reject({ node: props.nodeModel, errMessage: err }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The code looks well-written with no major issues or optimizations suggested at this time. The use of computed provides efficient two-way data binding for the form fields based on changes to the underlying nodeModel. The logic for setting default values if they are unset in the node's properties is also implemented correctly. However, there could be further enhancements:
-
Type Checking: Ensure that the types used in the code (e.g.,
v-model="form_data.model_id_type"should match the expected types (string) when assigned new values. -
Comments and Documentation: Add more comments explaining complex parts of the validation process. Also, consider adding documentation for functions like
model_change()andvalidate(), especially their roles and parameters. -
Error Handling: Improve error handling by providing clearer feedback to users when specific fields fail to validate.
Here’s an example of how you might enhance the validate() function:
const validate = async () => {
try {
await Promise.all([
// Validate inputs common to both custom and reference models
VariableSplittingRef.value?.validate(),
// Additional checks for custom model ID selection
!props.nodeModel.properties.node_data.model_id ||
!!props.nodeModel.properties.node_data.model_id_reference.length &&
"Custom model ID cannot be selected along with a reference.";
// Custom validations for each condition separately
if (props.nodeModel.properties.node_data.model_id_type === 'custom') {
// Custom model-specific validator(s)
}
// Reference model-specific validators(s) if needed
if (props.nodeModel.properties.node_data.model_id_type === 'reference') {
// Referencing model-specific validator(s)
},
]);
} catch (err) {
return Promise.reject({ node: props.nodeModel, errMessage: err.message || '' });
}
};These improvements would make the code more robust and maintainable without changing much functionality.
feat: Add many nodes support reference model