Skip to content

Conversation

GregoryComer
Copy link
Member

@GregoryComer GregoryComer commented Sep 3, 2025

Summary

Build and run C++ tests in the unit test CI job on Windows. Fix a few lingering issues on Windows that surfaced.

Changes:

  • Add CMake build + CTest execution the Windows unit test job.
  • Gate Core ML export recipe tests (and coremltools imports) to non-Windows platforms.
  • Add some explicit cases for size_t 0 literals to make MSVC overload resolution happy.
  • Remove 2> /dev/null from some CMake PTE export custom commands. I'm not entirely sure why this is there in the first place (keep the log a little cleaner?), but it doesn't work on Windows. Easiest to just remove it.
  • Update the temp file utility code (in extension/testing_util/temp_file.h) to use standard library file IO (as opposed to posix).
  • (Critical): Replace long with int64_t in a few portable kernels that deal with indices. It should be int64_t, as this is the correct type for long tensors. On Windows, for example, long is actually 32-bit for historical reasons, and it causes memory corruption / crashes when trying to read/write ScalarType::Long tensors with long* pointers.
  • Update a bunch of custom commands to use platform-specific syntax or replace with a cross-platform approach.
  • Use non-strict export for test models. We can support strict export on Windows, but it take a bit of env setup. This is covered in CI in places, but non-strict is the default so it makes it a bit easier to build tests this way.

I've also excluded method_test and tensor_parser_test for now, due to the env issues with some of the PTE test models. The tests pass locally - but I need to debug the build-time PTE creation for these tests. I'm disabling to get the remaining 98% of the tests up first, and will fix these shortly. Tracking in #13883.

Test plan

CI - the tests are covered by the unittest jobs (debug, release, and editable).

Copy link

pytorch-bot bot commented Sep 3, 2025

🔗 Helpful Links

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

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

❌ 1 New Failure, 42 Pending

As of commit 04826a7 with merge base 9af908d (image):

NEW FAILURE - The following job has failed:

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

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 3, 2025
@GregoryComer GregoryComer force-pushed the windows-native-test-ci branch 16 times, most recently from 8028d96 to 43355de Compare September 10, 2025 04:10
@GregoryComer GregoryComer force-pushed the windows-native-test-ci branch 11 times, most recently from 62784a7 to 9364aea Compare September 11, 2025 05:04
@@ -49,21 +49,21 @@ Tensor& argmax_out(
static constexpr const char op_name[] = "argmax.out";

ET_SWITCH_REALHBF16_TYPES(in.scalar_type(), ctx, op_name, CTYPE, [&] {
long* out_data = out.mutable_data_ptr<long>();
int64_t* out_data = out.mutable_data_ptr<int64_t>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. We should have a lint for this cc @manuelcandales


def tearDown(self) -> None:
super().tearDown()

@unittest.skipIf(sys.platform == "win32", "Core ML is not available on Windows.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like evidence that we shouldnt have a mono recipe test with real backends.

Copy link
Member Author

@GregoryComer GregoryComer Sep 11, 2025

Choose a reason for hiding this comment

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

Yeah, might make sense to refactor it as a follow up. CC @abhinaykukkadapu Do you have any preferences on handling of platform-specific recipes?

Copy link
Contributor

@abhinaykukkadapu abhinaykukkadapu Sep 11, 2025

Choose a reason for hiding this comment

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

I was under the impression that lowering works across all platforms, although i haven't thought about windows tbf.

This seems like evidence that we shouldnt have a mono recipe test with real backends.

these tests only call forward if those specific et backends are available, check here: https://github.com/pytorch/executorch/blob/main/export/tests/test_target_recipes.py#L103, otherwise these tests just lowers the model.

@metascroy do you know if coreml tools work on a windows platform?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. From what I read, coremltools AOT flow is not available on Windows.

@GregoryComer GregoryComer force-pushed the windows-native-test-ci branch 2 times, most recently from b0cbcc2 to a900763 Compare September 11, 2025 18:47
@GregoryComer
Copy link
Member Author

@JacobSzwejbka I've made a few small updates to address comments. Do you have any blocking concerns?

GregoryComer and others added 2 commits September 11, 2025 11:58
ghstack-source-id: 1dca42f
ghstack-comment-id: 3251228740
Pull-Request: pytorch#13925
@GregoryComer
Copy link
Member Author

After rebase and minor cleanup, I'm seeing one (flaky) test failure relating to etdump generation due to being unable to access a temp file because there's another open handle to it. Tracking this in #13883. I've updated and am re-running CI now. If no additional objections, I hope to land this shortly, pending final approval from reviewers.

@GregoryComer GregoryComer merged commit f1ca55a into pytorch:main Sep 11, 2025
280 of 287 checks passed
GregoryComer added a commit to GregoryComer/executorch that referenced this pull request Sep 12, 2025
GregoryComer added a commit that referenced this pull request Sep 12, 2025
GregoryComer added a commit to GregoryComer/executorch that referenced this pull request Sep 12, 2025
facebook-github-bot pushed a commit that referenced this pull request Sep 13, 2025
Differential Revision: D82345075

Pull Request resolved: #14287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. release notes: windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants