Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

[Unittest][Metal] Add minimal metal functionality test to CI #15756

Merged
merged 6 commits into from Sep 27, 2023

Conversation

Lunderberg
Copy link
Contributor

Prior to this commit, the CI compiled TVM with USE_METAL=ON on OSX, as defined in conda/recipe/build.sh, but did not validate the execution of any generated metal kernels. As a result, breakage could occur without being caught by the CI, such as found following #15103.

This commit adds the execution of a single metal kernel as a minimal functionality test of the metal backend.

Prior to this commit, the CI compiled TVM with `USE_METAL=ON` on OSX,
as defined in `conda/recipe/build.sh`, but did not validate the
execution of any generated metal kernels.  As a result, breakage could
occur without being caught by the CI, such as found following
apache#15103.

This commit adds the execution of a single metal kernel as a minimal
functionality test of the metal backend.
@Lunderberg Lunderberg changed the title [Unittest][Metal] Add minimal metal functionality test to CI [Draft][Unittest][Metal] Add minimal metal functionality test to CI Sep 15, 2023
@Lunderberg
Copy link
Contributor Author

The initial version of this PR has a deliberate failure by asserting that target != "metal". If the CI updates are implemented correctly, the "CI / MacOS" stage should fail due to this assert, and all other CI tests should pass. This step is to verify that (1) the updated main.yaml is enabling additional tests, (2) that the CI build for OSX has the metal backend enabled, and (3) that the CI execution for OSX can find a metal-capable device.

After these three items are verified, the assert statement will be removed, and all CI steps including the "CI / MacOS" stage should pass.

@Lunderberg
Copy link
Contributor Author

@MasterJH5574 @junrushao

Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

Thank you! Awesome improvement to add minimal Metal testing to CI. Excited about it!

@Lunderberg
Copy link
Contributor Author

Hmm, first time that I've been disappointed to have the CI show green. The "CI / MacOS" section skipped the Metal test, rather than failing on the assert.

@Lunderberg
Copy link
Contributor Author

And getting closer. The "compile-only" test has two versions, one of which defines the PackedFunc "tvm_callback_metal_compile" and one of which does not. The version that does not define the PackedFunc finishes without error, as it would provide the generated code to the driver at runtime. The version that does define the PackedFunc has the compilation function called at compile-time.

Now, the final validation of the CI: Generating code in CodeGenMetal that should result in a compile-time error.

@Lunderberg
Copy link
Contributor Author

And the results look good, with the intentionally-incorrect codegen resulting in a failure found at compile-time of the *.metallib, during execution of the xcrun command.

The intentionally-incorrect codegen is now removed, and the compile-only test should now pass. Assuming this latest update passes CI, this PR will be marked as ready for review.

@Lunderberg Lunderberg marked this pull request as ready for review September 26, 2023 18:54
@Lunderberg
Copy link
Contributor Author

And ready for review!

@Lunderberg Lunderberg changed the title [Draft][Unittest][Metal] Add minimal metal functionality test to CI [Unittest][Metal] Add minimal metal functionality test to CI Sep 26, 2023
Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you! Glad to see such tests in CI.

@Lunderberg Lunderberg merged commit 0d68328 into apache:main Sep 27, 2023
20 checks passed
@Lunderberg Lunderberg deleted the metal_minimal_testing_in_ci branch September 27, 2023 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants