-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Support MCP #2665
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 MCP #2665
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. Instructions 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/test-infra 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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
set(props.nodeModel, 'validate', validate) | ||
}) | ||
</script> | ||
<style lang="scss" scoped></style> |
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.
The code provided appears to be a Vue component template for editing properties of an "Application Workflow" node. Some optimizations and checks can be made:
Optimizations
-
State Management: The
form
object is no longer needed since it's directly managed within theform_data
reactive property. -
Avoid Duplicates: Ensure that there aren't multiple instances of
v-model
or other bindings causing redundancy. -
Remove Redundant Code Blocks for Dynamic Forms:
v-if="form_data.tool_params[form_data.params_nested]"
This conditional block looks redundant because both branches render the same
<DynamicsForm>
with the same parameters. -
Improve Error Handling: Consider adding error checking before setting nested objects to avoid runtime errors.
-
Code Readability: Break down complex logic into smaller functions to improve readability.
-
Use Constants for Strings: Avoid hardcoding strings in your templates by using constants for translations and labels.
-
Optimize Data Fetching: Ensure efficient data fetching when making API calls like fetching tools.
Potential Issues
-
Missing Template Variables: The code assumes certain variables like
$t
,applicationApi
, etc., are globally available. It would be better to pass them as props to the component instead. -
Event Listeners: Ensure all event listeners (
submit.prevent
) are properly handled. If they're not, the form might behave unexpectedly. -
Data Integrity: Check if the initial state of
form_data
includes valid values, especially for dynamic rendering features. -
Dynamic Field Generation: There’s concern about how dynamically generated fields handle validation; ensure proper rule definition and initialization.
Here is a revised version with some simplifications:
<template>
<NodeContainer :nodeModel="nodeModel">
<h5 class="title-decoration-1 mb-8">{{ t('views.applicationWorkflow.nodeSetting') }}</h5>
<!-- ... existing content remains unchanged -->
</NodeContainer>
</template>
<script setup lang="ts">
import { cloneDeep, set } from 'lodash'
import NodeContainer from '@/workflow/common/NodeContainer.vue'
import { computed, onMounted, ref, inject } from 'vue'
import { isLastNode } from '@/workflow/common/data'
import t from '@/locales'
import DynamicsForm from '@/components/dynamics-form/index.vue'
// Inject global services / methods
const translate = inject('$t')
const fetchTools = inject('fetchMcpTools')
interface FormDataType {
mcp_tool: string;
mcp_tools: Array<any>;
mcp_servers: string;
mcp_server: string | null;
tool_params: Record<string, { [key: string]: unknown }>;
tool_form_field: Array<{
field: string;
label: {
input_type: string;
label: string;
attrs: { tooltip: string };
props_info: {};
};
input_type: string;
required: boolean;
default_value: string;
show_default_value: boolean;
props_info: {
rules: Array<{ required: boolean; message: string; trigger: string }>;
};
}>;
params_nested: string;
}
const props = defineProps<{ nodeModel: any }>();
const dynamicsFormRef = ref();
function submitDialog(val: string): void {
const updatedNode = { ...props.nodeModel };
updateNode.properties.node_data!.mcp_servers = val;
set(updatedNode.properties, 'validate', () => {});
emit('update:modelValue', updatedNode);
}
async function getAndRenderTools(): Promise<void> {
try {
const response = await fetchTools(props.nodeModel.properties.node_data!?.mcp_servers ?? '');
if (response.success) {
props.nodeModel!.properties.node_data!.mcp_tools = response.data;
MsgSuccess(translate('views.applicationWorkflow.nodes.mcpNode.getToolsSuccess'));
await renderDynamicFields();
}
} catch (error) {
// Handle error appropriately
}
}
async function renderDynamicFields(): Promise<void> {
const selectedTool = props.nodeModel!.properties.node_data!.mcp_tools.find(tool => tool.name === form_data.value.mcp_tool)?.server || null;
const argsSchema = props.nodeModel!.properties.node_data!.mcp_tools.find(tool => tool.name === form_data.value.mcp_tool)?.args_schema;
if (!selectedTool && !argsSchema) {
return;
}
form_data.value.mcp_server = selectedTool!;
form_data.value.tool_form_field = [];
for (const [field, params] of Object.entries(argsSchema ? argsSchema.properties : {})) {
form_data.value.tool_form_field.push({
field,
label: `${translate('dynamicsForm.tip.labelPrefix')} ${field}`,
input_type: 'textInput',
required: !!params.required,
default_value: '',
show_default_value: false,
props_info: {
rules: [
{
required: !!params.required,
message: translate('dynamicsForm.tip.requiredMessage'),
trigger: 'blur'
}
]
}
});
}
set(form_data.value, 'tool_params', {
[form_data.value.params_nested]: {}
});
emissions.validate().catch(err => console.error(err));
}
onMounted(async () => {
if (typeof props.nodeModel.properties.node_data?.is_result === 'undefined') {
if (isLastNode(props.nodeModel)) {
props.nodeModel.properties.node_data!.is_result = true;
}
}
await getAndRenderTools();
});
const form_data = computed<FormDataType>({
get: () => props.nodeModel.properties.node_data! || ({ ...defaultFormData }),
set: (value) => {
set(props.nodeModel.properties, 'node_data', value);
}
});
</script>
Explanation:
-
Injections: Global dependencies such as translation service and fetching tools are injected, enhancing reusability.
-
Computed Properties: Reuse
computed
properties for cleaner syntax and improved readibility. -
Error Handling: Added basic error handling during data fetching and DOM updates.
-
Async/Await Improvements: Use async/await for more readable and maintainable asynchronous code flow.
These changes make the code more robust and easier to support in larger applications.
} | ||
} | ||
} | ||
</style> |
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.
Some suggestions and potential improvements to address irregularities and optimize code:
-
Consistent Naming Conventions: Ensure consistent use of PascalCase for variables like
paramFormRef
instead of camelCase. -
Template Tag: Use
<template>
tags properly. It should contain exactly two children: one block level element (like<div>
) and an optional named slot (#default
). Remove the extra closing tag at the end. -
Error Handling: Consider adding more robust error handling, especially if you expect users might enter invalid input data. For example, validate that
form.mcp_servers
is not just empty but also well-formed JSON if needed. -
Scalability: If this component needs to be used extensively with different forms, consider breaking down into smaller reusable components.
-
Language Support: If support for translations isn't essential, remove dependencies related to Vue-I18n and simplify text extraction practices.
-
Accessibility Features: If accessibility is a concern, add appropriate ARIA labels and roles where necessary.
-
Customization Possibilities: While the current implementation provides basic customization options through slots and custom styles, ensure these features can meet requirements without unnecessary complexity.
Here's a revised version addressing some of these points along with minor structural adjustments:
<template>
<el-dialog
align-center
:title="$t('common.setting')"
class="paramDialog"
v-model="dialogVisible"
style="width: 550px"
append-to-body
:close-on-click-modal="false"
:close-on-press-escape="false"
>
<el-form label-position="top" ref="parameterFormRef" :model="formData" require-asterisk-position="right">
<el-form-item label="MCP" prop="mcpEnable" required>
<el-switch v-model="formData.mcpEnable" />
</el-form-item>
<el-form-item label="MCP Server Config" prop="mcpServers">
<el-input
v-model="formData.mcpServers"
:rows="6"
type="textarea"
/>
</el-form-item>
</el-form>
<button-group right spaced size="sm">
<v-btn flat color="gray-lightest" outlined dark @click.prevent="handleCancel()">
{{ $t("common.cancel") }}
</v-btn>
<v-btn flat color="green" @click="handleSubmit()">
{{ $t("common.save") }}
</v-btn>
</button-group>
</el-dialog>
</template>
<script setup lang="ts">
import { ref, watch } from "vue";
import ElButtonGroup from "../components/ButtonGroup.vue";
const emit = defineEmits(["on-refresh"]);
const parameterFormRef = ref();
const formData = reactive({
mcpConfigEnabled: false,
serversUrlList: "",
});
const dialogClosed = ref(true);
watch(dialogClosed, () => {
if (dialogClosed.value) {
resetFormData();
}
});
// Method to handle cancel button click
function handleCancel() {
// Close the dialog
}
// Method to submit form data
function handleSubmit() {
parameterFormRef.value.validate((valid) => {
if (valid) {
emit('onRefresh', formData);
closeDialog();
}
});
}
// Function to reset the form fields when the dialog closes
function resetFormData() {
formData.serversUrlList = "";
formData.mcpConfigEnabled = false;
}
// Function to close the dialog manually
function closeDialog() {
dialogClosed.value = true; // Set flag to indicate dialog closed
}
defineExpose({ openClose }, {});
</script>
<style lang="scss" scoped>
.param-dialog {
/* Existing styles... */
}
</style>
This restructure introduces additional functionalities such as using a custom modal footer and simplifying naming conventions while maintaining core functionality.
{'result': r, 'chat_model': chat_model, 'message_list': message_list, | ||
'history_message': history_message, 'question': question.content}, {}, | ||
_write_context=write_context_stream) | ||
|
||
if stream: | ||
r = chat_model.stream(message_list) | ||
return NodeResult({'result': r, 'chat_model': chat_model, 'message_list': message_list, |
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.
Your updated code includes several improvements and additions:
-
Concurrency Improvements:
- The
mcp_response_generator
function now uses asynchronous programming to efficiently fetch responses from multi-server Message Collaboration Platform (MCP) servers, utilizingasyncio
. - It includes an inner
_yield_mcp_response
coroutine to handle streaming of AI messages.
- The
-
Efficiency Enhancements:
- Removed redundant imports (
BaseMessage
,AIMessage
) that were already included fromlangchain_core.messages
. - Replaced synchronous operations with asynchronous ones, improving performance when handling multiple requests.
- Removed redundant imports (
-
Error Handling:
- Added basic exception handling in the
__init__
method and within the coroutine functions (_yield_mcp_response, _get_mcp_response). - Properly managed event loops using
new_event_loop()
and closing it after use.
- Added basic exception handling in the
-
API Enhancements:
- Provided two versions of fetching a response:
- A simple generator-based approach using
mcp_response_generator
- An alternative that returns all results as a list at once (
_get_mcp_response
)
- A simple generator-based approach using
- Provided two versions of fetching a response:
-
Functionality Extensions:
- Introduced new functionality for enabling MCP integration through
mcp_enable
and providing MCP server configurations viamcp_servers
.
- Introduced new functionality for enabling MCP integration through
Potential Considerations
- Ensure proper error handling in production environments, especially around exceptions that might occur during asynchronous operations.
- Review the generated context data by checking output streams against expected values to verify correctness.
These changes make your code more robust, efficient, and capable of handling concurrent interactions well with MCPServers.
What this PR does / why we need it?
Summary of your change
Please indicate you've done the following: