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.

Sign up for GitHub

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

Use hash of all requirements files in cache keys for ci #6061

Merged
merged 1 commit into from
Apr 13, 2023
Merged

Use hash of all requirements files in cache keys for ci #6061

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ on:
branches:
- master

concurrency:
concurrency:
group: ${{ github.head_ref || github.run_id }}
cancel-in-progress: true

Expand Down Expand Up @@ -160,7 +160,7 @@ jobs:
- uses: actions/cache@v2
with:
path: ${{ env.pythonLocation }}
key: ${{ env.pythonLocation }}-${{ hashFiles('setup.py') }}-${{ hashFiles('dev-requirements.txt') }}
key: ${{ env.pythonLocation }}-${{ hashFiles('**/requirements.txt', 'dev_tools/requirements/**/*.txt') }}
- name: Install requirements
run: |
pip install wheel
Expand Down Expand Up @@ -229,7 +229,7 @@ jobs:
- uses: actions/cache@v2
with:
path: ${{ env.pythonLocation }}
key: ${{ env.pythonLocation }}-${{ hashFiles('setup.py') }}-${{ hashFiles('dev-requirements.txt') }}
key: ${{ env.pythonLocation }}-${{ hashFiles('**/requirements.txt', 'dev_tools/requirements/**/*.txt') }}
- name: Install requirements
run: |
pip install wheel
Expand All @@ -255,7 +255,7 @@ jobs:
- uses: actions/cache@v2
with:
path: ${{ env.pythonLocation }}
key: ${{ env.pythonLocation }}-${{ hashFiles('setup.py') }}-${{ hashFiles('dev-requirements.txt') }}
key: ${{ env.pythonLocation }}-${{ hashFiles('**/requirements.txt', 'dev_tools/requirements/**/*.txt') }}
- name: Install requirements
run: |
pip install wheel
Expand All @@ -280,7 +280,7 @@ jobs:
- uses: actions/cache@v2
with:
path: ${{ env.pythonLocation }}
key: ${{ env.pythonLocation }}-${{ hashFiles('setup.py') }}-${{ hashFiles('dev-requirements.txt') }}
key: ${{ env.pythonLocation }}-${{ hashFiles('**/requirements.txt', 'dev_tools/requirements/**/*.txt') }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Previously, the key was sensitive to setup.py, too. Should we keep that? It seems to me safer to do so.

Also, if you agree we should keep sensitivity to setup.py, then it might be prudent to broaden it to all setup.py files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's necessary since we don't install any of the cirq modules themselves in ci, we just run tests from the checked out cirq source. This may have come from very early days when we declared some dependencies in setup.py itself, but we stopped doing that probably even before we got rid of dev-requirements.txt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC, the cached content is for the Python environment content after pip install -r some/requirement.txt
We don't install cirq as check/pytest works directly with repository sources.

If so, we should not hash setup.py as it is not used in the CI run.

Copy link
Collaborator

Choose a reason for hiding this comment

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

SGTM!

- name: Install requirements
run: |
pip install wheel
Expand Down