Skip to content

[CI/Build] Publish synthetic image caches atomically - #9834

Closed
LinzeShi wants to merge 4 commits into
vllm-project:mainfrom
LinzeShi:fix/synthetic-image-cache-publication
Closed

LinzeShi wants to merge 4 commits into
vllm-project:mainfrom
LinzeShi:fix/synthetic-image-cache-publication

Conversation

@LinzeShi

@LinzeShi LinzeShi commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Contributor intent

1. What outcome do you want to achieve?

Prevent parallel CPU test collection from failing on incomplete synthetic images.

2. What is the highest-leverage variable for that outcome?

A shared JPEG must be complete before its cache path becomes visible.

3. How will this work move that variable?

Encode the image, write a private temporary file, then atomically replace the cache entry.

4. Whose customer or user needs does this address, and what are those needs?

Contributors and CI maintainers need parallel tests to collect reliably.

Purpose

Fixes #9833.

Follow-up to #9784. generate_synthetic_image writes directly to a shared JPEG cache, allowing another pytest worker to read a partial file. Nightly build 18703 fails during MiniMax H3 test collection with PIL.UnidentifiedImageError.

Follow the existing video-cache publication pattern: write a unique temporary file in the same directory, close it, and replace the destination. Clean up the temporary file if publication fails.

Only the test helper and two regression tests change. Cache names, JPEG settings and public configuration are unchanged; no user documentation changes are needed.

Test Plan

Base: 4cbaec9d01466bcf023019afac8a268d7c0d9628. Candidate: 6a45345d78a300d370a9563c4b1d2432b18e9182.

Python 3.12.4, Pillow 13.0.0, NumPy 2.3.5, pytest 9.1.1 and pytest-xdist 3.8.0.

HF_HUB_OFFLINE=1 TRANSFORMERS_OFFLINE=1 HF_DATASETS_OFFLINE=1 \
CUDA_VISIBLE_DEVICES='' VLLM_TARGET_DEVICE=cpu VLLM_PLUGINS='' OMP_NUM_THREADS=2 \
python -m pytest tests/helpers/tests/test_synthetic_image.py \
  tests/helpers/tests/test_synthetic_video.py \
  -m 'core_model and cpu' --run-level core_model -n 4 --dist loadgroup -q

Test Result

  • The original regression failed on the unmodified base. After the review updates, the two focused files pass: 21 passed with four workers, including existing-cache regeneration and failed-publication cleanup.
  • A separate four-process probe, 400 regenerations and 1,800 reader calls, produced 1,247 read failures before and zero after.
  • Failure injection preserves the previous image and leaves no temporary files. Valid cache reuse and creation permissions were also checked.
  • Changed-file pre-commit checks pass, including Ruff, mypy and test markers.

Validation ran offline on CPU without model downloads. The full nightly job was not rerun.

Signed-off-by: Linze-Shi <linzeshi0@gmail.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-11-03T19:16:35.642984Z
We couldn’t complete this review. Comment @codex review to try again.
2e97f53 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@vllm-omni-review-bot

Copy link
Copy Markdown

This PR was classified as CI work.

CI owner: @yenuo26 @NickCao

Routing: @yenuo26 via CI owner, CODEOWNERS; @NickCao via CODEOWNERS

@LinzeShi, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer.

Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment.

LinzeShi commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Checked that the JPEG is complete before publication, the temporary file stays in the same directory, and failure cleanup preserves the previous cache. The regression fails on the base; all 20 focused image/video tests pass with four workers. The concurrent reader/writer probe and injected encoding/replace failures also pass. The full nightly job was not rerun.

@vllm-omni-review-bot

vllm-omni-review-bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Omni ReviewBot triage note

Automated triage of commit 6a45345d78a3 produced:

  • Priority: high. Prompt maintainer attention is suggested.

These are automated triage suggestions only — the final decision belongs to the maintainers.

@yenuo26 yenuo26 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Blocking (P0)
None.

Blocking (P1)
None.

Non-blocking

  1. Mirror the video failure-preserve UT. test_synthetic_video.py::test_failed_cache_publication_preserves_old_file is the negative control for the same open("xb") + os.replace + finally: unlink(missing_ok=True) publish path. test_image_cache_is_published_atomically only covers the happy cold-publish path. Please add an image twin that seeds an existing JPEG, stubs os.replace to raise, asserts the old bytes survive, and asserts no leftover .tmp files — the PR description already claims this was probed manually.
  2. Strengthen the atomic probe like the video twin. Video’s test_video_cache_is_published_atomically uses force_regenerate=True against an existing cache and asserts readers still see the old complete file until replace runs. The new image test asserts not cache_path.exists() (first publish only). Prefer the same existing-cache / force-regenerate shape so a regression that truncates-in-place cannot slip past.
  3. Docstring parity (optional). generate_synthetic_video documents “Cache files are published atomically.”; generate_synthetic_image still does not. One short sentence matching the video wording would help the next reader.

Signed-off-by: Linze-Shi <linzeshi0@gmail.com>
Signed-off-by: Linze-Shi <linzeshi0@gmail.com>
Signed-off-by: Linze-Shi <linzeshi0@gmail.com>

LinzeShi commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! Added the failed replace test, switched the atomic test to regenerate over an existing cache, and added the docstring line. Image/video tests pass with four workers (21 passed).

@yenuo26 yenuo26 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Blocking (P0)
No blocking P0/P1.

Blocking (P1)
None.

Non-blocking
None new on tip 6a45345d. Prior soft nits from the 2e97f534 pass are addressed: failed-replace preserve UT, force-regenerate atomic probe over an existing JPEG (readers still see old bytes until os.replace), and docstring parity with the video publisher.

@vllm-omni-review-bot

Copy link
Copy Markdown
Omni ReviewBot routing record

Assigned Strict on claude-code (mimo-v2.6-pro) under experiment fleet-strict-claudecode-mimo26pro-100-c10-20261015.

@vllm-omni-review-bot

Copy link
Copy Markdown
Omni ReviewBot review status

Review for commit 6a45345d78a300d370a9563c4b1d2432b18e9182 ended as blocked (step 'review' (agent.review_diff): [ensemble x3] API Error: Request rejected (429) · quota exhausted).

LinzeShi commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

Closing since #9648 already landed the same atomic publication, plus recovery for incomplete caches and broader tests. Thanks for the reviews!

@LinzeShi LinzeShi closed this Nov 4, 2026
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.

[Bug]: Parallel CPU test collection can read an incomplete synthetic image

3 participants