Miscellaneous fixes for Windows builds #1376
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
woah. thank you. I had to reboot that Windows machine to test some linux driver stuff on that GPU so it will be a few days before I get back to Windows. Thank for sharing. |
sstamenova
reviewed
Sep 16, 2022
sstamenova
reviewed
Sep 16, 2022
powderluv
approved these changes
Sep 21, 2022
|
cc: @powderluv Although I marked this PR as ready for review, this is still not the full set of fixes required for Windows builds (I just didn't want to keep this PR pending for too long). There are a couple of upstream LLVM and StableHLO changes necessary, if you're trying to build at your end. But both those upstream changes should be merged into Torch-MLIR on Monday. |
|
cool Thanks for the heads up. Will wait for the monday roll. |
|
Cancelled the CI since it's anyway going to fail until the LTC problem is resolved. @silvasean Among all these commits, the last commit is perhaps the most controversial. Let me know if you think I should use a different approach (like, say, disabling multi-threaded tests on Windows, instead of adding a new module). Once approved, I'll be sure to squash the commits into one before merging. |
|
Out of the things that I learnt while working on this PR, there are two that are probably relevant to folks who will build on Windows.
(a) and (c) are well supported, whereas (b) produces tons of warnings and a build failure, which was fixed in this upstream change: https://reviews.llvm.org/D134165. I ran into another build failure, this time because of what seems like a bug in the MSVC
Combined, this means that I could use the following CMake command to build and test Torch-MLIR on Windows, modulo the fixes in this PR and the upstream StableHLO changes that will be merged into Torch-MLIR early next week. Happy hacking! |
On Windows, the path to the Python binary may contain spaces, so this patch adds quotes around the path to the python executable. Thanks to @sstamenova for suggesting the fix!
Similar to https://reviews.llvm.org/D125284, we can safely remove this header file without affecting the build on either Linux. It is necessary to remove this header file on Windows builds since otherwise it causes build errors.
`TORCH_API` should apply to functions that are either exported by libtorch.so or ones that are imported from libtorch.so by its downstream consumers (like Torch-MLIR). Neither case applies to the `importJitFunctionAsFuncOp()` function, since it is defined in Torch-MLIR (and thus outside libtorch.so). This patch fixes the problem by dropping `TORCH_API` from that function's declaration.
The `class-annotator-repr.py` test checks for class annotations in a specific order, but prior to this patch, the order was non-deterministic, since the code iterated on an _unordered_ map. This patch makes the iteration order deterministic through two changes: 1. using a sorted map 2. using the class qualified name instead of the address of the class in memory
This ensures that tests use the Python3 version that was detected using CMake, instead of whichever python version that happens to be in the PATH variable when invoking the test.
The parenthesis syntax does not run on Windows (the shell interprets the `(` character as part of the path). Moreover, the ODR violation in the comment no longer seems to apply.
Since Windows does not support `fork` natively, Python's `multiprocessing` module needs to use `spawn` on Windows. However, to use `spawn`, the multiprocessing module serializes (or pickles) the worker function and its arguments. Sadly, the multiprocessing module (both the default one in Python and the one that is extended in PyTorch) is unable to serialize lambda functions (see https://stackoverflow.com/a/19985580) for detals. Unfortunately, given how our tests are structured, we require that the function under test is passed as an argument to another function, so we cannot sidestep our use of lambda functions. To resolve this problem, this patch makes use of the `multiprocess` and `dill` Python modules, which together offers a multiprocessing mechanism that can serialize lambda functions. The multiprocess module also offers a process pool, which simplifies the code for our parallel testing framework.
|
should we add a Windows CI next ? |
I think that would be great to make sure we don't accidentally regress after all the work that was put into getting windows to work. |
AmosLewis
pushed a commit
to AmosLewis/torch-mlir
that referenced
this pull request
Sep 29, 2022
* test: allow spaces in path to Python executable On Windows, the path to the Python binary may contain spaces, so this patch adds quotes around the path to the python executable. Thanks to @sstamenova for suggesting the fix! * python: remove header file that causes Windows build failures Similar to https://reviews.llvm.org/D125284, we can safely remove this header file without affecting the build on either Linux. It is necessary to remove this header file on Windows builds since otherwise it causes build errors. * python: drop `TORCH_API` from function defined in Torch-MLIR `TORCH_API` should apply to functions that are either exported by libtorch.so or ones that are imported from libtorch.so by its downstream consumers (like Torch-MLIR). Neither case applies to the `importJitFunctionAsFuncOp()` function, since it is defined in Torch-MLIR (and thus outside libtorch.so). This patch fixes the problem by dropping `TORCH_API` from that function's declaration. * python: make output of class anotations deterministic The `class-annotator-repr.py` test checks for class annotations in a specific order, but prior to this patch, the order was non-deterministic, since the code iterated on an _unordered_ map. This patch makes the iteration order deterministic through two changes: 1. using a sorted map 2. using the class qualified name instead of the address of the class in memory * test: use Python3_EXECUTABLE as interpreter path for consistency This ensures that tests use the Python3 version that was detected using CMake, instead of whichever python version that happens to be in the PATH variable when invoking the test. * test: fix RUN string The parenthesis syntax does not run on Windows (the shell interprets the `(` character as part of the path). Moreover, the ODR violation in the comment no longer seems to apply. * python: port parallel test framework to Windows Since Windows does not support `fork` natively, Python's `multiprocessing` module needs to use `spawn` on Windows. However, to use `spawn`, the multiprocessing module serializes (or pickles) the worker function and its arguments. Sadly, the multiprocessing module (both the default one in Python and the one that is extended in PyTorch) is unable to serialize lambda functions (see https://stackoverflow.com/a/19985580) for detals. Unfortunately, given how our tests are structured, we require that the function under test is passed as an argument to another function, so we cannot sidestep our use of lambda functions. To resolve this problem, this patch makes use of the `multiprocess` and `dill` Python modules, which together offers a multiprocessing mechanism that can serialize lambda functions. The multiprocess module also offers a process pool, which simplifies the code for our parallel testing framework.
qedawkins
pushed a commit
to nod-ai/torch-mlir
that referenced
this pull request
Oct 3, 2022
Importing onnx graph fails if an output is also used by another node. This happens because the output ValueInfo will be registered, and then it will throw an error that it already exists when importing internal ValueInfos. Solution is to import the internal ValueInfos before importing the output ValueInfos. Resolves llvm#1376 Signed-off-by: Michael Holman <michhol@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch contains multiple commits that make it possible to build and test Torch-MLIR on Windows. See commit messages for descriptions.
TORCH_APIfrom function defined in Torch-MLIR