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

Split cache step into two, always save cache, fix hash files #422

Merged
merged 26 commits into from
Mar 16, 2023

Conversation

EnricoMi
Copy link
Owner

No description provided.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (setup-python)

       69 files  ±0         69 suites  ±0   29m 53s ⏱️ +55s
     405 tests ±0       405 ✔️ ±0      0 💤 ±0  0 ±0 
27 945 runs  ±0  27 405 ✔️ ±0  540 💤 ±0  0 ±0 

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (Test File)

97 tests  ±0   80 ✔️ ±0   3m 25s ⏱️ ±0s
  1 suites ±0   17 💤 ±0 
  1 files   ±0     0 ±0 

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (Dockerfile)

       69 files  ±0         69 suites  ±0   29m 53s ⏱️ +55s
     405 tests ±0       405 ✔️ ±0      0 💤 ±0  0 ±0 
27 945 runs  ±0  27 405 ✔️ ±0  540 💤 ±0  0 ±0 

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (macOS python 3.8)

       69 files  ±0         69 suites  ±0   29m 53s ⏱️ +55s
     405 tests ±0       405 ✔️ ±0      0 💤 ±0  0 ±0 
27 945 runs  ±0  27 405 ✔️ ±0  540 💤 ±0  0 ±0 

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

Comment on lines 243 to 244
uses: actions/cache/save@v3
if: always()

Choose a reason for hiding this comment

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

with this, you can essentially remove the date trick that accounts for #418 (comment), right?

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (Test Files)

   140 files  ±0       10 errors  730 suites  ±0   2h 12m 25s ⏱️ ±0s
1 786 tests ±0  1 499 ✔️ ±0    71 💤 ±0  208 ±0    8 🔥 ±0 
4 092 runs  ±0  3 410 ✔️ ±0  212 💤 ±0  452 ±0  18 🔥 ±0 

For more details on these parsing errors, failures and errors, see this check.

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (Docker Image)

       69 files  ±0         69 suites  ±0   29m 53s ⏱️ +55s
     405 tests ±0       405 ✔️ ±0      0 💤 ±0  0 ±0 
27 945 runs  ±0  27 405 ✔️ ±0  540 💤 ±0  0 ±0 

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

uses: actions/cache@v3
- name: Restore PIP packages cache
id: cache
uses: actions/cache/restore@v3

Choose a reason for hiding this comment

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

can I use this branch directly?

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

uses: EnricoMi/publish-unit-test-result-action/composite@branch-composite-cache-pip-packages-2

Choose a reason for hiding this comment

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

I went for the direct hash, so it's clear which commit was used, but now I realize that "Set up job" lists them.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (Linux python 3.8)

       69 files  ±0         69 suites  ±0   29m 53s ⏱️ +55s
     405 tests ±0       405 ✔️ ±0      0 💤 ±0  0 ±0 
27 945 runs  ±0  27 405 ✔️ ±0  540 💤 ±0  0 ±0 

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (Linux python installed)

       69 files  ±0         69 suites  ±0   29m 53s ⏱️ +55s
     405 tests ±0       405 ✔️ ±0      0 💤 ±0  0 ±0 
27 945 runs  ±0  27 405 ✔️ ±0  540 💤 ±0  0 ±0 

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (macOS 12 python installed)

       69 files  ±0         69 suites  ±0   29m 53s ⏱️ +55s
     405 tests ±0       405 ✔️ ±0      0 💤 ±0  0 ±0 
27 945 runs  ±0  27 405 ✔️ ±0  540 💤 ±0  0 ±0 

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (Linux 22.04 python installed)

       69 files  ±0         69 suites  ±0   29m 53s ⏱️ +55s
     405 tests ±0       405 ✔️ ±0      0 💤 ±0  0 ±0 
27 945 runs  ±0  27 405 ✔️ ±0  540 💤 ±0  0 ±0 

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (macOS python installed)

       69 files  ±0         69 suites  ±0   29m 53s ⏱️ +55s
     405 tests ±0       405 ✔️ ±0      0 💤 ±0  0 ±0 
27 945 runs  ±0  27 405 ✔️ ±0  540 💤 ±0  0 ±0 

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (Windows python installed)

       69 files  ±0         69 suites  ±0   29m 53s ⏱️ +55s
     405 tests ±0       405 ✔️ ±0      0 💤 ±0  0 ±0 
27 945 runs  ±0  27 405 ✔️ ±0  540 💤 ±0  0 ±0 

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

@@ -238,6 +239,14 @@ runs:
LOG_LEVEL: ${{ inputs.log_level }}
shell: bash

- name: Save PIP packages cache
uses: actions/cache/save@v3
if: always()
Copy link

@TWiStErRob TWiStErRob Mar 14, 2023

Choose a reason for hiding this comment

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

if: success() || failure()

always() includes cancelled() which might result in a partial broken cache

https://docs.github.com/en/actions/learn-github-actions/expressions#status-check-functions

A timeout can easily happen on tests, in which case an empty cache might over-write pip cache. Or the timeout happens while the pip is running which results in bad state.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results (reference)

       69 files  ±0         69 suites  ±0   29m 53s ⏱️ +55s
     405 tests ±0       405 ✔️ ±0      0 💤 ±0  0 ±0 
27 945 runs  ±0  27 405 ✔️ ±0  540 💤 ±0  0 ±0 

Results for commit 3b245f4. ± Comparison against base commit 2c22e15.

♻️ This comment has been updated with latest results.

@EnricoMi EnricoMi force-pushed the branch-composite-cache-pip-packages-2 branch from 6df3e74 to f3d26d2 Compare March 14, 2023 13:58
@EnricoMi EnricoMi changed the title Split cache step into two, always save cache Split cache step into two, always save cache, fix hash files Mar 14, 2023
@EnricoMi EnricoMi force-pushed the branch-composite-cache-pip-packages-2 branch 4 times, most recently from 463c81c to 8501241 Compare March 14, 2023 16:49
@TWiStErRob
Copy link

Please let me know if I should re-run on latest of this PR. Also please see the comments in this PR, they might be hiding between the test results.

@EnricoMi EnricoMi force-pushed the branch-composite-cache-pip-packages-2 branch from 8501241 to 39cb97a Compare March 14, 2023 17:15
@EnricoMi
Copy link
Owner Author

@TWiStErRob go for it, please test this branch!

@TWiStErRob
Copy link

TWiStErRob commented Mar 15, 2023

@TWiStErRob
Copy link

😍 image

@TWiStErRob
Copy link

TWiStErRob commented Mar 15, 2023

I reran it just to try and it took more than a minute second time:
https://github.com/TWiStErRob/net.twisterrob.libraries/actions/runs/4416412575/attempts/4

Both times it restored caches.
I cleared caches and running it again:
https://github.com/TWiStErRob/net.twisterrob.libraries/actions/runs/4416412575/attempts/5 (cancelled)
and will run again when it finishes to see the cache-reuse.

@EnricoMi
Copy link
Owner Author

Can you rerun the 🧪 Instrumentation Tests on 21 job, please? There is still an issue with the cache key.

@TWiStErRob
Copy link

Cancelled and running again with latest: https://github.com/TWiStErRob/net.twisterrob.libraries/actions/runs/4416412575/attempts/6
invited you, feel free to edit the branch and run the CI directly on TWiStErRob/net.twisterrob.libraries#38

@EnricoMi
Copy link
Owner Author

invited you

Thanks!

@TWiStErRob
Copy link

Sorry, cancelled again, hopefully reduced CI time significantly by running less tests.

@EnricoMi EnricoMi force-pushed the branch-composite-cache-pip-packages-2 branch from 8530724 to 1781604 Compare March 16, 2023 14:33
@EnricoMi EnricoMi force-pushed the branch-composite-cache-pip-packages-2 branch from d5f8056 to 7b55088 Compare March 16, 2023 15:20
continue-on-error: true
with:
path: ${{ steps.os.outputs.pip-cache }}
key: enricomi-publish-action-${{ runner.os }}-${{ runner.arch }}-pip-${{ steps.python.outputs.version }}-${{ hashFiles('**/requirements.txt', 'composite/action.yml') }}
key: enricomi-publish-action-${{ runner.os }}-${{ runner.arch }}-pip-${{ steps.python.outputs.version }}-f3f2c295046c91ed612b4efb6c9fb352

- name: Install Python dependencies

Choose a reason for hiding this comment

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

Can this affect steps.python.outputs.version, so that when it comes to cache/save it's a different version but it's still using the old key?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Don't get it. With key: ${{ steps.cache.outputs.cache-primary-key }}, the same key is used when saving the cache. Why should the python version change?

continue-on-error: true
with:
path: ${{ steps.os.outputs.pip-cache }}
key: enricomi-publish-action-${{ runner.os }}-${{ runner.arch }}-pip-${{ steps.python.outputs.version }}-${{ hashFiles('**/requirements.txt', 'composite/action.yml') }}
key: enricomi-publish-action-${{ runner.os }}-${{ runner.arch }}-pip-${{ steps.python.outputs.version }}-f3f2c295046c91ed612b4efb6c9fb352

Choose a reason for hiding this comment

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

I saw you were fighting with this, in the end you'll update it manually for each release?

Choose a reason for hiding this comment

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

Could you pull in https://github.com/actions/github-script and call hashFiles directly from JS?

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

you'll update it manually for each release

yes, but only if requirements.txt changes

Copy link
Owner Author

Choose a reason for hiding this comment

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

and call hashFiles directly from JS?

maybe, but I'd prefer not to add more complexity

@TWiStErRob
Copy link

Amazing! Congrats, it looked like a very hard labor. It's funny how 15MB can cut 3.5min to 15 seconds

@EnricoMi
Copy link
Owner Author

Found two bugs or undocumented edge cases where hashFiles does not work. With . or .. in the path parsing the yaml fails, and files in some paths are not reachable. Very annoying.

@EnricoMi EnricoMi merged commit acf931c into master Mar 16, 2023
@EnricoMi EnricoMi deleted the branch-composite-cache-pip-packages-2 branch March 16, 2023 18:15
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