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

Problem about caching strategy for Lean #64

Closed
SnO2WMaN opened this issue Jun 18, 2024 · 5 comments
Closed

Problem about caching strategy for Lean #64

SnO2WMaN opened this issue Jun 18, 2024 · 5 comments

Comments

@SnO2WMaN
Copy link

SnO2WMaN commented Jun 18, 2024

Note: This is based on my assumptions and might not be entirely accurate.

This issue occurs when we update our project from v4.9.0-rc1 to v4.9.0-rc2. FormalizedFormalLogic/Foundation#85
And it seems to be happens same problem branch in the nightly branch of the import-graph.

In our project, we set the GitHub Action cache strategy as follows

path: .lake
key: lake-${{ runner.os }}-${{ hashFiles('lake-manifest.json') }}-${{ github.ref }}
restore-keys: |
    lake-${{ runner.os }}-${{ hashFiles('lake-manifest.json') }}-${{ github.ref }}
    lake-${{ runner.os }}-${{ hashFiles('lake-manifest.json') }}
    lake-${{ runner.os }}

The problem we encountered was with the restore-keys we specified. The issue is that when updating the Lean version itself, the previous version's cache also becomes a fallback candidate. In our project, it seems we referenced the v4.9.0-rc1 cache, which caused the build to fail.

Solution is remove fallbacks of restore-keys to lake-${{ runner.os }}-${{ hashFiles('lake-manifest.json') }}-${{ github.ref }} only.

Currently, the cache strategy of this action is similar (i.e., it doesn't avoid using the cache when the Lean version itself is updated).

lean-action/action.yml

Lines 93 to 99 in 52906d4

- uses: actions/cache@v4
if: ${{ inputs.use-github-cache == 'true' }}
with:
path: ${{ inputs.lake-package-directory }}/.lake
key: ${{ runner.os }}-lake-${{ github.sha }}
# if no cache hit, fall back to the (latest) previous cache
restore-keys: ${{ runner.os }}-lake-

This seems to be why the nightly branch of the import-graph is broken.
As a solution, specifying ${{ hashFiles('lean-toolchain') }} as the key should work.

@SnO2WMaN
Copy link
Author

Maybe the cache key should be like this, specifying lean-toolchain and lake-manifest.json

key: lake-${{ runner.os }}-${{ hashFiles('lean-toolchain') }}-${{ hashFiles('lake-manifest.json') }}-${{ github.ref }}
restore-keys: lake-${{ runner.os }}-${{ hashFiles('lean-toolchain') }}-${{ hashFiles('lake-manifest.json') }}-${{ github.ref }}

@digama0
Copy link

digama0 commented Jun 18, 2024

duplicate of #62

@austinletson
Copy link
Collaborator

Even though the failure is the same as #62, I think the suggestion to improve the key for the caching is still valid.

Maybe the cache key should be like this, specifying lean-toolchain and lake-manifest.json

key: lake-${{ runner.os }}-${{ hashFiles('lean-toolchain') }}-${{ hashFiles('lake-manifest.json') }}-${{ github.ref }}
restore-keys: lake-${{ runner.os }}-${{ hashFiles('lean-toolchain') }}-${{ hashFiles('lake-manifest.json') }}-${{ github.ref }}

I think this is an improvement over the existing key input for the cache

However, it seems to me that the restore-keys need to be a prefix match of whatever existing cache key is hit. If you have the same value for the key and restore-key input it seems redundant, although I could be missing something here.

For the restore-key input what about:

restore-keys: lake-${{ runner.os }}-${{ hashFiles('lean-toolchain') }}-${{ hashFiles('lake-manifest.json') }}

With this lean-action can restore caches where lean-toolchain and lake-manifest.json remain unchanged.

@SnO2WMaN what do you think?

Also for reference, the initial discussion of what key to use for the hash was here.

@SnO2WMaN
Copy link
Author

SnO2WMaN commented Jun 19, 2024

Thanks for your advice.

Once considered, we added ${{ github.ref }} to prevent build errors caused by cache from different Lean versions, but now we have switch to use lean-toolchain. It might no longer necessary so we might be able to remove ${{ github.ref }} from both the key and restore-key.

@austinletson
Copy link
Collaborator

Thank you @SnO2WMaN for raising this issue and coming up with suggestions for improved caching keys! #71 implements these changes.

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

No branches or pull requests

3 participants