Ovis2 baseline #42
Ovis2 baseline #42
Conversation
Summary of ChangesHello @brukew, 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 establishes a robust baseline evaluation framework for Video-Language Models (VLMs), focusing on automating video annotation tasks. It integrates the Ovis2 model, introduces a modular architecture for model interaction, evaluation, and postprocessing, and provides configurable settings for various annotation types. The changes aim to streamline the process of applying and assessing VLMs for tasks like activity descriptions and gesture classifications in video data. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 baseline framework for video annotation using the Ovis2 VLM. The overall structure is well-organized, with clear separation of concerns for models, evaluation, and configuration. The code is generally clean and includes good practices like robust error handling in video processing and detailed output artifacts for reproducibility. My review focuses on improving dependency management, configuration portability, and some areas for code refinement and extensibility. The most critical issue is the use of non-existent package versions in pyproject.toml, which will prevent installation.
| torch = "^2.9.1" | ||
| transformers = "^4.57.3" | ||
| scikit-learn = "^1.8.0" |
There was a problem hiding this comment.
The versions specified for torch, transformers, and scikit-learn appear to be non-existent future versions. This will cause dependency resolution to fail during installation. Please update them to valid, existing versions. You can check PyPI for the latest stable versions.
| torch = "^2.9.1" | |
| transformers = "^4.57.3" | |
| scikit-learn = "^1.8.0" | |
| torch = "^2.3.1" | |
| transformers = "^4.41.2" | |
| scikit-learn = "^1.5.0" |
| max_frames: 16 | ||
|
|
||
| data: | ||
| ground_truth_csv: /orcd/scratch/bcs/001/sensein/sails/BIDS_data/test.csv |
There was a problem hiding this comment.
This configuration file contains a hardcoded absolute path for ground_truth_csv. The save_dir on line 20 is also hardcoded. This makes the code non-portable and difficult to run in different environments. Consider using environment variables or making paths relative to a configurable root directory. This issue is present in all newly added YAML configuration files.
| from .ovis2 import Ovis2VLM | ||
|
|
||
|
|
||
| def load_model(model_config: Dict[str, Any]) -> Ovis2VLM: |
There was a problem hiding this comment.
The return type hint is Ovis2VLM. This should be the base class BaseVLM to allow this factory to return different VLM implementations in the future without needing to change the function signature. You will need to add from .base_vlm import BaseVLM at the top of the file.
| def load_model(model_config: Dict[str, Any]) -> Ovis2VLM: | |
| def load_model(model_config: Dict[str, Any]) -> "BaseVLM": |
| out: Dict[str, Any] = {"n": int(len(predictions_df))} | ||
|
|
||
| # Example: just track average length | ||
| if "avg_len" in metrics: | ||
| out["avg_len"] = float(predictions_df["prediction"].fillna("").map(len).mean()) |
There was a problem hiding this comment.
The int() and float() casts here are redundant. len() already returns an integer, and .mean() on a pandas Series typically returns a float.
| out: Dict[str, Any] = {"n": int(len(predictions_df))} | |
| # Example: just track average length | |
| if "avg_len" in metrics: | |
| out["avg_len"] = float(predictions_df["prediction"].fillna("").map(len).mean()) | |
| out: Dict[str, Any] = {"n": len(predictions_df)} | |
| # Example: just track average length | |
| if "avg_len" in metrics: | |
| out["avg_len"] = predictions_df["prediction"].fillna("").map(len).mean() |
| ### How to add a new model | ||
|
|
||
| ## How to Add a New Model |
There was a problem hiding this comment.
There is a duplicate section header here. Line 60 has ### How to add a new model and line 62 has ## How to Add a New Model. One of them should be removed to avoid confusion.
| ### How to add a new model | |
| ## How to Add a New Model | |
| ## How to Add a New Model |
| if name == "ovis2": | ||
| return Ovis2VLM(model_config) |
There was a problem hiding this comment.
This factory pattern using if/elif statements requires modifying this file every time a new model is added. For better extensibility, consider a registration mechanism, such as using entry points or a dictionary of registered models, to allow adding new models without changing the core library code.
|
|
||
| # --- Frame extraction (keep here for now; move later if you want) --- | ||
|
|
||
| def _extract_frames( |
There was a problem hiding this comment.
The _extract_frames method is very long and handles logic for three different video reading backends (decord, opencv, moviepy). To improve readability and maintainability, consider refactoring this into smaller, dedicated functions for each backend, e.g., _extract_frames_decord, _extract_frames_opencv, etc. The main method could then try them in sequence.
|
|
||
| # 1) decord | ||
| try: | ||
| from decord import VideoReader, cpu # type: ignore |
There was a problem hiding this comment.
The import from decord import VideoReader, cpu is located inside the _extract_frames method. It's generally better to place imports at the top of the file for clarity, to avoid repeated import attempts, and to make dependencies explicit. This also applies to the import cv2 on line 249. If these are optional dependencies, you can wrap the top-level import in a try...except ImportError block.
| if pred_label is None: | ||
| pred_label = INVALID_LABEL | ||
|
|
||
| if str(pred_label) == INVALID_LABEL: | ||
| invalid_preds += 1 |
There was a problem hiding this comment.
This block can be simplified. The validate_classification_output function is guaranteed to return a string, so the if pred_label is None: check is redundant. Additionally, pred_label is already a string, so str(pred_label) is also redundant.
| if pred_label is None: | |
| pred_label = INVALID_LABEL | |
| if str(pred_label) == INVALID_LABEL: | |
| invalid_preds += 1 | |
| if pred_label == INVALID_LABEL: | |
| invalid_preds += 1 |
| import sys | ||
|
|
||
| if len(sys.argv) < 2: | ||
| raise SystemExit("Usage: python -m runners.run_experiment path/to/config.yaml") | ||
| main(sys.argv[1]) |
There was a problem hiding this comment.
There are a couple of minor issues here:
import syson line 271 is redundant, assysis already imported at the top of the file (line 7).- The usage message on line 274 is a bit confusing. It refers to
run_experiment, but the script isrun_prediction.py. A clearer message would be helpful.
if len(sys.argv) < 2:
raise SystemExit("Usage: python vlm_baseline/runners/run_prediction.py <path_to_config.yaml>")
main(sys.argv[1])
1 participant
No description provided.