8000 fix codecache write_atomic path issue on Windows. (#138331) by xuhancn · Pull Request #138992 · pytorch/pytorch · GitHub
[go: up one dir, main page]
Skip to content

fix codecache write_atomic path issue on Windows. (#138331) #138992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fix codecache write_atomic path issue on Windows. (#138331) #138992

wants to merge 1 commit into from

Conversation

xuhancn
Copy link
Collaborator
@xuhancn xuhancn commented Oct 26, 2024 •
edited by pytorch-bot bot
Loading

Fixes #138211

Path.rename function has Windows OS specific behavior, that will raise FileExistsError when the target file existing. This behavior is not happened on Linux, so I write a small repoduce code to figure out what happened.

After stepping trace the repo code:

import os
import sys
from pathlib import Path

_IS_WINDOWS = sys.platform == "win32"

def test_case():
    cwd = os.getcwd()
    path1 = os.path.join(cwd, "haha1.txt")
    path2 = Path(os.path.join(cwd, "haha2.txt"))

    try:
        path2.rename(path1)
    except FileExistsError as e_file_exist:
        if _IS_WINDOWS:
            # on Windows file exist is expected: https://docs.python.org/3/library/pathlib.html#pathlib.Path.rename
            shutil.copy2(path2, path1)
            os.remove(path2)
        else:
            raise e_file_exist
    except BaseException as e:
        raise e

    print("run here.")

if __name__ == "__main__":
    test_case()

We found the code path2.rename(path1) can breakdown into:

  1. copy file2's content to file1.
  2. delete file2.

So, we can implemented equal code on Windows path:

shutil.copy2(src=tmp_path, dst=path)
os.remove(tmp_path)

So, we can get current PR.

TODO: need cherry-pick to release/2.5 branch, CC: @atalman .

Pull Request resolved: #138331
Approved by: https://github.com/malfet

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @ColinPeppler @amjames @desertfire @chauhang @aakhundov

@xuhancn @malfet
fix codecache write_atomic path issue on Windows. (pytorch#138331)
4c874ec 
Fixes pytorch#138211

`Path.rename` function has Windows OS specific behavior, that will raise `FileExistsError` when the target file existing.
This behavior is not happened on Linux, so I write a small repoduce code to figure out what happened.

After stepping trace the repo code:
```python
import os
import sys
from pathlib import Path

_IS_WINDOWS = sys.platform == "win32"

def test_case():
    cwd = os.getcwd()
    path1 = os.path.join(cwd, "haha1.txt")
    path2 = Path(os.path.join(cwd, "haha2.txt"))

    try:
        path2.rename(path1)
    except FileExistsError as e_file_exist:
        if _IS_WINDOWS:
            # on Windows file exist is expected: https://docs.python.org/3/library/pathlib.html#pathlib.Path.rename
            shutil.copy2(path2, path1)
            os.remove(path2)
        else:
            raise e_file_exist
    except BaseException as e:
        raise e

    print("run here.")

if __name__ == "__main__":
    test_case()
```
We found the code `path2.rename(path1)` can breakdown into:
1. copy file2's content to file1.
2. delete file2.

So, we can implemented equal code on Windows path:
```python
shutil.copy2(src=tmp_path, dst=path)
os.remove(tmp_path)
```

So, we can get current PR.

TODO: need cherry-pick to release/2.5 branch, CC: @atalman .

Pull Request resolved: pytorch#138331
Approved by: https://github.com/malfet

Co-authored-by: Nikita Shulga <2453524+malfet@users.noreply.github.com>
@pytorch-bot PyTorch Bot
Copy link
pytorch-bot bot commented Oct 26, 2024 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/138992

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 4c874ec with merge base b7eb725 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@xuhancn xuhancn mentioned this pull request Oct 26, 2024
Closed
8 tasks
@xuhancn xuhancn added this to the 2.5.1 milestone Oct 28, 2024
@kit1980 kit1980 modified the milestones: 2.5.1, 2.6.0 Oct 28, 2024
@kit1980
Copy link
Contributor
kit1980 commented Oct 28, 2024

Hi @xuhancn
We are not going to pick this into 2.5.1, as @malfet explained to you in #138331 (comment)

@kit1980 kit1980 closed this Oct 28, 2024
@xuhancn
Copy link
Collaborator Author
xuhancn commented Oct 29, 2024

Hi @xuhancn We are not going to pick this into 2.5.1, as @malfet explained to you in #138331 (comment)

Okay, let's move on to release/2.6.

@xuhancn xuhancn mentioned this pull request Dec 6, 2024
Closed
@atalman atalman mentioned this pull request Jan 13, 2025
Closed
73 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
ciflow/inductor module: inductor open source
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@xuhancn @kit1980 @pytorchbot
0