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

Enable use of the new leaner ported BatchEngine code path #40241

Merged
merged 32 commits into from
Apr 2, 2025

Conversation

ralph-msft
Copy link
Member

  • 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

- 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
@Copilot Copilot bot review requested due to automatic review settings March 27, 2025 01:02
@ralph-msft ralph-msft requested a review from a team as a code owner March 27, 2025 01:02
@github-actions github-actions bot added the Evaluation Issues related to the client library for Azure AI Evaluation label Mar 27, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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):

@ralph-msft ralph-msft enabled auto-merge (squash) March 27, 2025 01:06
@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-ai-evaluation

@ralph-msft ralph-msft merged commit 3fe7515 into Azure:main Apr 2, 2025
26 checks passed
nik1097 pushed a commit to nik1097/azure-sdk-for-python that referenced this pull request Apr 5, 2025
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Evaluation Issues related to the client library for Azure AI Evaluation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants