Conversation
![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 support for VITA multimodal models by adding the VITAGenerateAPI model wrapper and specialized evaluators for the TextVQA and VideoBench datasets. The feedback focuses on improving code maintainability and robustness, including suggestions to replace a hardcoded model path with a portable alternative, refactor duplicated URL processing logic, and remove a redundant try-except block in the TextVQA evaluator. Additionally, the reviewer recommends ensuring the API URL remains customizable and adding checks for the API response format to prevent potential indexing errors.
| attr="service", | ||
| type=VITAGenerateAPI, | ||
| abbr="vita-generate-chat", | ||
| path="/mnt/nfs/weight/VITA-MLLM/VITA-1___5", |
There was a problem hiding this comment.
The model path is hardcoded to a specific local directory. This reduces the portability of the configuration across different environments. Consider using a placeholder or an environment variable.
| processed_predictions = [] | ||
| for pred in predictions: | ||
| try: | ||
| processed_predictions.append(self._extract_answer(pred)) | ||
| except Exception: | ||
| processed_predictions.append(pred if isinstance(pred, str) else "") |
There was a problem hiding this comment.
Catching a broad Exception is generally discouraged as it can hide unexpected bugs. Since _extract_answer already includes a check for non-string inputs (line 356), this try-except block is redundant and can be simplified using a list comprehension.
| processed_predictions = [] | |
| for pred in predictions: | |
| try: | |
| processed_predictions.append(self._extract_answer(pred)) | |
| except Exception: | |
| processed_predictions.append(pred if isinstance(pred, str) else "") | |
| processed_predictions = [self._extract_answer(pred) for pred in predictions] |
| verbose=verbose, | ||
| ) | ||
| self.model = model | ||
| self.url = self._get_url() |
There was a problem hiding this comment.
The url parameter passed to the constructor is unconditionally overwritten by self._get_url(). This prevents users from providing a custom full URL via the configuration. It is better to use the provided URL if it is not empty.
| self.url = self._get_url() | |
| self.url = url if url else self._get_url() |
| for item in input_data: | ||
| if item["type"] == "image_url": | ||
| image_url = item["image_url"] | ||
| if isinstance(image_url, dict) and "url" in image_url: | ||
| image_url = image_url["url"] | ||
| if isinstance(image_url, str) and image_url.startswith("file://"): | ||
| image_url = image_url[len("file://"):] | ||
| prompt.append({ | ||
| "type": "image_url", | ||
| "image_url": image_url | ||
| }) | ||
|
|
||
| elif item["type"] == "video_url": | ||
| video_url = item["video_url"] | ||
| if isinstance(video_url, dict) and "url" in video_url: | ||
| video_url = video_url["url"] | ||
| if isinstance(video_url, str) and video_url.startswith("file://"): | ||
| video_url = video_url[len("file://"):] | ||
| prompt.append({ | ||
| "type": "video_url", | ||
| "video_url": video_url | ||
| }) | ||
|
|
||
| elif item["type"] == "audio_url": | ||
| audio_url = item["audio_url"] | ||
| if isinstance(audio_url, dict) and "url" in audio_url: | ||
| audio_url = audio_url["url"] | ||
| if isinstance(audio_url, str) and audio_url.startswith("file://"): | ||
| audio_url = audio_url[len("file://"):] | ||
| prompt.append({ | ||
| "type": "audio_url", | ||
| "audio_url": audio_url | ||
| }) | ||
|
|
||
| elif item["type"] == "text": | ||
| prompt.append({ | ||
| "type": "text", | ||
| "text": item["text"] | ||
| }) |
There was a problem hiding this comment.
There is significant code duplication in handling image_url, video_url, and audio_url. These blocks follow the same logic of extracting the URL and stripping the file:// prefix. Refactoring this into a more generic loop improves maintainability.
| for item in input_data: | |
| if item["type"] == "image_url": | |
| image_url = item["image_url"] | |
| if isinstance(image_url, dict) and "url" in image_url: | |
| image_url = image_url["url"] | |
| if isinstance(image_url, str) and image_url.startswith("file://"): | |
| image_url = image_url[len("file://"):] | |
| prompt.append({ | |
| "type": "image_url", | |
| "image_url": image_url | |
| }) | |
| elif item["type"] == "video_url": | |
| video_url = item["video_url"] | |
| if isinstance(video_url, dict) and "url" in video_url: | |
| video_url = video_url["url"] | |
| if isinstance(video_url, str) and video_url.startswith("file://"): | |
| video_url = video_url[len("file://"):] | |
| prompt.append({ | |
| "type": "video_url", | |
| "video_url": video_url | |
| }) | |
| elif item["type"] == "audio_url": | |
| audio_url = item["audio_url"] | |
| if isinstance(audio_url, dict) and "url" in audio_url: | |
| audio_url = audio_url["url"] | |
| if isinstance(audio_url, str) and audio_url.startswith("file://"): | |
| audio_url = audio_url[len("file://"):] | |
| prompt.append({ | |
| "type": "audio_url", | |
| "audio_url": audio_url | |
| }) | |
| elif item["type"] == "text": | |
| prompt.append({ | |
| "type": "text", | |
| "text": item["text"] | |
| }) | |
| for item in input_data: | |
| item_type = item["type"] | |
| if item_type in ["image_url", "video_url", "audio_url"]: | |
| url = item[item_type] | |
| if isinstance(url, dict) and "url" in url: | |
| url = url["url"] | |
| if isinstance(url, str) and url.startswith("file://"): | |
| url = url[len("file://"):] | |
| prompt.append({"type": item_type, item_type: url}) | |
| elif item_type == "text": | |
| prompt.append({"type": "text", "text": item["text"]}) |
|
|
||
| async def parse_text_response(self, api_response: dict, output: Output): | ||
| texts = api_response.get("text", []) | ||
| output.content = texts[0] if texts else "" |
There was a problem hiding this comment.
If the API response for text is a string instead of a list of strings, texts[0] will return only the first character of the response. Consider adding a check to ensure texts is a list or handle the string case explicitly to avoid parsing errors.
Huangzjun
temporarily deployed
to
smoke-test-approval
GitHub Actions
Inactive
2 participants
Thanks for your contribution; we appreciate it a lot. The following instructions will make your pull request healthier and help you get feedback more easily. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.
感谢您的贡献，我们非常重视。以下说明将使您的拉取请求更健康，更易于获得反馈。如果您不理解某些项目，请不要担心，只需提交拉取请求并从维护人员那里寻求帮助即可。
PR Type / PR类型
Related Issue | 关联 Issue
Fixes #(issue ID / issue 编号) / Relates to #(issue ID / issue 编号)
🔍 Motivation / 变更动机
Please describe the motivation of this PR and the goal you want to achieve through this PR.
请描述您的拉取请求的动机和您希望通过此拉取请求实现的目标。
📝 Modification / 修改内容
Please briefly describe what modification is made in this PR.
请简要描述此拉取请求中进行的修改。
📐 Associated Test Results / 关联测试结果
Please provide links to the related test results, such as CI pipelines, test reports, etc.
请提供相关测试结果的链接，例如 CI 管道、测试报告等。
Does the modification introduce changes that break the backward compatibility of the downstream repositories? If so, please describe how it breaks the compatibility and how the downstream projects should modify their code to keep compatibility with this PR.
是否引入了会破坏下游存储库向后兼容性的更改？如果是，请描述它如何破坏兼容性，以及下游项目应该如何修改其代码以保持与此 PR 的兼容性。
If the modification introduces performance degradation, please describe the impact of the performance degradation and the expected performance improvement.
如果引入了性能下降，请描述性能下降的影响和预期的性能改进。
🌟 Use cases (Optional) / 使用案例（可选）
If this PR introduces a new feature, it is better to list some use cases here and update the documentation.
如果此拉取请求引入了新功能，最好在此处列出一些用例并更新文档。
✅ Checklist / 检查列表
Before PR:
After PR:
👥 Collaboration Info / 协作信息
🌟 Useful CI Command / 实用的CI命令
/gemini review/gemini summary/gemini help/readthedocs build