Skip to content

fix: bound the image fetch in the TRT-LLM preprocessing template - #129

Merged
mattwittwer merged 4 commits into
mainfrom
mwittwer/bound_example_image_fetch
Sep 14, 2026
Merged

mattwittwer merged 4 commits into
mainfrom
mwittwer/bound_example_image_fetch

Conversation

@mattwittwer

@mattwittwer mattwittwer commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

What does the PR do?

Bounds the remote image fetch in the TRT-LLM preprocessing template. load_images_from_urls fetched a request-supplied URL with no timeout, no size limit, and no status check. Adds an inactivity timeout, a total transfer deadline, and a max response size, and raises on HTTP errors instead of passing an error page to PIL. The inline base64 branch is unchanged.

Checklist

  • PR title reflects the change and is of format <commit_type>: <Title>
  • Changes are described in the pull request.
  • Related issues are referenced.
  • Populated github labels field
  • Added test plan and verified test passes.
  • Verified that the PR passes existing CI.
  • Verified copyright is correct on all changed files.
  • Added succinct git squash message before merging.
  • All template sections are filled out.
  • Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

  • fix

Related PRs:

Test plan:

  • CI Pipeline ID: N/A
    Ran this example locally after the updates

@greptile-apps

greptile-apps Bot commented Aug 31, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 3/5

The PR does not yet appear safe to merge because remote fetches can still run indefinitely and fetch failures can still abort valid sibling requests in a dynamic batch.

Findings

  1. P1 Fetch errors abort batches ▶

Summary

  • The PR bounds remote image downloads with an inactivity timeout, HTTP status validation, and a 32 MiB response limit, while leaving inline base64 handling unchanged. It also pins the CI Ruff version to match the pre-commit configuration.

Reviews (4) · Last reviewed commit: "fix pre-commit python package issue"

Comment thread src/triton_cli/templates/trt_llm/preprocessing/1/model.py Outdated
Comment on lines +781 to +788
# Read one byte past the limit so that an oversized response is
# rejected rather than silently truncated.
image_data = response.raw.read(self.IMAGE_FETCH_MAX_BYTES + 1,
decode_content=True)
if len(image_data) > self.IMAGE_FETCH_MAX_BYTES:
raise ValueError(
f"[TensorRT-LLM][ERROR] Image at {img_url} exceeds the "
f"{self.IMAGE_FETCH_MAX_BYTES} byte limit.")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Fetch errors abort batches

When one request in a dynamic batch receives an HTTP error, exceeds the size limit, or times out, these new exceptions escape the unguarded per-request loop and abort the entire execute invocation, causing otherwise valid requests in the same batch to fail. Convert fetch failures into an error response for only the offending request.

Knowledge Base Used: TensorRT-LLM model pipeline templates

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated

@greptile-apps

greptile-apps Bot commented Aug 31, 2026

Copy link
Copy Markdown

Want your agent to iterate on Greptile's feedback? Try greploops.

@mattwittwer mattwittwer self-assigned this Sep 10, 2026
whoisj
whoisj previously approved these changes Sep 10, 2026

@whoisj whoisj left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

lgtm

@mattwittwer
mattwittwer merged commit 068dae8 into main Sep 14, 2026
4 of 6 checks passed

This branch was successfully deployed

No deployments
GITLAB — 5cf1e59b Deployed Sep 14, 2026 by mattwittwer via mirror_repo #190
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.

2 participants