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

force render all articles #187

Merged
merged 5 commits into from
Jan 30, 2024
Merged

force render all articles #187

merged 5 commits into from
Jan 30, 2024

Conversation

pawelru
Copy link
Contributor

@pawelru pawelru commented Jan 3, 2024

When reading the logs from deployment I noticed that only part of the articles are being rendered (and the other part is taken from cache?) Few examples from the latest build:

We do want to render (cover) all the articles so as to test sufficiently against changes in the packages used. This PR aims to force render all. I hope this is enough.
This is desirable for both stable and devel profile so changes in the main config file.

Relevant docs:
https://quarto.org/docs/projects/code-execution.html#freeze
https://quarto.org/docs/projects/code-execution.html#cache

Signed-off-by: Pawel Rucki <[email protected]>
@pawelru pawelru requested a review from cicdguy January 3, 2024 15:14
Copy link
Contributor

@cicdguy cicdguy left a comment

Choose a reason for hiding this comment

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

This could be a very expensive operation every time. I'd recommend just clearing the runner caches if we want to force render.

Copy link
Contributor

github-actions bot commented Jan 3, 2024

Unit Tests Summary

  1 files  2 suites   1m 0s ⏱️
 24 tests 0 ✅  24 💤 0 ❌
289 runs  0 ✅ 289 💤 0 ❌

Results for commit 26077d0.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Jan 3, 2024

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
graph-snaps 💔 $7.93$ $+2.01$ $0$ $0$ $0$ $0$
markdown-snaps 💔 $49.83$ $+1.06$ $0$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
graph-snaps 💔 $3.55$ $+1.36$ plot_v1

Results for commit 656c7e9

♻️ This comment has been updated with latest results.

@pawelru
Copy link
Contributor Author

pawelru commented Jan 3, 2024

This could be a very expensive operation every time.

Yes, we should expect increased timings on each workflow run but I guess this is unavoidable if we want to test unchanged code against environment (packages) for potential incompatibility - we have to run everything.
I have only one idea to address it but this probably requires more thinking and development. We can differentiate scheduled and push-triggered runs. Only the latter would use cache and the former will render everything. LMKWYT

I'd recommend just clearing the runner caches if we want to force render.

Yes that's essentially it. Would you mind push necessary changes?

@cicdguy
Copy link
Contributor

cicdguy commented Jan 4, 2024

Looks like GitHub has provided an out-of-the-box solution for cache eviction: https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#force-deleting-cache-entries

I'll add it here.

@cicdguy
Copy link
Contributor

cicdguy commented Jan 4, 2024

FWIW, these caches are big, and literally expensive :)

Screenshot 2024-01-04 at 8 15 35 AM

@pawelru
Copy link
Contributor Author

pawelru commented Jan 24, 2024

Hey @cicdguy I noticed that caches are gone. Is there anything required from the perspective of action definition? Things like remove cache creation / push?
Would you mind review this PR if that's enough?

@shajoezhu shajoezhu added the sme label Jan 27, 2024
@cicdguy cicdguy enabled auto-merge (squash) January 30, 2024 13:26
@cicdguy cicdguy merged commit 0ccf662 into main Jan 30, 2024
15 checks passed
@cicdguy cicdguy deleted the pawelru-patch-1 branch January 30, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants