Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evaluation parallele bug fix #1406

Conversation

vishalvanpariya
Copy link

@vishalvanpariya vishalvanpariya commented Feb 22, 2024

Evaluation batch parallel processing bug fixed
Bug: #1358

Additional Information

New backend dependency: nest-asyncio

@vishalvanpariya
Copy link
Author

@aakrem @mmabrouk

@aakrem aakrem requested a review from aybruhm February 22, 2024 09:08
Copy link
Member

@aybruhm aybruhm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR, @vishalvanpariya!

Unfortunately, it still doesn't solve the issue:

The LLM app invocation occurs for all data points before any evaluation begins. As a result, we must wait for the complete invocation process to finish across all data points before initiating any evaluations. This leads to significant delays in obtaining evaluation results, particularly for large test sets.

What we want to achieve is to start receiving evaluation outputs immediately after the first data point is processed.

@mmabrouk
Copy link
Member

@vishalvanpariya Thanks a lot for the PR!
@aybruhm I think there is a confusion in the issue number. The issue that @aakrem has created is for a different feature (the one that you have cited). However this PR resolves another bug, right now we are not calling LLM apps in batches as we say we do! Each batch calls are made in sequence and not in parallel as they should be. This is why evaluation is taking an eternity for long test sets.
So to summarize, this does not close #1358 , however it fixes a bug without an issue right now.
Can you please review the code @aybruhm accordingly. Thank you!

Copy link
Member

@mmabrouk mmabrouk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR @vishalvanpariya !
If I understand correctly, we are adding nest_asynco to be able to nest run_batch calls. Is there a way to change the logic so that we don't have the problem and we don't need to use nest_asyncio? It seems nest_asyncio.apply() patches asyncio which I am not very keen to as it might have side effects (what are your thoughts @aybruhm ?)

@aybruhm
Copy link
Member

aybruhm commented Feb 25, 2024

Thanks again for the PR @vishalvanpariya ! If I understand correctly, we are adding nest_asynco to be able to nest run_batch calls. Is there a way to change the logic so that we don't have the problem and we don't need to use nest_asyncio? It seems nest_asyncio.apply() patches asyncio which I am not very keen to as it might have side effects (what are your thoughts @aybruhm ?)

Yes, you're right. I am also concerned about why we are patching the event loop as it may result in unpredictable behaviour.

@vishalvanpariya here's what I suggest doing:

async def run_batch(start_idx: int):
        # ... (existing code up to the loop)

        tasks = []  # Store the tasks for parallel execution
        for index in range(start_idx, end_idx):
            task = asyncio.create_task(run_with_retry(
                uri,
                testset_data[index],
                parameters,
                max_retries,
                retry_delay,
                openapi_parameters,
            ))
            tasks.append(task)

        # Gather results of all tasks 
        results = await asyncio.gather(*tasks)
        for result in results:
            list_of_app_outputs.append(result)
            print(f"Adding outputs to batch {start_idx}") 

The create_task() function schedules the coroutine to run and returns a task object that we can use to monitor its status, cancel it, or await its completion. And the gather() allows the coroutines to be executed concurrently and wait for all of them to complete before continuing.

@mmabrouk
Copy link
Member

Thank you a lot @vishalvanpariya ! We have fixed the bug in a different PR. It should be live in the next oss release in an hour or so. Sorry it took so long

@mmabrouk
Copy link
Member

@all-contributors please add @vishalvanpariya for code

Copy link
Contributor

@mmabrouk

I've put up a pull request to add @vishalvanpariya! 🎉

@vishalvanpariya
Copy link
Author

Thanks @mmabrouk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants