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

ci: enable ccache on Windows #1548

Merged
merged 1 commit into from
Nov 3, 2022
Merged

ci: enable ccache on Windows #1548

merged 1 commit into from
Nov 3, 2022

Conversation

ashay
Copy link
Collaborator

@ashay ashay commented Nov 3, 2022

This patch makes a few small, but key, changes to enable ccache on
Windows. First, it replaces the hendrikmuhs/ccache-action action with
command line invocations to the ccache binary, since the action has two
bugs, one of which causes CI to refer to different ccache artifacts
before versus after the build on Windows whereas the other bug can
sometimes cause the action to incorrectly infer that the cache is empty.

Second, this patch slightly alters the cache key, so that our old cache
artifacts, which have grown too big, are eventually discarded in favor
of the new, smaller cache artifacts. Along the way, this patch also
keeps the RollPyTorch's cache artifact separate from the regular build's
cache artifact so as to keep these artifacts small, and also because the
RollPyTorch action is off the critical path for most contributors.

Third, this patch makes small changes to the CMake file so that on
Windows, the ccache binary is added as a prefix, as recommended on the
ccache Wiki.

Finally, as an anciallary change, this patch drops the PowerShell
scripts for Windows builds and instead moves the CMake invocation into
the workflow file. This patch doesn't invoke tests on Windows (since
the workflow prior to this patch also did not invoke tests), but I will
add the test invocation in a subsequent patch.


Time consumed for Windows builds:

  1. Cold cache run: 2 hours and 30 minutes.
  2. Exact cache hit: 25 minutes
  3. Imprecise cache hit: 25 minutes.
  • Imprecise cache hits occur when there was no cache entry for the precise commit hash, so the caching layer uses the most recent cache artifact.

This change does not address the recent macOS cache regression, where an imprecise cache hit causes CI to run a full rebuild of Torch-MLIR.

@ashay ashay requested a review from powderluv November 3, 2022 04:35
@powderluv
Copy link
Collaborator

Thank you for the great work on the CI. It may be worth leaving the Powershell script since we would want to "recreate the CI" locally and it is better to have a single entry point

Copy link
Collaborator

@powderluv powderluv left a comment

Choose a reason for hiding this comment

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

LGTM but may be worth having the .ps1 scripts around to test like the cI

@ashay
Copy link
Collaborator Author

ashay commented Nov 3, 2022

It may be worth leaving the Powershell script since we would want to "recreate the CI" locally and it is better to have a single entry point

Excellent point. I've added it back in, and tried to match the inlined CMake invocation. Hopefully I got the syntax right. :)

This patch makes a few small, but key, changes to enable ccache on
Windows.  First, it replaces the hendrikmuhs/ccache-action action with
command line invocations to the ccache binary, since the action has two
bugs, one of which causes CI to refer to different ccache artifacts
before versus after the build on Windows whereas the other bug can
sometimes cause the action to incorrectly infer that the cache is empty.

Second, this patch slightly alters the cache key, so that our old cache
artifacts, which have grown too big, are eventually discarded in favor
of the new, smaller cache artifacts.  Along the way, this patch also
keeps the RollPyTorch's cache artifact separate from the regular build's
cache artifact so as to keep these artifacts small, and also because the
RollPyTorch action is off the critical path for most contributors.

Finally, this patch makes small changes to the CMake file so that on
Windows, the ccache binary is added as a prefix, as recommended on the
[ccache Wiki](https://github.com/ccache/ccache/wiki/MS-Visual-Studio).
@powderluv
Copy link
Collaborator

oh but it is a bash script. I think we want it windows native .ps1 no ? Anyway not a big deal we can readd that later whenever required so we don't hold up your cache enhancements.

@ashay
Copy link
Collaborator Author

ashay commented Nov 3, 2022

I think we want it windows native .ps1 no ?

Indeed, we do, but I struggled with the PowerShell syntax until I finally went back to bash. I need to read up on variable substitution and the Start-Process thing before I come back to PowerShell.

we can readd that later whenever required so we don't hold up your cache enhancements.

I think so, too. I'll merge this PR and have a separate PR for switching to PowerShell.

@ashay ashay merged commit 2846776 into llvm:main Nov 3, 2022
@ashay ashay deleted the ashay/ccache-on-windows branch November 3, 2022 17:17
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.

2 participants