diffusers icon indicating copy to clipboard operation
diffusers copied to clipboard

fix: Fixed `requests.get()` function call by adding `timeout` parameter

Open Sai-Suraj-27 opened this issue 1 year ago • 2 comments

What does this PR do?

Fixed requests.get() function call by adding 10 seconds timeout. Fixes #7730

Before submitting

  • [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • [x] Did you read the contributor guideline?
  • [ ] Did you read our philosophy doc (important for complex PRs)?
  • [ ] Was this discussed/approved via a GitHub issue or the forum? Please add a link to it if that's the case.
  • [ ] Did you make sure to update the documentation with your changes? Here are the documentation guidelines, and here are tips on formatting docstrings.
  • [ ] Did you write any new necessary tests?

Who can review?

@DN6 @sayakpaul

Sai-Suraj-27 avatar Apr 22 '24 15:04 Sai-Suraj-27

cc @DN6 here for a review

yiyixuxu avatar Apr 22 '24 18:04 yiyixuxu

Don't think the timeout should be hardcoded. I think it's better to define a constant e.g DIFFUSERS_REQUEST_TIMEOUT that you should be able to override via environment variable and then pass that to the requests timeout. That way you can change the timeout based on your network.

Also, I think we can set a default to 60s (downloading images can take time on slow connections)

DN6 avatar Apr 24 '24 07:04 DN6

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

github-actions[bot] avatar Sep 14 '24 15:09 github-actions[bot]