Change finetuning eval code to use single AsyncOpenAI Client#12
Change finetuning eval code to use single AsyncOpenAI Client#12singh-saurabh wants to merge 1 commit intomainfrom
Conversation
…revents file descriptors from exhausting
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @singh-saurabh, 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 refactors the finetuning evaluation script to utilize a single, shared AsyncOpenAI client across all asynchronous operations. This change is crucial for preventing the exhaustion of file descriptors, which can occur when numerous client instances are created and closed rapidly during evaluation runs.
Highlights
- Client Centralization: The AsyncOpenAI client is now instantiated once at a higher level (
run_benchmark_async) and passed down to functions that require it, rather than being created repeatedly. - Function Signature Update: The
get_model_response_asyncandprocess_single_samplefunctions have been updated to accept anAsyncOpenAIclient as an explicit argument. - Resource Management: This refactoring ensures efficient resource utilization by avoiding the repeated creation and destruction of network connections, thereby mitigating file descriptor exhaustion.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the evaluation script to use a single AsyncOpenAI client, which is a great improvement for resource management and preventing file descriptor exhaustion. The implementation is mostly correct, but I've identified a few areas for improvement. The shared client is not being closed, which can lead to resource leaks. I've suggested using an async with statement to manage the client's lifecycle properly. Additionally, I've recommended adding type hints for the new client parameter in function signatures for better code clarity and maintainability.
| # Create shared client for all requests | ||
| client = AsyncOpenAI( | ||
| api_key=API_KEY, | ||
| base_url=BASE_URL | ||
| ) |
There was a problem hiding this comment.
The newly created shared AsyncOpenAI client is not being closed after use. This can lead to resource leaks (like unclosed connections), which undermines the goal of this PR. It's best practice to use the client as an asynchronous context manager with async with to ensure it's always closed properly. The subsequent for loop (starting on line 391) should be indented to be within this async with block.
async with AsyncOpenAI(
api_key=API_KEY,
base_url=BASE_URL
) as client:|
|
||
|
|
||
| async def get_model_response_async(model, system_prompt, user_query, semaphore): | ||
| async def get_model_response_async(model, system_prompt, user_query, semaphore, client): |
There was a problem hiding this comment.
For better code clarity and maintainability, please add a type hint for the new client parameter. This function currently lacks type hints, and adding them, starting with the new parameter, would improve readability.
| async def get_model_response_async(model, system_prompt, user_query, semaphore, client): | |
| async def get_model_response_async(model, system_prompt, user_query, semaphore, client: AsyncOpenAI): |
|
|
||
|
|
||
| async def process_single_sample(sample: Dict[str, Any], model: str, semaphore: asyncio.Semaphore) -> Dict[str, Any]: | ||
| async def process_single_sample(sample: Dict[str, Any], model: str, semaphore: asyncio.Semaphore, client) -> Dict[str, Any]: |
There was a problem hiding this comment.
The new client parameter is missing a type hint. Since other parameters in this function are type-hinted, adding one for client would maintain consistency and improve code clarity.
| async def process_single_sample(sample: Dict[str, Any], model: str, semaphore: asyncio.Semaphore, client) -> Dict[str, Any]: | |
| async def process_single_sample(sample: Dict[str, Any], model: str, semaphore: asyncio.Semaphore, client: AsyncOpenAI) -> Dict[str, Any]: |
1 participant
This prevents file descriptors from exhausting