Skip to content

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

Merged
merged 2 commits into from
Mar 24, 2025
Merged

Support MCP #2665

merged 2 commits into from
Mar 24, 2025

Conversation

liuruibin
Copy link
Member

What this PR does / why we need it?

Summary of your change

Please indicate you've done the following:

  • Made sure tests are passing and test coverage is added if needed.
  • Made sure commit message follow the rule of Conventional Commits specification.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed.

Copy link

f2c-ci-robot bot commented Mar 24, 2025

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.

Copy link

f2c-ci-robot bot commented Mar 24, 2025

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

set(props.nodeModel, 'validate', validate)
})
</script>
<style lang="scss" scoped></style>
Copy link
Contributor

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

  1. State Management: The form object is no longer needed since it's directly managed within the form_data reactive property.

  2. Avoid Duplicates: Ensure that there aren't multiple instances of v-model or other bindings causing redundancy.

  3. 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.

  4. Improve Error Handling: Consider adding error checking before setting nested objects to avoid runtime errors.

  5. Code Readability: Break down complex logic into smaller functions to improve readability.

  6. Use Constants for Strings: Avoid hardcoding strings in your templates by using constants for translations and labels.

  7. Optimize Data Fetching: Ensure efficient data fetching when making API calls like fetching tools.

Potential Issues

  1. 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.

  2. Event Listeners: Ensure all event listeners (submit.prevent) are properly handled. If they're not, the form might behave unexpectedly.

  3. Data Integrity: Check if the initial state of form_data includes valid values, especially for dynamic rendering features.

  4. 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:

  1. Injections: Global dependencies such as translation service and fetching tools are injected, enhancing reusability.

  2. Computed Properties: Reuse computed properties for cleaner syntax and improved readibility.

  3. Error Handling: Added basic error handling during data fetching and DOM updates.

  4. 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>
Copy link
Contributor

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:

  1. Consistent Naming Conventions: Ensure consistent use of PascalCase for variables like paramFormRef instead of camelCase.

  2. 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.

  3. 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.

  4. Scalability: If this component needs to be used extensively with different forms, consider breaking down into smaller reusable components.

  5. Language Support: If support for translations isn't essential, remove dependencies related to Vue-I18n and simplify text extraction practices.

  6. Accessibility Features: If accessibility is a concern, add appropriate ARIA labels and roles where necessary.

  7. 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,
Copy link
Contributor

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:

  1. Concurrency Improvements:

    • The mcp_response_generator function now uses asynchronous programming to efficiently fetch responses from multi-server Message Collaboration Platform (MCP) servers, utilizing asyncio.
    • It includes an inner _yield_mcp_response coroutine to handle streaming of AI messages.
  2. Efficiency Enhancements:

    • Removed redundant imports (BaseMessage, AIMessage) that were already included from langchain_core.messages.
    • Replaced synchronous operations with asynchronous ones, improving performance when handling multiple requests.
  3. 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.
  4. 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)
  5. Functionality Extensions:

    • Introduced new functionality for enabling MCP integration through mcp_enable and providing MCP server configurations via mcp_servers.

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.

@liuruibin liuruibin merged commit 563516f into main Mar 24, 2025
3 of 4 checks passed
@liuruibin liuruibin deleted the mcp branch March 24, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants