feat: update gradio to v2.0 #302
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 introduces a substantial update to the Gradio interface, enhancing the flexibility and user-friendliness of configuring evaluation pipelines. It transitions from a rigid set of individual input fields to dynamic 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 significant refactoring of the Gradio application, moving towards a more flexible and scalable configuration model using dataframes for field mappings and evaluator parameters. The changes greatly improve the user interface and the underlying configuration structure.
My review includes a few points for improvement:
- There is a high-severity issue with silent error handling when parsing JSON parameters, which could hide configuration errors from the user.
- Several large blocks of commented-out code related to rule configurations should be removed to improve code clarity.
- The logic for determining required data fields for LLMs is based on string matching, which is fragile. I've suggested a more robust, class-based approach for better maintainability.
| # if rule_config_data is not None and len(rule_config_data) > 0: | ||
| # for row in rule_config_data.values.tolist(): | ||
| # if len(row) >= 6 and row[0]: # Rule name exists | ||
| # rule_name = row[0] | ||
| # config = {} | ||
| # | ||
| # # threshold | ||
| # if row[1] is not None and str(row[1]).strip(): | ||
| # try: | ||
| # config['threshold'] = float(row[1]) | ||
| # except: | ||
| # pass | ||
| # | ||
| # # pattern | ||
| # if row[2] and str(row[2]).strip(): | ||
| # config['pattern'] = str(row[2]) | ||
| # | ||
| # # key_list | ||
| # if row[3] and str(row[3]).strip(): | ||
| # try: | ||
| # val = str(row[3]) | ||
| # config['key_list'] = json.loads(val) if val.startswith('[') else [k.strip() for k in val.split(',') if k.strip()] | ||
| # except: | ||
| # config['key_list'] = [k.strip() for k in str(row[3]).split(',') if k.strip()] | ||
| # | ||
| # # refer_path | ||
| # if row[4] and str(row[4]).strip(): | ||
| # try: | ||
| # val = str(row[4]) | ||
| # config['refer_path'] = json.loads(val) if val.startswith('[') else [p.strip() for p in val.split(',') if p.strip()] | ||
| # except: | ||
| # config['refer_path'] = [p.strip() for p in str(row[4]).split(',') if p.strip()] | ||
| # | ||
| # # parameters | ||
| # if row[5] and str(row[5]).strip(): | ||
| # try: | ||
| # config['parameters'] = json.loads(str(row[5])) | ||
| # except: | ||
| # pass | ||
| # | ||
| # if config: | ||
| # rule_configs[rule_name] = config | ||
|
|
There was a problem hiding this comment.
There are several large blocks of commented-out code related to rule_config_data and generate_rule_config_dataframe (e.g., here, and at lines 216-226, 404-414, 453-457). If this functionality is not yet implemented or has been deferred, it would be better to remove the commented-out code to improve readability and reduce clutter. Dead code can be confusing for future maintainers. If this is intended for future work, consider moving it to a separate feature branch or documenting it in an issue tracker.
| for llm_name in llm_list: | ||
| # Specify different field requirements based on specific LLM type | ||
| if 'VLM' in llm_name or 'Image' in llm_name: | ||
| mapping[llm_name] = ['content', 'image'] | ||
| elif 'Relevant' in llm_name: | ||
| mapping[llm_name] = ['prompt', 'content'] | ||
| else: | ||
| mapping[llm_name] = ['content'] | ||
| return mapping |
There was a problem hiding this comment.
The logic for determining required fields for an LLM relies on checking for substrings like 'VLM', 'Image', or 'Relevant' in the LLM's name. This approach is fragile and not easily extensible. If a new model is added that requires specific fields but doesn't have these keywords in its name, this logic will fail to assign the correct fields.
A more robust approach would be to define these requirements as a property of the model class itself. For example, you could add a required_fields class attribute to each LLM model class. This would make the system more modular and easier to maintain.
Example of a more robust approach:
# In the model definition (e.g., dingo/model/llm/vlm_model.py)
class MyVLMModel(BaseLLM):
required_fields = ['content', 'image']
...
# In app_gradio/app.py
def get_llm_column_mapping():
"""Get column mapping required by each LLM from the model class."""
llm_name_map = Model.get_llm_name_map()
mapping = {}
for llm_name, llm_class in llm_name_map.items():
# Default to ['content'] if not specified
mapping[llm_name] = getattr(llm_class, 'required_fields', ['content'])
return mapping
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
No description provided.