Skip to content

Add timeout to contributor avatar fetch#1057

Open
massy-o wants to merge 2 commits into
keras-team:masterfrom
massy-o:codex/add-contributor-avatar-timeout
Open

Add timeout to contributor avatar fetch#1057
massy-o wants to merge 2 commits into
keras-team:masterfrom
massy-o:codex/add-contributor-avatar-timeout

Conversation

@massy-o
Copy link
Copy Markdown

@massy-o massy-o commented May 14, 2026

Summary

  • add an explicit timeout when the contributor SVG helper downloads GitHub avatars

Why

The helper currently waits indefinitely if an avatar request stalls. A small timeout makes the maintenance script fail fast instead of hanging forever on a single contributor image.

Testing

  • python3 -m py_compile shell/contributors.py

@google-cla
Copy link
Copy Markdown

google-cla Bot commented May 14, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Comment thread shell/contributors.py
import requests
from PIL import Image

REQUEST_TIMEOUT_SECONDS = 10
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review: Keeping the timeout as a named constant makes it easy to tune later and avoids leaving a maintenance script blocked indefinitely on one slow avatar response.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a timeout for network requests when fetching contributor avatars to prevent the script from hanging indefinitely. The reviewer suggested enhancing error handling by adding response.raise_for_status() to ensure the script fails explicitly on HTTP errors.

Comment thread shell/contributors.py
Comment on lines +43 to +45
response = requests.get(
contributor["avatar_url"], timeout=REQUEST_TIMEOUT_SECONDS
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While adding a timeout is a good practice to prevent the script from hanging, it's also important to ensure the request was successful before processing the response. Adding response.raise_for_status() will cause the script to fail with a clear HTTP error if the avatar download fails (e.g., 404 or 500), which is consistent with the 'fail fast' goal of this PR.

Suggested change
response = requests.get(
contributor["avatar_url"], timeout=REQUEST_TIMEOUT_SECONDS
)
response = requests.get(
contributor["avatar_url"], timeout=REQUEST_TIMEOUT_SECONDS
)
response.raise_for_status()

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, accepted in 1e1baef by adding response.raise_for_status() after the avatar request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant