-
Notifications
You must be signed in to change notification settings - Fork 680
[Windows] Run Python unit test CI on Windows #13716
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.
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
Conversation
Stack from ghstack (oldest at bottom): |
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/13716
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 5 New Failures, 4 Unrelated FailuresAs of commit fbde97d with merge base 6c1ef96 ( NEW FAILURES - The following jobs have failed:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
# Similar to flatbuffers, we want to build flatcc for the host. See inline comments | ||
# in the flatbuffers ExternalProject_Add for more details. | ||
ExternalProject_Add( | ||
flatcc_external_project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to resolve path length issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline
.ci/scripts/unittest-windows.ps1
Outdated
# pytest -n auto --cov=./ --cov-report=xml | ||
pytest --continue-on-collection-errors -v --full-trace -c pytest-windows.ini -n auto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why --continue-on-collection-errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. This is a holdover from progressive enablement and should not be necessary any more. I've removed it.
flatbuffers_external_project | ||
PREFIX ${CMAKE_CURRENT_BINARY_DIR}/flatbuffers_external_project | ||
flatbuffers_ep | ||
PREFIX ${CMAKE_CURRENT_BINARY_DIR}/flatc_proj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it flatbuffers_ep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was intentional, though I can rename it if desired. Flatbuffers builds flatc (distinct from flatcc). The path limitations are pretty tight in CI so I squeezed out a few more characters.
@@ -0,0 +1,115 @@ | |||
# NOTE: This file is a copy of pytest.ini, but with additional tests disabled for Windows. This | |||
# is intended to be a short-term solution to allow for incrementally enabling tests on Windows. | |||
# This file is intended to be deleted once the enablement is complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create an issue to track deletion of this file please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed as #13883.
After rebasing, I do see one test failure on the Windows job that looks like a flake. I'm going to investigate.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good!
There appears to be a race condition with the inductor code cache when performing strict mode export from multiple processes on Windows. It's causing occasional test failures on ET, so I'm disabling parallel execution on the Windows unit test jobs for now. I'll see if I can put together a nicer repro and file a bug against PyTorch. If not having parallel execution is prohibitive, we might be able work around it by not exporting on import in the devtools path, which is what consistently repros the issue during test collection. The race condition will still exist, but the rate at which we hit it might be acceptably low. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw - can you add a release notes: windows
and add this to it?
I've verified that the remaining CI failures are pre-existing (arm tests, core ML model tests, and NXP tests). |
from pathlib import Path | ||
|
||
libs = list(Path(__file__).parent.resolve().glob("**/libquantized_ops_aot_lib.*")) | ||
libs = list(Path(__file__).parent.resolve().glob("**/*quantized_ops_aot_lib.*")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change? is lib name something different on windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's named quantized_ops_aot_lib.dll (no lib prefix). We could maybe force the dll to have the lib prefix in the filename in cmake, but it seemed more idiomatic to do this.
Run unit test CI on Windows. I've disabled a few jobs (mainly pending tokenizers being available on Windows), but most everything is working out of box.
I did also make a few additional fixes: