Fix/openvino genai model call - #1
Conversation
…penVINO pipeline to avoid incompatible arg errors
…uts; prefer logprobs/scores
Summary of ChangesHello @samarth-w, 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 addresses a fundamental incompatibility between the 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
|
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring of the OpenVINOCausalLM model. By inheriting from HFLM, it significantly reduces code duplication and improves maintainability. The addition of a configurable cache directory is also a welcome improvement.
However, there are a couple of critical issues that need to be addressed:
- A Byte Order Mark (BOM) has been added to
lm_eval/models/optimum_lm_genai.py, which should be removed. - A new temporary script,
tmp_smoke_test.py, has been added with a hardcoded absolute file path, making it non-portable. This file should either be removed or converted into a proper, parameterized test.
Once these issues are resolved, this will be a solid contribution.
| @@ -1,4 +1,4 @@ | |||
| import logging | |||
| import logging | |||
There was a problem hiding this comment.
|
|
||
| # --- Configuration --- | ||
| # Define the model path and device for the test. | ||
| MODEL_PATH = r"C:\Users\Administrator\Downloads\openvino.genai\tools\llm_bench\models\gpu_models\google_gemma-2b-it_int4_cw" |
There was a problem hiding this comment.
The MODEL_PATH is hardcoded to an absolute local path. This makes the script non-portable and will cause it to fail on any other machine. This path should be parameterized, for example, by using command-line arguments or environment variables.
Additionally, temporary or debugging scripts like this one (as suggested by the name tmp_smoke_test.py) should generally not be committed to the repository. Please either convert this into a formal test within the tests directory or remove it from the pull request.
There was a problem hiding this comment.
Pull Request Overview
This PR removes a large block of duplicated code from the OpenVINO GenAI model implementation and adds configuration for model caching. The purpose is to clean up the codebase by eliminating redundant class definitions and methods while improving the cache directory handling.
Key changes:
- Removed ~570 lines of duplicate code including a complete redundant class definition
- Added cache_dir parameter support for OpenVINO model caching
- Added a temporary smoke test file for debugging OpenVINO GenAI functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tmp_smoke_test.py | New temporary test file for analyzing OpenVINO GenAI log probability functionality |
| lm_eval/models/optimum_lm_genai.py | Removed duplicate code and added cache_dir parameter support for model caching |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # --- Setup --- | ||
| # Configure logging for clear, professional output. | ||
| logging.basicConfig(level=logging.INFO, format='%(asctime)s - %(levelname)s - %(message)s') | ||
|
|
||
| # --- Configuration --- | ||
| # Define the model path and device for the test. | ||
| MODEL_PATH = r"C:\Users\Administrator\Downloads\openvino.genai\tools\llm_bench\models\gpu_models\google_gemma-2b-it_int4_cw" |
There was a problem hiding this comment.
Hard-coded absolute path should be made configurable or use environment variables to avoid system-specific dependencies in test code.
| # --- Setup --- | |
| # Configure logging for clear, professional output. | |
| logging.basicConfig(level=logging.INFO, format='%(asctime)s - %(levelname)s - %(message)s') | |
| # --- Configuration --- | |
| # Define the model path and device for the test. | |
| MODEL_PATH = r"C:\Users\Administrator\Downloads\openvino.genai\tools\llm_bench\models\gpu_models\google_gemma-2b-it_int4_cw" | |
| import os | |
| # --- Setup --- | |
| # Configure logging for clear, professional output. | |
| logging.basicConfig(level=logging.INFO, format='%(asctime)s - %(levelname)s - %(message)s') | |
| # --- Configuration --- | |
| # Define the model path and device for the test. | |
| MODEL_PATH = os.environ.get( | |
| "MODEL_PATH", | |
| r"C:\Users\Administrator\Downloads\openvino.genai\tools\llm_bench\models\gpu_models\google_gemma-2b-it_int4_cw" | |
| ) | |
| if "MODEL_PATH" not in os.environ: | |
| logging.warning("MODEL_PATH environment variable not set. Using default model path: %s", MODEL_PATH) |
| logging.info("="*70) | ||
|
|
||
| full_text = prompt + choice | ||
| raw_tokens = tokenizer.encode(full_text).input_ids.data.tolist()[0] |
There was a problem hiding this comment.
Potential IndexError if the tokenizer returns an empty list. Consider adding bounds checking before accessing index [0].
| raw_tokens = tokenizer.encode(full_text).input_ids.data.tolist()[0] | |
| tokenized_list = tokenizer.encode(full_text).input_ids.data.tolist() | |
| if not tokenized_list: | |
| logging.error("Tokenizer returned an empty list for input: '%s'. Cannot proceed with log-likelihood analysis.", full_text) | |
| return | |
| raw_tokens = tokenized_list[0] |
|
|
||
| formatted_prompt = f"{prompt}\n" + "\n".join([f"{chr(65+i)}) {choice}" for i, choice in enumerate(choices)]) + "\nAnswer:" | ||
|
|
||
| raw_tokens = tokenizer.encode(formatted_prompt).input_ids.data.tolist()[0] |
There was a problem hiding this comment.
Potential IndexError if the tokenizer returns an empty list. Consider adding bounds checking before accessing index [0].
| raw_tokens = tokenizer.encode(formatted_prompt).input_ids.data.tolist()[0] | |
| token_list = tokenizer.encode(formatted_prompt).input_ids.data.tolist() | |
| if not token_list: | |
| logging.error("Tokenization failed: the tokenizer returned an empty list for the prompt.") | |
| return | |
| raw_tokens = token_list[0] |
No description provided.