Skip to content

Conversation

@ZJY0516
Copy link
Contributor

@ZJY0516 ZJY0516 commented Sep 26, 2025

Purpose

unique_filepath was introduced in #24542 but the corresponding implementation was not included in the final commit.

CC @ProExpertProg

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: zjy0516 <[email protected]>
Copy link
Contributor

@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 adds the unique_filepath utility function, which was intended to be included in a previous PR. The implementation finds a unique file path by incrementing an integer suffix. My review identifies a critical Time-of-Check to Time-of-Use (TOCTOU) race condition in the current implementation. I've suggested a fix that uses an atomic file creation operation to prevent potential file conflicts and data corruption in multi-threaded or multi-process environments. Additionally, I recommend adding unit tests for this new utility to ensure its correctness and handle edge cases, as they appear to be missing.

@ProExpertProg ProExpertProg enabled auto-merge (squash) September 26, 2025 15:13
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 26, 2025
@ProExpertProg ProExpertProg changed the title [Misc] fix unique_filepath [Fix][torch.compile] fix unique_filepath Sep 26, 2025
@ProExpertProg ProExpertProg merged commit 56aafa8 into vllm-project:main Sep 26, 2025
49 of 51 checks passed
@ZJY0516 ZJY0516 deleted the unique_filepath branch September 30, 2025 14:09
pdasigi pushed a commit to pdasigi/vllm that referenced this pull request Oct 2, 2025
Signed-off-by: zjy0516 <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
yewentao256 pushed a commit that referenced this pull request Oct 3, 2025
Signed-off-by: zjy0516 <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
Signed-off-by: yewentao256 <[email protected]>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
Signed-off-by: zjy0516 <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
Signed-off-by: xuebwang-amd <[email protected]>
choprahetarth pushed a commit to Tandemn-Labs/vllm that referenced this pull request Oct 11, 2025
Signed-off-by: zjy0516 <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
lywa1998 pushed a commit to lywa1998/vllm that referenced this pull request Oct 20, 2025
Signed-off-by: zjy0516 <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
alhridoy pushed a commit to alhridoy/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: zjy0516 <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: zjy0516 <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
Signed-off-by: xuebwang-amd <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants