-
Notifications
You must be signed in to change notification settings - Fork 580
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
Bug - [InferenceClient] - use proxy set in var env #2421
Bug - [InferenceClient] - use proxy set in var env #2421
Conversation
c281750
to
328b7a2
Compare
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.
Prevent the breaking change for aiohttp default trust_env is False see my suggestion
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.
Thanks @morgandiverrez, looks good to me 👍️.
I saw that @Wauplin worked on this files a few weeks ago, thanks in advance for taking a look at this small PR.
Really helpful fix for those working under corporate proxy that want to take advantage of existing environment variables.
…ync and request client for Sync
…ync and request client for Sync
…ync and request client for Sync
Co-authored-by: Benjamin BERNARD <[email protected]>
244bb2a
to
2ee0fb6
Compare
@morgandiverrez @Benvii thanks for the PR and your inputs and sorry for the delay! I'm having a look at it right now :) |
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
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.
@morgandiverrez I hope you don't mind, I pushed commits to your PR. Changing stuff in AsyncClientInference
without changing InferenceClient
is a bit more complex/annoying as it requires to update a generating script. Anyway, now we should have something good. I didn't want to add trust_env
to InferenceClient
since this PR is mostly about AsyncInferenceClient
.
New commits:
- Do not modify
InferenceClient
263d64b - Add
trust_env
parameter + respect it inAsyncInferenceClient
15ba44a (also updating the generation script) - (misc) add docstring + clean newlines cd47edb 392e96e
Once the CI is green, I think we'll be good to merge.
Failing tests are unrelated so let's merge this :) |
The AsyncInferenceClient doesn't use proxies set in environment variables.
It uses aiohttp but doesn't set the trust_env parameter to True.
this PR handle trust_env parameter to and set aiohttp client and request client
issue : AsyncInferenceClient doesn't use proxies set in environment variables #2420