Skip to content
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

[PT2.6][Windows] Fix Windows Codegen #1057

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

[PT2.6][Windows] Fix Windows Codegen #1057

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
40514ad
Fix Windows Codegen
ratnampa Nov 6, 2024
894eebe
Merge branch 'main' into ratnampa/fix_windows_codegen
ratnampa Nov 7, 2024
774cf82
Merge branch 'main' into ratnampa/fix_windows_codegen
EikanWang Nov 11, 2024
3d4dfaf
Merge branch 'main' into ratnampa/fix_windows_codegen
EikanWang Nov 12, 2024
e31154f
Merge branch 'main' into ratnampa/fix_windows_codegen
xytintel Nov 12, 2024
1d7968c
Merge branch 'main' into ratnampa/fix_windows_codegen
EikanWang Nov 12, 2024
1b528e6
update code for win only
ratnampa Nov 12, 2024
9b9d67f
Update Codegen.cmake
ratnampa Nov 12, 2024
db34db7
Merge branch 'main' into ratnampa/fix_windows_codegen
ratnampa Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions cmake/Codegen.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ function(GEN_XPU file_yaml)

# Codegen prepare process
if(WIN32)
string(REPLACE "/" "\\" LinkPATH "${CODEGEN_TEMPLATE}templates")
string(REPLACE "/" "\\" TargetPATH "${CMAKE_SOURCE_DIR}/aten/src/ATen/templates")
execute_process(COMMAND cmd /c mklink /D ${LinkPATH} ${TargetPATH})
string(REPLACE "/" "\\" DestPATH "${CODEGEN_TEMPLATE}templates")
string(REPLACE "/" "\\" SrcPATH "${CMAKE_SOURCE_DIR}/aten/src/ATen/templates")
execute_process(COMMAND cmd /c xcopy ${SrcPATH} ${DestPATH} /E /H /C /I /Y > nul)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ratnampa , do you know why mklink did not impact the Windows build in the past?

Copy link
Author

Choose a reason for hiding this comment

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

@EikanWang I am not sure why this wasn't an issue before, but seems that the admin privileges is the only issue. I have updated the code, it should pass CI, and copy should work.

Copy link
Contributor

@ZhiweiYan-96 ZhiweiYan-96 Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If you're not in the Administrators group, you can still create a symbolic link if: UAC is on and You have the "Create Symbolic Links" privilege

@ratnampa @EikanWang Maybe an explanation from google for this. Seems the symbolic links right can be set independently.

Copy link
Author

Choose a reason for hiding this comment

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

@ZhiweiYan-96 when there is an admin privilege issue to create symlink, there are two errors, first the obvious visible one is FileNotFoundError: [Errno 2] No such file or directory: 'C:/Users/sdp/ratnam-work/pytorch/third_party/torch-xpu-ops/yaml/templates\\RegisterDispatchDefinitions.ini' and the other one is the message saying You do not have sufficient privilege to perform this operation. in the cmake configuration info outputted at the start of the build, which is very hard to find for any user. Then they will need to perform some extra steps for the same. There is another option I found is to activate developer mode.

Now the copying might be a concern if the templates folder is large in size, currently it is only 70KB.

Another option is to create a junction using (/J) which will be similar, but it needs the files to be on the same volume (not andissue for us).

Copy link
Contributor

@ZhiweiYan-96 ZhiweiYan-96 Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Now the copying might be a concern if the templates folder is large in size

@ratnampa Thanks for your info, the template file size should be steady and small, as all files in this directory are just come code templates. I think we the copy is affordable for us.

string(REPLACE "/" "\\" RegisterXPU_PATH_BACKSLASH "${RegisterXPU_PATH}")
string(REPLACE "/" "\\" XPUFallback_PATH_BACKSLASH "${XPUFallback_PATH}")
set(REGISTER_FALLBACK_CMD ${FILE_DISPLAY_CMD} ${XPUFallback_PATH_BACKSLASH} ">>" ${RegisterXPU_PATH_BACKSLASH})
Expand Down Expand Up @@ -93,10 +93,20 @@ function(GEN_XPU file_yaml)
${SIMPLE_TRACE}
WORKING_DIRECTORY ${TORCH_ROOT}
DEPENDS
${depended_files}
${depended_files}
${TORCH_XPU_OPS_ROOT}/yaml/native/${file_yaml}
${XPUFallback_PATH}
)

# Post codegen delete the copied templates folder only on Windows.
if(WIN32)
add_custom_target(DELETE_TEMPLATES ALL DEPENDS ${generated_files})
add_custom_command(
TARGET DELETE_TEMPLATES
POST_BUILD
COMMAND ${CMAKE_COMMAND} -E remove_directory "${DestPATH}"
)
endif()
endfunction(GEN_XPU)

# GEN_BACKEND(
Expand Down