[Build] fix build issues for native Windows.

[python3kgae]

Add install_requirements.ps1 for build on native Windows.

Also fix issues block build on native Windows.

1. Get the Windows version of buck2 for Windows.
2. Check the path based on the build configuration for Windows in setup.py.
3. Avoid generating 'def flat.exe()' which is illegal python.
4. Add the Windows version of kernel_link_options.
5. Add links of pthreadpool and cpuinfo for custom_ops_aot_lib.
6. Define ssize_t for Windows.

Still have one link issue tracked with #4659.

For #4661

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

❌ 7 New Failures

As of commit 1b8899c with merge base f887d72 ():

NEW FAILURES - The following jobs have failed:

• pull / test-llama-runner-linux (fp32, buck2, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 007eed95632dd1c485687bc868cd04fc48325219d6188733b822da5063d2e75a /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t 4d9cd537001a4281f6ced99e9e4d1b82e2f15243a8722a7d147ae8ea654dc6be /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t e87aeea7e2272c772de0142480e548a1e08ad44fd9ec623e09327a94bed7c214 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 986b1ffb3f6cce96b12fc13e166efed015bb9412ee8e3d1d3b3011e92ac0f20e /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t b337d3e4bc07c394f7ddf0b038da21e5ad4cbb2789994da3406a0d5d044bae04 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t a05270c98de59ed963537b8cb166a46757ab345399db5dc74162bb04355bccb1 /exec failed with exit code 1
• pull / test-llama-runner-qnn-linux (fp32, cmake, qnn) / linux-job (gh)
  RuntimeError: Command docker exec -t f03d94b2132bfd7bb7a31b4b2e4c15c213d154fdad789e9ccb4726366a4152db /exec failed with exit code 1

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

[kirklandsign]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

[python3kgae]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

I didn't really do any real tests except make sure the getting-started-setup works under native Windows and WSL.
Will the CI pipeline test cover all tests or there're some tests must be running locally?

And yes, install_requirements.ps1 is for windows only.

[dbort]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

[dbort]

This shouldn't be necessary since platform/compiler.h has the same logic.

If this needed to happen because one of the headers between here and line 29 refer to ssize_t, then the header that needs it should also include compiler.h.

But in general, compiler.h is the only place in the ET code that is allowed to have system-specific checks like #ifdef _WIN32

[dbort]

Please add a comment explaining what this does, and why it's only necessary on windows. If it would be safe to use this flag on all systems, I'd rather remove the nt check.

[dbort]

Rather than duplicating this logic, please hoist it (and the other example near line 435) into a common global function like get_build_type() -> str

[python3kgae]

Added get_build_type.

[dbort]

I'd rather extend BuiltFile to handle this situation in a common way, instead of manually repeating the pattern for every entry.

E.g., it could take an extra is_executable: bool = False param that could be set to true here, letting it know to add the .exe suffix on windows.

And we could embed a CMAKE_BUILD_TYPE (debug vs release) placeholder in the paths that is ignored on non-windows systems. How about

third-party/flatbuffers/%BUILD_TYPE%/flatc

Then, on windows systems we can replace %BUILD_TYPE% with get_build_type(), and on non-windows systems we can delete the string %BUILD_TYPE%/ from the path if present.

So this this entry could become

 ext_modules.append(
BuiltFile(
"third-party/flatbuffers/%BUILD_TYPE%/flatc",
"executorch/data/bin/",
is_executable=True,
)
)

And entries below can use the %BUILD_TYPE% placeholder, and use .* to match .so/.dll/.dylib on different systems.

[python3kgae]

Updated.

[dbort]

I really want to avoid _WIN32 checks in any of the code, but this is reasonable for now. The best solution would be to avoid using POSIX concepts and to just use the standard C APIs.

Suggested change

	#ifndef _WIN32
	// TODO: Rewrite FileDataLoader to use standard C FILE* instead of POSIX file
	// descriptors, so that we don't need this _WIN32 check.
	#ifndef _WIN32

[python3kgae]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

Thanks for the review.
To avoid duplicating install_requirements.sh, I’m considering rewriting it in portable Python. I’ll give it a try to see if it works.

I don’t have access to Linux or macOS, so I’ll need help testing it on those platforms once I’ve got install_requirements.py working

[python3kgae]

Updated based on the comments.
Tested install_requirements.sh on WSL and install_requirements.bat on Windows.
Need help to test Linux and Mac.

[dbort]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

[dbort]

Comments should begin with a capital letter. And I suggest making this a little more general.

Suggested change

	# avoid def flat.exe() on windows.
	# Remove .exe suffix on windows.

[dbort]

Heads up that #4760 merged recently, which changes this class to use pread(), which I think will break windows. It might make sense to write a separate WindowsFileDataLoader.

There would be a lot of duplicated code, though. If we needed to split into a windows-specific data loader, it might make sense to have an AbstractFileDataLoader with one pure-virtual protected method to do the actual read.

[dbort]

This comment repeats what the code does, and doesn't provide extra value. Either remove it, or (ideally) add a comment explaining why this logic is necessary. It'd also be helpful to explain what ClangCL is, since non-windows devs will not be familiar with it.

[dbort]

Your PR actually fixes the TODO, so we can remove it.

Suggested change

	#ifndef_WIN32
	#include<sys/types.h>// TODO(T126923429): Include size_t, ssize_t
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif
	// Define size_t and ssize_t.
	#ifndef_WIN32
	#include<sys/types.h>
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif

[dbort]

Please add a comment explaining why this is necessary.

[dbort]

Now that there are more than two params, please use kwargs for all params. Same for other BuiltFile uses below.

Suggested change

	"third-party/flatbuffers/%BUILD_TYPE%/",
	"flatc",
	"executorch/data/bin/",
	src_path="third-party/flatbuffers/%BUILD_TYPE%/",
	src_name="flatc",
	dst="executorch/data/bin/",

[dbort]

Please call this src_dir since it's specifically a directory. src_path sounds like it could still refer to the path to the source file.

[dbort]

Will %BUILD_TYPE% always be the final component of the directory path? If so, we could remove the %BUILD_TYPE% placeholder and just add it to the source path inside BuiltFile.

It's ok as-is for now, though.

[python3kgae]

It is, but it does not have access to the installer in the constructor of BuiltFile.
If I understand correctly, obtaining the build configuration requires access to installer.debug.

[dbort]

Please mention %BUILD_TYPE% here and how to use it.

[python3kgae]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

Created 7 small PRs. Need more time for FileDataLoader.

[python3kgae]

Close after split to small PRs.

[python3kgae]

Add install_requirements.ps1 for build on native Windows.

Also fix issues block build on native Windows.

1. Get the Windows version of buck2 for Windows.
2. Check the path based on the build configuration for Windows in setup.py.
3. Avoid generating 'def flat.exe()' which is illegal python.
4. Add the Windows version of kernel_link_options.
5. Add links of pthreadpool and cpuinfo for custom_ops_aot_lib.
6. Define ssize_t for Windows.

Still have one link issue tracked with #4659.

For #4661

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

❌ 7 New Failures

As of commit 1b8899c with merge base f887d72 ():

NEW FAILURES - The following jobs have failed:

• pull / test-llama-runner-linux (fp32, buck2, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 007eed95632dd1c485687bc868cd04fc48325219d6188733b822da5063d2e75a /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t 4d9cd537001a4281f6ced99e9e4d1b82e2f15243a8722a7d147ae8ea654dc6be /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t e87aeea7e2272c772de0142480e548a1e08ad44fd9ec623e09327a94bed7c214 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 986b1ffb3f6cce96b12fc13e166efed015bb9412ee8e3d1d3b3011e92ac0f20e /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t b337d3e4bc07c394f7ddf0b038da21e5ad4cbb2789994da3406a0d5d044bae04 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t a05270c98de59ed963537b8cb166a46757ab345399db5dc74162bb04355bccb1 /exec failed with exit code 1
• pull / test-llama-runner-qnn-linux (fp32, cmake, qnn) / linux-job (gh)
  RuntimeError: Command docker exec -t f03d94b2132bfd7bb7a31b4b2e4c15c213d154fdad789e9ccb4726366a4152db /exec failed with exit code 1

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

[kirklandsign]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

[python3kgae]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

I didn't really do any real tests except make sure the getting-started-setup works under native Windows and WSL.
Will the CI pipeline test cover all tests or there're some tests must be running locally?

And yes, install_requirements.ps1 is for windows only.

[dbort]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

[dbort]

This shouldn't be necessary since platform/compiler.h has the same logic.

If this needed to happen because one of the headers between here and line 29 refer to ssize_t, then the header that needs it should also include compiler.h.

But in general, compiler.h is the only place in the ET code that is allowed to have system-specific checks like #ifdef _WIN32

[dbort]

Please add a comment explaining what this does, and why it's only necessary on windows. If it would be safe to use this flag on all systems, I'd rather remove the nt check.

[dbort]

Rather than duplicating this logic, please hoist it (and the other example near line 435) into a common global function like get_build_type() -> str

[python3kgae]

Added get_build_type.

[dbort]

I'd rather extend BuiltFile to handle this situation in a common way, instead of manually repeating the pattern for every entry.

E.g., it could take an extra is_executable: bool = False param that could be set to true here, letting it know to add the .exe suffix on windows.

And we could embed a CMAKE_BUILD_TYPE (debug vs release) placeholder in the paths that is ignored on non-windows systems. How about

third-party/flatbuffers/%BUILD_TYPE%/flatc

Then, on windows systems we can replace %BUILD_TYPE% with get_build_type(), and on non-windows systems we can delete the string %BUILD_TYPE%/ from the path if present.

So this this entry could become

 ext_modules.append(
BuiltFile(
"third-party/flatbuffers/%BUILD_TYPE%/flatc",
"executorch/data/bin/",
is_executable=True,
)
)

And entries below can use the %BUILD_TYPE% placeholder, and use .* to match .so/.dll/.dylib on different systems.

[python3kgae]

Updated.

[dbort]

I really want to avoid _WIN32 checks in any of the code, but this is reasonable for now. The best solution would be to avoid using POSIX concepts and to just use the standard C APIs.

Suggested change

	#ifndef _WIN32
	// TODO: Rewrite FileDataLoader to use standard C FILE* instead of POSIX file
	// descriptors, so that we don't need this _WIN32 check.
	#ifndef _WIN32

[python3kgae]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

Thanks for the review.
To avoid duplicating install_requirements.sh, I’m considering rewriting it in portable Python. I’ll give it a try to see if it works.

I don’t have access to Linux or macOS, so I’ll need help testing it on those platforms once I’ve got install_requirements.py working

[python3kgae]

Updated based on the comments.
Tested install_requirements.sh on WSL and install_requirements.bat on Windows.
Need help to test Linux and Mac.

[dbort]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

[dbort]

Comments should begin with a capital letter. And I suggest making this a little more general.

Suggested change

	# avoid def flat.exe() on windows.
	# Remove .exe suffix on windows.

[dbort]

Heads up that #4760 merged recently, which changes this class to use pread(), which I think will break windows. It might make sense to write a separate WindowsFileDataLoader.

There would be a lot of duplicated code, though. If we needed to split into a windows-specific data loader, it might make sense to have an AbstractFileDataLoader with one pure-virtual protected method to do the actual read.

[dbort]

This comment repeats what the code does, and doesn't provide extra value. Either remove it, or (ideally) add a comment explaining why this logic is necessary. It'd also be helpful to explain what ClangCL is, since non-windows devs will not be familiar with it.

[dbort]

Your PR actually fixes the TODO, so we can remove it.

Suggested change

	#ifndef_WIN32
	#include<sys/types.h>// TODO(T126923429): Include size_t, ssize_t
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif
	// Define size_t and ssize_t.
	#ifndef_WIN32
	#include<sys/types.h>
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif

[dbort]

Please add a comment explaining why this is necessary.

[dbort]

Now that there are more than two params, please use kwargs for all params. Same for other BuiltFile uses below.

Suggested change

	"third-party/flatbuffers/%BUILD_TYPE%/",
	"flatc",
	"executorch/data/bin/",
	src_path="third-party/flatbuffers/%BUILD_TYPE%/",
	src_name="flatc",
	dst="executorch/data/bin/",

[dbort]

Please call this src_dir since it's specifically a directory. src_path sounds like it could still refer to the path to the source file.

[dbort]

Will %BUILD_TYPE% always be the final component of the directory path? If so, we could remove the %BUILD_TYPE% placeholder and just add it to the source path inside BuiltFile.

It's ok as-is for now, though.

[python3kgae]

It is, but it does not have access to the installer in the constructor of BuiltFile.
If I understand correctly, obtaining the build configuration requires access to installer.debug.

[dbort]

Please mention %BUILD_TYPE% here and how to use it.

[python3kgae]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

Created 7 small PRs. Need more time for FileDataLoader.

[python3kgae]

Close after split to small PRs.

[python3kgae]

Add install_requirements.ps1 for build on native Windows.

Also fix issues block build on native Windows.

1. Get the Windows version of buck2 for Windows.
2. Check the path based on the build configuration for Windows in setup.py.
3. Avoid generating 'def flat.exe()' which is illegal python.
4. Add the Windows version of kernel_link_options.
5. Add links of pthreadpool and cpuinfo for custom_ops_aot_lib.
6. Define ssize_t for Windows.

Still have one link issue tracked with #4659.

For #4661

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

❌ 7 New Failures

As of commit 1b8899c with merge base f887d72 ():

NEW FAILURES - The following jobs have failed:

• pull / test-llama-runner-linux (fp32, buck2, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 007eed95632dd1c485687bc868cd04fc48325219d6188733b822da5063d2e75a /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t 4d9cd537001a4281f6ced99e9e4d1b82e2f15243a8722a7d147ae8ea654dc6be /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t e87aeea7e2272c772de0142480e548a1e08ad44fd9ec623e09327a94bed7c214 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 986b1ffb3f6cce96b12fc13e166efed015bb9412ee8e3d1d3b3011e92ac0f20e /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t b337d3e4bc07c394f7ddf0b038da21e5ad4cbb2789994da3406a0d5d044bae04 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t a05270c98de59ed963537b8cb166a46757ab345399db5dc74162bb04355bccb1 /exec failed with exit code 1
• pull / test-llama-runner-qnn-linux (fp32, cmake, qnn) / linux-job (gh)
  RuntimeError: Command docker exec -t f03d94b2132bfd7bb7a31b4b2e4c15c213d154fdad789e9ccb4726366a4152db /exec failed with exit code 1

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

[kirklandsign]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

[python3kgae]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

I didn't really do any real tests except make sure the getting-started-setup works under native Windows and WSL.
Will the CI pipeline test cover all tests or there're some tests must be running locally?

And yes, install_requirements.ps1 is for windows only.

[dbort]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

[dbort]

This shouldn't be necessary since platform/compiler.h has the same logic.

If this needed to happen because one of the headers between here and line 29 refer to ssize_t, then the header that needs it should also include compiler.h.

But in general, compiler.h is the only place in the ET code that is allowed to have system-specific checks like #ifdef _WIN32

[dbort]

Please add a comment explaining what this does, and why it's only necessary on windows. If it would be safe to use this flag on all systems, I'd rather remove the nt check.

[dbort]

Rather than duplicating this logic, please hoist it (and the other example near line 435) into a common global function like get_build_type() -> str

[python3kgae]

Added get_build_type.

[dbort]

I'd rather extend BuiltFile to handle this situation in a common way, instead of manually repeating the pattern for every entry.

E.g., it could take an extra is_executable: bool = False param that could be set to true here, letting it know to add the .exe suffix on windows.

And we could embed a CMAKE_BUILD_TYPE (debug vs release) placeholder in the paths that is ignored on non-windows systems. How about

third-party/flatbuffers/%BUILD_TYPE%/flatc

Then, on windows systems we can replace %BUILD_TYPE% with get_build_type(), and on non-windows systems we can delete the string %BUILD_TYPE%/ from the path if present.

So this this entry could become

 ext_modules.append(
BuiltFile(
"third-party/flatbuffers/%BUILD_TYPE%/flatc",
"executorch/data/bin/",
is_executable=True,
)
)

And entries below can use the %BUILD_TYPE% placeholder, and use .* to match .so/.dll/.dylib on different systems.

[python3kgae]

Updated.

[dbort]

I really want to avoid _WIN32 checks in any of the code, but this is reasonable for now. The best solution would be to avoid using POSIX concepts and to just use the standard C APIs.

Suggested change

	#ifndef _WIN32
	// TODO: Rewrite FileDataLoader to use standard C FILE* instead of POSIX file
	// descriptors, so that we don't need this _WIN32 check.
	#ifndef _WIN32

[python3kgae]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

Thanks for the review.
To avoid duplicating install_requirements.sh, I’m considering rewriting it in portable Python. I’ll give it a try to see if it works.

I don’t have access to Linux or macOS, so I’ll need help testing it on those platforms once I’ve got install_requirements.py working

[python3kgae]

Updated based on the comments.
Tested install_requirements.sh on WSL and install_requirements.bat on Windows.
Need help to test Linux and Mac.

[dbort]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

[dbort]

Comments should begin with a capital letter. And I suggest making this a little more general.

Suggested change

	# avoid def flat.exe() on windows.
	# Remove .exe suffix on windows.

[dbort]

Heads up that #4760 merged recently, which changes this class to use pread(), which I think will break windows. It might make sense to write a separate WindowsFileDataLoader.

There would be a lot of duplicated code, though. If we needed to split into a windows-specific data loader, it might make sense to have an AbstractFileDataLoader with one pure-virtual protected method to do the actual read.

[dbort]

This comment repeats what the code does, and doesn't provide extra value. Either remove it, or (ideally) add a comment explaining why this logic is necessary. It'd also be helpful to explain what ClangCL is, since non-windows devs will not be familiar with it.

[dbort]

Your PR actually fixes the TODO, so we can remove it.

Suggested change

	#ifndef_WIN32
	#include<sys/types.h>// TODO(T126923429): Include size_t, ssize_t
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif
	// Define size_t and ssize_t.
	#ifndef_WIN32
	#include<sys/types.h>
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif

[dbort]

Please add a comment explaining why this is necessary.

[dbort]

Now that there are more than two params, please use kwargs for all params. Same for other BuiltFile uses below.

Suggested change

	"third-party/flatbuffers/%BUILD_TYPE%/",
	"flatc",
	"executorch/data/bin/",
	src_path="third-party/flatbuffers/%BUILD_TYPE%/",
	src_name="flatc",
	dst="executorch/data/bin/",

[dbort]

Please call this src_dir since it's specifically a directory. src_path sounds like it could still refer to the path to the source file.

[dbort]

Will %BUILD_TYPE% always be the final component of the directory path? If so, we could remove the %BUILD_TYPE% placeholder and just add it to the source path inside BuiltFile.

It's ok as-is for now, though.

[python3kgae]

It is, but it does not have access to the installer in the constructor of BuiltFile.
If I understand correctly, obtaining the build configuration requires access to installer.debug.

[dbort]

Please mention %BUILD_TYPE% here and how to use it.

[python3kgae]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

Created 7 small PRs. Need more time for FileDataLoader.

[python3kgae]

Close after split to small PRs.

[python3kgae]

Add install_requirements.ps1 for build on native Windows.

Also fix issues block build on native Windows.

1. Get the Windows version of buck2 for Windows.
2. Check the path based on the build configuration for Windows in setup.py.
3. Avoid generating 'def flat.exe()' which is illegal python.
4. Add the Windows version of kernel_link_options.
5. Add links of pthreadpool and cpuinfo for custom_ops_aot_lib.
6. Define ssize_t for Windows.

Still have one link issue tracked with #4659.

For #4661

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

❌ 7 New Failures

As of commit 1b8899c with merge base f887d72 ():

NEW FAILURES - The following jobs have failed:

• pull / test-llama-runner-linux (fp32, buck2, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 007eed95632dd1c485687bc868cd04fc48325219d6188733b822da5063d2e75a /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t 4d9cd537001a4281f6ced99e9e4d1b82e2f15243a8722a7d147ae8ea654dc6be /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t e87aeea7e2272c772de0142480e548a1e08ad44fd9ec623e09327a94bed7c214 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 986b1ffb3f6cce96b12fc13e166efed015bb9412ee8e3d1d3b3011e92ac0f20e /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t b337d3e4bc07c394f7ddf0b038da21e5ad4cbb2789994da3406a0d5d044bae04 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t a05270c98de59ed963537b8cb166a46757ab345399db5dc74162bb04355bccb1 /exec failed with exit code 1
• pull / test-llama-runner-qnn-linux (fp32, cmake, qnn) / linux-job (gh)
  RuntimeError: Command docker exec -t f03d94b2132bfd7bb7a31b4b2e4c15c213d154fdad789e9ccb4726366a4152db /exec failed with exit code 1

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

[kirklandsign]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

[python3kgae]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

I didn't really do any real tests except make sure the getting-started-setup works under native Windows and WSL.
Will the CI pipeline test cover all tests or there're some tests must be running locally?

And yes, install_requirements.ps1 is for windows only.

[dbort]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

[dbort]

This shouldn't be necessary since platform/compiler.h has the same logic.

If this needed to happen because one of the headers between here and line 29 refer to ssize_t, then the header that needs it should also include compiler.h.

But in general, compiler.h is the only place in the ET code that is allowed to have system-specific checks like #ifdef _WIN32

[dbort]

Please add a comment explaining what this does, and why it's only necessary on windows. If it would be safe to use this flag on all systems, I'd rather remove the nt check.

[dbort]

Rather than duplicating this logic, please hoist it (and the other example near line 435) into a common global function like get_build_type() -> str

[python3kgae]

Added get_build_type.

[dbort]

I'd rather extend BuiltFile to handle this situation in a common way, instead of manually repeating the pattern for every entry.

E.g., it could take an extra is_executable: bool = False param that could be set to true here, letting it know to add the .exe suffix on windows.

And we could embed a CMAKE_BUILD_TYPE (debug vs release) placeholder in the paths that is ignored on non-windows systems. How about

third-party/flatbuffers/%BUILD_TYPE%/flatc

Then, on windows systems we can replace %BUILD_TYPE% with get_build_type(), and on non-windows systems we can delete the string %BUILD_TYPE%/ from the path if present.

So this this entry could become

 ext_modules.append(
BuiltFile(
"third-party/flatbuffers/%BUILD_TYPE%/flatc",
"executorch/data/bin/",
is_executable=True,
)
)

And entries below can use the %BUILD_TYPE% placeholder, and use .* to match .so/.dll/.dylib on different systems.

[python3kgae]

Updated.

[dbort]

I really want to avoid _WIN32 checks in any of the code, but this is reasonable for now. The best solution would be to avoid using POSIX concepts and to just use the standard C APIs.

Suggested change

	#ifndef _WIN32
	// TODO: Rewrite FileDataLoader to use standard C FILE* instead of POSIX file
	// descriptors, so that we don't need this _WIN32 check.
	#ifndef _WIN32

[python3kgae]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

Thanks for the review.
To avoid duplicating install_requirements.sh, I’m considering rewriting it in portable Python. I’ll give it a try to see if it works.

I don’t have access to Linux or macOS, so I’ll need help testing it on those platforms once I’ve got install_requirements.py working

[python3kgae]

Updated based on the comments.
Tested install_requirements.sh on WSL and install_requirements.bat on Windows.
Need help to test Linux and Mac.

[dbort]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

[dbort]

Comments should begin with a capital letter. And I suggest making this a little more general.

Suggested change

	# avoid def flat.exe() on windows.
	# Remove .exe suffix on windows.

[dbort]

Heads up that #4760 merged recently, which changes this class to use pread(), which I think will break windows. It might make sense to write a separate WindowsFileDataLoader.

There would be a lot of duplicated code, though. If we needed to split into a windows-specific data loader, it might make sense to have an AbstractFileDataLoader with one pure-virtual protected method to do the actual read.

[dbort]

This comment repeats what the code does, and doesn't provide extra value. Either remove it, or (ideally) add a comment explaining why this logic is necessary. It'd also be helpful to explain what ClangCL is, since non-windows devs will not be familiar with it.

[dbort]

Your PR actually fixes the TODO, so we can remove it.

Suggested change

	#ifndef_WIN32
	#include<sys/types.h>// TODO(T126923429): Include size_t, ssize_t
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif
	// Define size_t and ssize_t.
	#ifndef_WIN32
	#include<sys/types.h>
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif

[dbort]

Please add a comment explaining why this is necessary.

[dbort]

Now that there are more than two params, please use kwargs for all params. Same for other BuiltFile uses below.

Suggested change

	"third-party/flatbuffers/%BUILD_TYPE%/",
	"flatc",
	"executorch/data/bin/",
	src_path="third-party/flatbuffers/%BUILD_TYPE%/",
	src_name="flatc",
	dst="executorch/data/bin/",

[dbort]

Please call this src_dir since it's specifically a directory. src_path sounds like it could still refer to the path to the source file.

[dbort]

Will %BUILD_TYPE% always be the final component of the directory path? If so, we could remove the %BUILD_TYPE% placeholder and just add it to the source path inside BuiltFile.

It's ok as-is for now, though.

[python3kgae]

It is, but it does not have access to the installer in the constructor of BuiltFile.
If I understand correctly, obtaining the build configuration requires access to installer.debug.

[dbort]

Please mention %BUILD_TYPE% here and how to use it.

[python3kgae]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

Created 7 small PRs. Need more time for FileDataLoader.

[python3kgae]

Close after split to small PRs.

[python3kgae]

Add install_requirements.ps1 for build on native Windows.

Also fix issues block build on native Windows.

1. Get the Windows version of buck2 for Windows.
2. Check the path based on the build configuration for Windows in setup.py.
3. Avoid generating 'def flat.exe()' which is illegal python.
4. Add the Windows version of kernel_link_options.
5. Add links of pthreadpool and cpuinfo for custom_ops_aot_lib.
6. Define ssize_t for Windows.

Still have one link issue tracked with #4659.

For #4661

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

❌ 7 New Failures

As of commit 1b8899c with merge base f887d72 ():

NEW FAILURES - The following jobs have failed:

• pull / test-llama-runner-linux (fp32, buck2, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 007eed95632dd1c485687bc868cd04fc48325219d6188733b822da5063d2e75a /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t 4d9cd537001a4281f6ced99e9e4d1b82e2f15243a8722a7d147ae8ea654dc6be /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t e87aeea7e2272c772de0142480e548a1e08ad44fd9ec623e09327a94bed7c214 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 986b1ffb3f6cce96b12fc13e166efed015bb9412ee8e3d1d3b3011e92ac0f20e /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t b337d3e4bc07c394f7ddf0b038da21e5ad4cbb2789994da3406a0d5d044bae04 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t a05270c98de59ed963537b8cb166a46757ab345399db5dc74162bb04355bccb1 /exec failed with exit code 1
• pull / test-llama-runner-qnn-linux (fp32, cmake, qnn) / linux-job (gh)
  RuntimeError: Command docker exec -t f03d94b2132bfd7bb7a31b4b2e4c15c213d154fdad789e9ccb4726366a4152db /exec failed with exit code 1

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

[kirklandsign]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

[python3kgae]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

I didn't really do any real tests except make sure the getting-started-setup works under native Windows and WSL.
Will the CI pipeline test cover all tests or there're some tests must be running locally?

And yes, install_requirements.ps1 is for windows only.

[dbort]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

[dbort]

This shouldn't be necessary since platform/compiler.h has the same logic.

If this needed to happen because one of the headers between here and line 29 refer to ssize_t, then the header that needs it should also include compiler.h.

But in general, compiler.h is the only place in the ET code that is allowed to have system-specific checks like #ifdef _WIN32

[dbort]

Please add a comment explaining what this does, and why it's only necessary on windows. If it would be safe to use this flag on all systems, I'd rather remove the nt check.

[dbort]

Rather than duplicating this logic, please hoist it (and the other example near line 435) into a common global function like get_build_type() -> str

[python3kgae]

Added get_build_type.

[dbort]

I'd rather extend BuiltFile to handle this situation in a common way, instead of manually repeating the pattern for every entry.

E.g., it could take an extra is_executable: bool = False param that could be set to true here, letting it know to add the .exe suffix on windows.

And we could embed a CMAKE_BUILD_TYPE (debug vs release) placeholder in the paths that is ignored on non-windows systems. How about

third-party/flatbuffers/%BUILD_TYPE%/flatc

Then, on windows systems we can replace %BUILD_TYPE% with get_build_type(), and on non-windows systems we can delete the string %BUILD_TYPE%/ from the path if present.

So this this entry could become

 ext_modules.append(
BuiltFile(
"third-party/flatbuffers/%BUILD_TYPE%/flatc",
"executorch/data/bin/",
is_executable=True,
)
)

And entries below can use the %BUILD_TYPE% placeholder, and use .* to match .so/.dll/.dylib on different systems.

[python3kgae]

Updated.

[dbort]

I really want to avoid _WIN32 checks in any of the code, but this is reasonable for now. The best solution would be to avoid using POSIX concepts and to just use the standard C APIs.

Suggested change

	#ifndef _WIN32
	// TODO: Rewrite FileDataLoader to use standard C FILE* instead of POSIX file
	// descriptors, so that we don't need this _WIN32 check.
	#ifndef _WIN32

[python3kgae]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

Thanks for the review.
To avoid duplicating install_requirements.sh, I’m considering rewriting it in portable Python. I’ll give it a try to see if it works.

I don’t have access to Linux or macOS, so I’ll need help testing it on those platforms once I’ve got install_requirements.py working

[python3kgae]

Updated based on the comments.
Tested install_requirements.sh on WSL and install_requirements.bat on Windows.
Need help to test Linux and Mac.

[dbort]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

[dbort]

Comments should begin with a capital letter. And I suggest making this a little more general.

Suggested change

	# avoid def flat.exe() on windows.
	# Remove .exe suffix on windows.

[dbort]

Heads up that #4760 merged recently, which changes this class to use pread(), which I think will break windows. It might make sense to write a separate WindowsFileDataLoader.

There would be a lot of duplicated code, though. If we needed to split into a windows-specific data loader, it might make sense to have an AbstractFileDataLoader with one pure-virtual protected method to do the actual read.

[dbort]

This comment repeats what the code does, and doesn't provide extra value. Either remove it, or (ideally) add a comment explaining why this logic is necessary. It'd also be helpful to explain what ClangCL is, since non-windows devs will not be familiar with it.

[dbort]

Your PR actually fixes the TODO, so we can remove it.

Suggested change

	#ifndef_WIN32
	#include<sys/types.h>// TODO(T126923429): Include size_t, ssize_t
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif
	// Define size_t and ssize_t.
	#ifndef_WIN32
	#include<sys/types.h>
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif

[dbort]

Please add a comment explaining why this is necessary.

[dbort]

Now that there are more than two params, please use kwargs for all params. Same for other BuiltFile uses below.

Suggested change

	"third-party/flatbuffers/%BUILD_TYPE%/",
	"flatc",
	"executorch/data/bin/",
	src_path="third-party/flatbuffers/%BUILD_TYPE%/",
	src_name="flatc",
	dst="executorch/data/bin/",

[dbort]

Please call this src_dir since it's specifically a directory. src_path sounds like it could still refer to the path to the source file.

[dbort]

Will %BUILD_TYPE% always be the final component of the directory path? If so, we could remove the %BUILD_TYPE% placeholder and just add it to the source path inside BuiltFile.

It's ok as-is for now, though.

[python3kgae]

It is, but it does not have access to the installer in the constructor of BuiltFile.
If I understand correctly, obtaining the build configuration requires access to installer.debug.

[dbort]

Please mention %BUILD_TYPE% here and how to use it.

[python3kgae]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

Created 7 small PRs. Need more time for FileDataLoader.

[python3kgae]

Close after split to small PRs.

[python3kgae]

Add install_requirements.ps1 for build on native Windows.

Also fix issues block build on native Windows.

1. Get the Windows version of buck2 for Windows.
2. Check the path based on the build configuration for Windows in setup.py.
3. Avoid generating 'def flat.exe()' which is illegal python.
4. Add the Windows version of kernel_link_options.
5. Add links of pthreadpool and cpuinfo for custom_ops_aot_lib.
6. Define ssize_t for Windows.

Still have one link issue tracked with #4659.

For #4661

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

❌ 7 New Failures

As of commit 1b8899c with merge base f887d72 ():

NEW FAILURES - The following jobs have failed:

• pull / test-llama-runner-linux (fp32, buck2, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 007eed95632dd1c485687bc868cd04fc48325219d6188733b822da5063d2e75a /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t 4d9cd537001a4281f6ced99e9e4d1b82e2f15243a8722a7d147ae8ea654dc6be /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t e87aeea7e2272c772de0142480e548a1e08ad44fd9ec623e09327a94bed7c214 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 986b1ffb3f6cce96b12fc13e166efed015bb9412ee8e3d1d3b3011e92ac0f20e /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t b337d3e4bc07c394f7ddf0b038da21e5ad4cbb2789994da3406a0d5d044bae04 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t a05270c98de59ed963537b8cb166a46757ab345399db5dc74162bb04355bccb1 /exec failed with exit code 1
• pull / test-llama-runner-qnn-linux (fp32, cmake, qnn) / linux-job (gh)
  RuntimeError: Command docker exec -t f03d94b2132bfd7bb7a31b4b2e4c15c213d154fdad789e9ccb4726366a4152db /exec failed with exit code 1

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

[kirklandsign]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

[python3kgae]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

I didn't really do any real tests except make sure the getting-started-setup works under native Windows and WSL.
Will the CI pipeline test cover all tests or there're some tests must be running locally?

And yes, install_requirements.ps1 is for windows only.

[dbort]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

[dbort]

This shouldn't be necessary since platform/compiler.h has the same logic.

If this needed to happen because one of the headers between here and line 29 refer to ssize_t, then the header that needs it should also include compiler.h.

But in general, compiler.h is the only place in the ET code that is allowed to have system-specific checks like #ifdef _WIN32

[dbort]

Please add a comment explaining what this does, and why it's only necessary on windows. If it would be safe to use this flag on all systems, I'd rather remove the nt check.

[dbort]

Rather than duplicating this logic, please hoist it (and the other example near line 435) into a common global function like get_build_type() -> str

[python3kgae]

Added get_build_type.

[dbort]

I'd rather extend BuiltFile to handle this situation in a common way, instead of manually repeating the pattern for every entry.

E.g., it could take an extra is_executable: bool = False param that could be set to true here, letting it know to add the .exe suffix on windows.

And we could embed a CMAKE_BUILD_TYPE (debug vs release) placeholder in the paths that is ignored on non-windows systems. How about

third-party/flatbuffers/%BUILD_TYPE%/flatc

Then, on windows systems we can replace %BUILD_TYPE% with get_build_type(), and on non-windows systems we can delete the string %BUILD_TYPE%/ from the path if present.

So this this entry could become

 ext_modules.append(
BuiltFile(
"third-party/flatbuffers/%BUILD_TYPE%/flatc",
"executorch/data/bin/",
is_executable=True,
)
)

And entries below can use the %BUILD_TYPE% placeholder, and use .* to match .so/.dll/.dylib on different systems.

[python3kgae]

Updated.

[dbort]

I really want to avoid _WIN32 checks in any of the code, but this is reasonable for now. The best solution would be to avoid using POSIX concepts and to just use the standard C APIs.

Suggested change

	#ifndef _WIN32
	// TODO: Rewrite FileDataLoader to use standard C FILE* instead of POSIX file
	// descriptors, so that we don't need this _WIN32 check.
	#ifndef _WIN32

[python3kgae]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

Thanks for the review.
To avoid duplicating install_requirements.sh, I’m considering rewriting it in portable Python. I’ll give it a try to see if it works.

I don’t have access to Linux or macOS, so I’ll need help testing it on those platforms once I’ve got install_requirements.py working

[python3kgae]

Updated based on the comments.
Tested install_requirements.sh on WSL and install_requirements.bat on Windows.
Need help to test Linux and Mac.

[dbort]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

[dbort]

Comments should begin with a capital letter. And I suggest making this a little more general.

Suggested change

	# avoid def flat.exe() on windows.
	# Remove .exe suffix on windows.

[dbort]

Heads up that #4760 merged recently, which changes this class to use pread(), which I think will break windows. It might make sense to write a separate WindowsFileDataLoader.

There would be a lot of duplicated code, though. If we needed to split into a windows-specific data loader, it might make sense to have an AbstractFileDataLoader with one pure-virtual protected method to do the actual read.

[dbort]

This comment repeats what the code does, and doesn't provide extra value. Either remove it, or (ideally) add a comment explaining why this logic is necessary. It'd also be helpful to explain what ClangCL is, since non-windows devs will not be familiar with it.

[dbort]

Your PR actually fixes the TODO, so we can remove it.

Suggested change

	#ifndef_WIN32
	#include<sys/types.h>// TODO(T126923429): Include size_t, ssize_t
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif
	// Define size_t and ssize_t.
	#ifndef_WIN32
	#include<sys/types.h>
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif

[dbort]

Please add a comment explaining why this is necessary.

[dbort]

Now that there are more than two params, please use kwargs for all params. Same for other BuiltFile uses below.

Suggested change

	"third-party/flatbuffers/%BUILD_TYPE%/",
	"flatc",
	"executorch/data/bin/",
	src_path="third-party/flatbuffers/%BUILD_TYPE%/",
	src_name="flatc",
	dst="executorch/data/bin/",

[dbort]

Please call this src_dir since it's specifically a directory. src_path sounds like it could still refer to the path to the source file.

[dbort]

Will %BUILD_TYPE% always be the final component of the directory path? If so, we could remove the %BUILD_TYPE% placeholder and just add it to the source path inside BuiltFile.

It's ok as-is for now, though.

[python3kgae]

It is, but it does not have access to the installer in the constructor of BuiltFile.
If I understand correctly, obtaining the build configuration requires access to installer.debug.

[dbort]

Please mention %BUILD_TYPE% here and how to use it.

[python3kgae]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

Created 7 small PRs. Need more time for FileDataLoader.

[python3kgae]

Close after split to small PRs.

[python3kgae]

Add install_requirements.ps1 for build on native Windows.

Also fix issues block build on native Windows.

1. Get the Windows version of buck2 for Windows.
2. Check the path based on the build configuration for Windows in setup.py.
3. Avoid generating 'def flat.exe()' which is illegal python.
4. Add the Windows version of kernel_link_options.
5. Add links of pthreadpool and cpuinfo for custom_ops_aot_lib.
6. Define ssize_t for Windows.

Still have one link issue tracked with #4659.

For #4661

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

❌ 7 New Failures

As of commit 1b8899c with merge base f887d72 ():

NEW FAILURES - The following jobs have failed:

• pull / test-llama-runner-linux (fp32, buck2, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 007eed95632dd1c485687bc868cd04fc48325219d6188733b822da5063d2e75a /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t 4d9cd537001a4281f6ced99e9e4d1b82e2f15243a8722a7d147ae8ea654dc6be /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t e87aeea7e2272c772de0142480e548a1e08ad44fd9ec623e09327a94bed7c214 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 986b1ffb3f6cce96b12fc13e166efed015bb9412ee8e3d1d3b3011e92ac0f20e /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t b337d3e4bc07c394f7ddf0b038da21e5ad4cbb2789994da3406a0d5d044bae04 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t a05270c98de59ed963537b8cb166a46757ab345399db5dc74162bb04355bccb1 /exec failed with exit code 1
• pull / test-llama-runner-qnn-linux (fp32, cmake, qnn) / linux-job (gh)
  RuntimeError: Command docker exec -t f03d94b2132bfd7bb7a31b4b2e4c15c213d154fdad789e9ccb4726366a4152db /exec failed with exit code 1

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

[kirklandsign]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

[python3kgae]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

I didn't really do any real tests except make sure the getting-started-setup works under native Windows and WSL.
Will the CI pipeline test cover all tests or there're some tests must be running locally?

And yes, install_requirements.ps1 is for windows only.

[dbort]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

[dbort]

This shouldn't be necessary since platform/compiler.h has the same logic.

If this needed to happen because one of the headers between here and line 29 refer to ssize_t, then the header that needs it should also include compiler.h.

But in general, compiler.h is the only place in the ET code that is allowed to have system-specific checks like #ifdef _WIN32

[dbort]

Please add a comment explaining what this does, and why it's only necessary on windows. If it would be safe to use this flag on all systems, I'd rather remove the nt check.

[dbort]

Rather than duplicating this logic, please hoist it (and the other example near line 435) into a common global function like get_build_type() -> str

[python3kgae]

Added get_build_type.

[dbort]

I'd rather extend BuiltFile to handle this situation in a common way, instead of manually repeating the pattern for every entry.

E.g., it could take an extra is_executable: bool = False param that could be set to true here, letting it know to add the .exe suffix on windows.

And we could embed a CMAKE_BUILD_TYPE (debug vs release) placeholder in the paths that is ignored on non-windows systems. How about

third-party/flatbuffers/%BUILD_TYPE%/flatc

Then, on windows systems we can replace %BUILD_TYPE% with get_build_type(), and on non-windows systems we can delete the string %BUILD_TYPE%/ from the path if present.

So this this entry could become

 ext_modules.append(
BuiltFile(
"third-party/flatbuffers/%BUILD_TYPE%/flatc",
"executorch/data/bin/",
is_executable=True,
)
)

And entries below can use the %BUILD_TYPE% placeholder, and use .* to match .so/.dll/.dylib on different systems.

[python3kgae]

Updated.

[dbort]

I really want to avoid _WIN32 checks in any of the code, but this is reasonable for now. The best solution would be to avoid using POSIX concepts and to just use the standard C APIs.

Suggested change

	#ifndef _WIN32
	// TODO: Rewrite FileDataLoader to use standard C FILE* instead of POSIX file
	// descriptors, so that we don't need this _WIN32 check.
	#ifndef _WIN32

[python3kgae]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

Thanks for the review.
To avoid duplicating install_requirements.sh, I’m considering rewriting it in portable Python. I’ll give it a try to see if it works.

I don’t have access to Linux or macOS, so I’ll need help testing it on those platforms once I’ve got install_requirements.py working

[python3kgae]

Updated based on the comments.
Tested install_requirements.sh on WSL and install_requirements.bat on Windows.
Need help to test Linux and Mac.

[dbort]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

[dbort]

Comments should begin with a capital letter. And I suggest making this a little more general.

Suggested change

	# avoid def flat.exe() on windows.
	# Remove .exe suffix on windows.

[dbort]

Heads up that #4760 merged recently, which changes this class to use pread(), which I think will break windows. It might make sense to write a separate WindowsFileDataLoader.

There would be a lot of duplicated code, though. If we needed to split into a windows-specific data loader, it might make sense to have an AbstractFileDataLoader with one pure-virtual protected method to do the actual read.

[dbort]

This comment repeats what the code does, and doesn't provide extra value. Either remove it, or (ideally) add a comment explaining why this logic is necessary. It'd also be helpful to explain what ClangCL is, since non-windows devs will not be familiar with it.

[dbort]

Your PR actually fixes the TODO, so we can remove it.

Suggested change

	#ifndef_WIN32
	#include<sys/types.h>// TODO(T126923429): Include size_t, ssize_t
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif
	// Define size_t and ssize_t.
	#ifndef_WIN32
	#include<sys/types.h>
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif

[dbort]

Please add a comment explaining why this is necessary.

[dbort]

Now that there are more than two params, please use kwargs for all params. Same for other BuiltFile uses below.

Suggested change

	"third-party/flatbuffers/%BUILD_TYPE%/",
	"flatc",
	"executorch/data/bin/",
	src_path="third-party/flatbuffers/%BUILD_TYPE%/",
	src_name="flatc",
	dst="executorch/data/bin/",

[dbort]

Please call this src_dir since it's specifically a directory. src_path sounds like it could still refer to the path to the source file.

[dbort]

Will %BUILD_TYPE% always be the final component of the directory path? If so, we could remove the %BUILD_TYPE% placeholder and just add it to the source path inside BuiltFile.

It's ok as-is for now, though.

[python3kgae]

It is, but it does not have access to the installer in the constructor of BuiltFile.
If I understand correctly, obtaining the build configuration requires access to installer.debug.

[dbort]

Please mention %BUILD_TYPE% here and how to use it.

[python3kgae]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

Created 7 small PRs. Need more time for FileDataLoader.

[python3kgae]

Close after split to small PRs.

[python3kgae]

Add install_requirements.ps1 for build on native Windows.

Also fix issues block build on native Windows.

1. Get the Windows version of buck2 for Windows.
2. Check the path based on the build configuration for Windows in setup.py.
3. Avoid generating 'def flat.exe()' which is illegal python.
4. Add the Windows version of kernel_link_options.
5. Add links of pthreadpool and cpuinfo for custom_ops_aot_lib.
6. Define ssize_t for Windows.

Still have one link issue tracked with #4659.

For #4661

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

❌ 7 New Failures

As of commit 1b8899c with merge base f887d72 ():

NEW FAILURES - The following jobs have failed:

• pull / test-llama-runner-linux (fp32, buck2, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 007eed95632dd1c485687bc868cd04fc48325219d6188733b822da5063d2e75a /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t 4d9cd537001a4281f6ced99e9e4d1b82e2f15243a8722a7d147ae8ea654dc6be /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, buck2, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t e87aeea7e2272c772de0142480e548a1e08ad44fd9ec623e09327a94bed7c214 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, portable) / linux-job (gh)
  RuntimeError: Command docker exec -t 986b1ffb3f6cce96b12fc13e166efed015bb9412ee8e3d1d3b3011e92ac0f20e /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom) / linux-job (gh)
  RuntimeError: Command docker exec -t b337d3e4bc07c394f7ddf0b038da21e5ad4cbb2789994da3406a0d5d044bae04 /exec failed with exit code 1
• pull / test-llama-runner-linux (fp32, cmake, xnnpack+custom+qe) / linux-job (gh)
  RuntimeError: Command docker exec -t a05270c98de59ed963537b8cb166a46757ab345399db5dc74162bb04355bccb1 /exec failed with exit code 1
• pull / test-llama-runner-qnn-linux (fp32, cmake, qnn) / linux-job (gh)
  RuntimeError: Command docker exec -t f03d94b2132bfd7bb7a31b4b2e4c15c213d154fdad789e9ccb4726366a4152db /exec failed with exit code 1

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

[kirklandsign]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

[python3kgae]

Thank you! Is this PR tested and ready for review? Also for the install_requirements.ps1, it's for windows only?

I didn't really do any real tests except make sure the getting-started-setup works under native Windows and WSL.
Will the CI pipeline test cover all tests or there're some tests must be running locally?

And yes, install_requirements.ps1 is for windows only.

[dbort]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

[dbort]

This shouldn't be necessary since platform/compiler.h has the same logic.

If this needed to happen because one of the headers between here and line 29 refer to ssize_t, then the header that needs it should also include compiler.h.

But in general, compiler.h is the only place in the ET code that is allowed to have system-specific checks like #ifdef _WIN32

[dbort]

Please add a comment explaining what this does, and why it's only necessary on windows. If it would be safe to use this flag on all systems, I'd rather remove the nt check.

[dbort]

Rather than duplicating this logic, please hoist it (and the other example near line 435) into a common global function like get_build_type() -> str

[python3kgae]

Added get_build_type.

[dbort]

I'd rather extend BuiltFile to handle this situation in a common way, instead of manually repeating the pattern for every entry.

E.g., it could take an extra is_executable: bool = False param that could be set to true here, letting it know to add the .exe suffix on windows.

And we could embed a CMAKE_BUILD_TYPE (debug vs release) placeholder in the paths that is ignored on non-windows systems. How about

third-party/flatbuffers/%BUILD_TYPE%/flatc

Then, on windows systems we can replace %BUILD_TYPE% with get_build_type(), and on non-windows systems we can delete the string %BUILD_TYPE%/ from the path if present.

So this this entry could become

 ext_modules.append(
BuiltFile(
"third-party/flatbuffers/%BUILD_TYPE%/flatc",
"executorch/data/bin/",
is_executable=True,
)
)

And entries below can use the %BUILD_TYPE% placeholder, and use .* to match .so/.dll/.dylib on different systems.

[python3kgae]

Updated.

[dbort]

I really want to avoid _WIN32 checks in any of the code, but this is reasonable for now. The best solution would be to avoid using POSIX concepts and to just use the standard C APIs.

Suggested change

	#ifndef _WIN32
	// TODO: Rewrite FileDataLoader to use standard C FILE* instead of POSIX file
	// descriptors, so that we don't need this _WIN32 check.
	#ifndef _WIN32

[python3kgae]

Thank you for taking the time to track down and fix all of these issues! I'm generally happy with the approach, but I'd really like to find a way to avoid duplicating so much data and logic from install_requirements.sh.

Will the CI pipeline test cover all tests or there're some tests must be running locally?

Unfortunately the CI pipeline doesn't cover install_requirements.sh right now. If you can, please try running it on a linux and/or mac with your changes. If you don't have access to either, let us know, and we can help testing.

Thanks for the review.
To avoid duplicating install_requirements.sh, I’m considering rewriting it in portable Python. I’ll give it a try to see if it works.

I don’t have access to Linux or macOS, so I’ll need help testing it on those platforms once I’ve got install_requirements.py working

[python3kgae]

Updated based on the comments.
Tested install_requirements.sh on WSL and install_requirements.bat on Windows.
Need help to test Linux and Mac.

[dbort]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

[dbort]

Comments should begin with a capital letter. And I suggest making this a little more general.

Suggested change

	# avoid def flat.exe() on windows.
	# Remove .exe suffix on windows.

[dbort]

Heads up that #4760 merged recently, which changes this class to use pread(), which I think will break windows. It might make sense to write a separate WindowsFileDataLoader.

There would be a lot of duplicated code, though. If we needed to split into a windows-specific data loader, it might make sense to have an AbstractFileDataLoader with one pure-virtual protected method to do the actual read.

[dbort]

This comment repeats what the code does, and doesn't provide extra value. Either remove it, or (ideally) add a comment explaining why this logic is necessary. It'd also be helpful to explain what ClangCL is, since non-windows devs will not be familiar with it.

[dbort]

Your PR actually fixes the TODO, so we can remove it.

Suggested change

	#ifndef_WIN32
	#include<sys/types.h>// TODO(T126923429): Include size_t, ssize_t
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif
	// Define size_t and ssize_t.
	#ifndef_WIN32
	#include<sys/types.h>
	#else
	#include<stddef.h>
	usingssize_t=ptrdiff_t;
	#endif

[dbort]

Please add a comment explaining why this is necessary.

[dbort]

Now that there are more than two params, please use kwargs for all params. Same for other BuiltFile uses below.

Suggested change

	"third-party/flatbuffers/%BUILD_TYPE%/",
	"flatc",
	"executorch/data/bin/",
	src_path="third-party/flatbuffers/%BUILD_TYPE%/",
	src_name="flatc",
	dst="executorch/data/bin/",

[dbort]

Please call this src_dir since it's specifically a directory. src_path sounds like it could still refer to the path to the source file.

[dbort]

Will %BUILD_TYPE% always be the final component of the directory path? If so, we could remove the %BUILD_TYPE% placeholder and just add it to the source path inside BuiltFile.

It's ok as-is for now, though.

[python3kgae]

It is, but it does not have access to the installer in the constructor of BuiltFile.
If I understand correctly, obtaining the build configuration requires access to installer.debug.

[dbort]

Please mention %BUILD_TYPE% here and how to use it.

[python3kgae]

This is looking great!

Consider splitting this into some smaller PRs. E.g. we can probably merge the setup.py stuff pretty quickly, but the FileDataLoader might take longer. And, in general, we prefer to have smaller self-contained PRs.

Created 7 small PRs. Need more time for FileDataLoader.

[python3kgae]

Close after split to small PRs.