Fix proxy environment variables not used in v1.0 #3412
Merged
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.
Solve huggingface/transformers#41301 reported by @jerryzh168.
The root cause of this issue was when using a custom transport, the proxy env variables are not used by
httpxeven iftrust_env=True. This is the case since encode/httpx#1122. We were using custom Transport in order to handleHF_HUB_OFFLINEenv variable (i.e. raise exception if attempt to make an HTTP call) and add custom request id header to help with debugging.The workaround in this PR is to use
event_hooksinstead. We are still catching offline mode + adding debug logs at runtime but don't use custom transport anymore. I've added a regression test to check thatHTTP_PROXY/HTTPS_PROXYare correctly read from env.Note: there is a drawback to this PR. We do not catch any
httpx.RequestErrors anymore to add the request id to the error message itself. This is not possible to do without a custom Transport. In practice it's not much of an issue because the request id is added back byhf_raise_for_status. The only regression is if an error happens before the server respond (i.e. a readtimeout or connection error), in which case we won't have a request id in the error message. The benefits of this PR still outweigh this drawback IMO.