-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Enable use of the new leaner ported BatchEngine code path #40241
Conversation
- Basic load test works - Basic load test with image works - More testing needed still
Still need tests for: - JSON response instead of text - Not first response - Streaming
Deserialization class for PrivateEndpointConnection was missing. Since we don't actually need this information for current functionality, this field in the JSON is now ignored by commenting out the respective sections in the model
Deserialization class for PrivateEndpointConnection was missing. Since we don't actually need this information for current functionality, this field in the JSON is now ignored by commenting out the respective sections in the model
…test failures due to irrelevant header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enables the use of a new, leaner BatchEngine code path by introducing a new kwarg (_use_run_submitter_client) and unifying client interfaces under the BatchClient protocol. Key changes include:
- Adding the _use_run_submitter_client argument to trigger the new run path.
- Creating and integrating the BatchClient protocol to standardize CodeClient, ProxyClient, and RunSubmitterClient.
- Refactoring adapter, evaluation, and telemetry code to remove hard dependencies on promptflow.
Reviewed Changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated no comments.
File | Description |
---|---|
sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/_legacy/_adapters/* | Updates to support optional promptflow dependency and legacy adapter usage. |
sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/_evaluate/* | Refactored evaluate and run context logic to use the BatchClient protocol and new run submitter client when requested. |
sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/_evaluators/* | Minor updates to evaluators to align with the new client interfaces and dependency changes. |
Comments suppressed due to low confidence (3)
sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/_evaluate/_utils.py:40
- Using 'inplace=True' with drop() returns None, which will cause the subsequent call to rename() to fail. Consider calling drop() with inplace=False and assigning the returned dataframe.
result_df = source_df.drop(columns=columns_to_drop, inplace=inplace)
sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/_evaluate/_batch_run/proxy_client.py:79
- The debug print statements in get_metrics() should be removed in favor of structured logging to avoid unintended console output in production.
print("Aggregated metrics")
sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/_evaluate/_batch_run/proxy_client.py:50
- [nitpick] Disallowing pandas DataFrame in the run() method may be unexpected given that other client implementations allow it; please add a clarifying comment to explain why DataFrame input is not supported in ProxyClient.
if isinstance(data, pd.DataFrame):
API change check APIView has identified API level changes in this PR and created following API reviews. |
- Added a new kwargs argument called `_use_run_submitter_client` that is set to true when calling evaluate() will enable the use of the new faster and leaner ported code path without the promptflow dependenecy. Please note that this is still in a "happy path" works state with some features missing (e.g. handling target function calls which will be added in a future PR). The goal here is to enable testing of this code path sooner to enable us to start finding bugs/issues - Created a `BatchClient` protocol to standardize the existing `CodeClient`, `ProxyClient`, as well as the newly added `RunSubmitterClient`. This makes the evaluate logic simpler in the rest of the code - Though still installed by default, made promptflow an optional dependency by: - Creating some adapters to handle the case where promptflow is not installed, and replace with either some stub code, or the closest ported version of the code as needed - Added some additional dependencies to setup.py that were implicitly brought in by the promptflow dependency that are still needed by the ported legacy code - Removed some unneeded code now that tracing support has been deprecated/disabled, as well as some unneeded test code
_use_run_submitter_client
that is set to true when calling evaluate() will enable the use of the new faster and leaner ported code path without the promptflow dependenecy. Please note that this is still in a "happy path" works state with some features missing (e.g. handling target function calls which will be added in a future PR). The goal here is to enable testing of this code path sooner to enable us to start finding bugs/issuesBatchClient
protocol to standardize the existingCodeClient
,ProxyClient
, as well as the newly addedRunSubmitterClient
. This makes the evaluate logic simpler in the rest of the code