refactor: Remove networking imports outside azure core #3683
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…pipeline
The `RetryClient` from `aiohttp_retry` was used extensively through
promptflow/evals/synthetic to do async http requests. Both directly
and as the basis of the `AsyncHTTPClientWithRetry` class.
The two retry strategies used with RetryClient were:
* Retries with fixed length backoff
* Retries with jittered exponential backoff
azure.core pipelines essentially covers those two strategies with
their (Async)RetryPolicy, with the major difference being that
the exponential flavor doesn't incorporate random jitter.
This commit ends up accomplishing the following:
* All references to RetryClient, and the two duplicate (?)
implementation of `AsyncHTTPClientWithRetry` were replaced
with our "requests-like" azure.core.pipeline. Appropriate retry
policies were applied to replicate the previous functionality.
* The dependency on `aiohttp_retry` was removed, since the SDK
no longer references it
kdestin
changed the title
promptflow-evals test result 12 files ± 0 12 suites ±0 1h 40m 58s ⏱️ + 1h 22m 48s Results for commit ce632fb. ± Comparison against base commit cef8ee5. This pull request removes 110 and adds 13 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
pylint "struggles" with decorators, and seems unable to infer
the parameters of the decorated functions of (Async)HttpPipeline.
Adding #pylint: disable comments to those invocations that are
known to be correct
kdestin
force-pushed
the
kdestin/remove-networking-imports-outside-azure-core
branch
from
3fd6b5b to
e077ca4
Compare
Removing `aiohttp_retry` means we don't have a guarantee `aiohttp`
kdestin
force-pushed
the
kdestin/remove-networking-imports-outside-azure-core
branch
from
e077ca4 to
2b4c502
Compare
diondrapeck
previously approved these changes
Aug 23, 2024
diondrapeck
approved these changes
Aug 27, 2024
needuv
approved these changes
Aug 27, 2024
kdestin
deleted the
kdestin/remove-networking-imports-outside-azure-core
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request removes all networking imports flagged by rule
C4749(networking-import-outside-azure-core-transport)from azure-pylint-guidelines-checker, and refactors the uses of those imports to do http requests through azure-core.Concretely this pull request:
src/promptflow-evals/promptflow/evals/_http_utils.pywhich includes a sync and async version of azure.core.pipeline.Pipeline that provides a requests-like api for general http requests.aiohttp_retryAsyncHTTPClientWithRetry.Background
This PR is part of an effort to ready
promptflow-evalsto be migrated to Azure/azure-sdk-for-python.Azure SDKs are disallowed from directly using networking libraries like
requests,aiohttp, etc... They're instead meant to useazure-core, which under the hood can delegate to those libraries.All Promptflow Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines