feat: add required fileds #327
Conversation
Summary of ChangesHello @shijinpjlab, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the system's data input handling by introducing a standardized mechanism for declaring required fields for LLM models and evaluation rules. This change centralizes field definitions and improves the user experience in the Gradio application by dynamically suggesting necessary input fields based on selected components, making the data mapping process more intuitive and less error-prone. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a _required_fields attribute to models, which is a great improvement for dynamically configuring the UI. The implementation in the Gradio app correctly uses this new attribute. My review includes suggestions to refactor some duplicated code in the Gradio app, correct an improperly assigned required field for a search tool, and improve consistency in how required fields are declared for vision models to avoid confusion.
| description = "Search the web for factual information using Tavily AI" | ||
| config: TavilyConfig = TavilyConfig() | ||
|
|
||
| _required_fields = [RequiredField.IMAGE] |
There was a problem hiding this comment.
The _required_fields is set to [RequiredField.IMAGE], which is incorrect. The TavilySearch tool performs a web search based on a text query, not an image. This is misleading and could cause issues if this attribute is used for tools in the future. It should likely be [RequiredField.CONTENT] to reflect that the query often originates from text content.
| _required_fields = [RequiredField.IMAGE] | |
| _required_fields = [RequiredField.CONTENT] |
|
|
||
| def suggest_fields_dataframe(rule_list, llm_list): | ||
| """Suggest required field mappings based on selected evaluators""" | ||
| from dingo.io.input import RequiredField |
There was a problem hiding this comment.
The import from dingo.io.input import RequiredField is located inside the suggest_fields_dataframe function. According to PEP 8, imports should be at the top of the file. This also avoids the overhead of re-importing the module on every function call.
| for rule in rule_list: | ||
| # Find which type this rule belongs to | ||
| for rule_type, rules in rule_type_mapping.items(): | ||
| if rule in rules: | ||
| if rule_type in data_column_mapping: | ||
| suggested_fields.update(data_column_mapping[rule_type]) | ||
| break | ||
| if rule in rule_name_map: | ||
| rule_class = rule_name_map[rule] | ||
| if hasattr(rule_class, '_required_fields'): | ||
| for field in rule_class._required_fields: | ||
| if isinstance(field, RequiredField): | ||
| suggested_fields.add(field.value) | ||
|
|
||
| # Fields required by LLM evaluators | ||
| llm_column_mapping = get_llm_column_mapping() | ||
| for llm in llm_list: | ||
| if llm in llm_column_mapping: | ||
| suggested_fields.update(llm_column_mapping[llm]) | ||
|
|
||
| # Generate suggested fields rows | ||
| if llm in llm_name_map: | ||
| llm_class = llm_name_map[llm] | ||
| if hasattr(llm_class, '_required_fields'): | ||
| for field in llm_class._required_fields: | ||
| if isinstance(field, RequiredField): | ||
| suggested_fields.add(field.value) |
There was a problem hiding this comment.
The logic for iterating through rule_list and llm_list to collect _required_fields is duplicated. This can be refactored into a helper function to avoid code repetition and improve maintainability. For example, you could create a helper that takes a list of model names and a map of names to classes, and returns the set of required fields.
| "evaluation_results": "" | ||
| } | ||
|
|
||
| _required_fields = [RequiredField.CONTENT] |
There was a problem hiding this comment.
This model classifies images, but _required_fields is set to [RequiredField.CONTENT]. This is confusing because content usually implies text. For consistency with other vision models and to make the UI mapping more intuitive, this should be [RequiredField.IMAGE]. The build_messages method would also need to be updated to use input_data.image instead of input_data.content to align with this change.
| _required_fields = [RequiredField.CONTENT] | |
| _required_fields = [RequiredField.IMAGE] |
| "evaluation_results": "通过对比VLM输出与OCR ground truth，识别文字遗漏、错误、幻觉等问题" | ||
| } | ||
|
|
||
| _required_fields = [RequiredField.CONTENT] |
There was a problem hiding this comment.
This is a Vision-Language Model for OCR understanding, which implies it needs an image. However, _required_fields is set to [RequiredField.CONTENT]. This is confusing and inconsistent with other VLM models in this PR. It should be [RequiredField.IMAGE] to make it clear that an image input is expected. The inherited build_messages method would also need to be overridden to handle input_data.image.
| _required_fields = [RequiredField.CONTENT] | |
| _required_fields = [RequiredField.IMAGE] |
1 participant
No description provided.