Attempted fix / investigate Action caching issues #264
Conversation
|
Warning Rate limit exceeded@CoMPaTech has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe GitHub Actions workflow was refactored to centralize and simplify caching by introducing a unified cache key and a new Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant CacheJob
participant PrepareJob
participant RestoreVenvAction
participant TestJob
Workflow->>CacheJob: Start cache job (compute cache key)
CacheJob-->>Workflow: Output unified cache key
Workflow->>PrepareJob: Start prepare job (depends on cache)
PrepareJob->>RestoreVenvAction: Restore venv/pre-commit cache (using cache key)
RestoreVenvAction-->>PrepareJob: venv/pre-commit ready
Workflow->>TestJob: Start test jobs (depend on cache and prepare)
TestJob->>RestoreVenvAction: Restore venv/pre-commit cache (using cache key)
RestoreVenvAction-->>TestJob: venv/pre-commit ready
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
CoMPaTech
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
scripts/setup_test.sh (1)
34-34: Optional: Fail on HTTP errors incurl
Add the-fflag (e.g.curl -sfL) so the script exits on non-200 responses rather than silently writing an error page to the output file undertmp/biome..github/workflows/verify.yml (1)
74-74: Remove trailing whitespace
The step name line has trailing spaces, triggering a YAML lint error. Please remove extra spaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/verify.yml(10 hunks)scripts/setup_test.sh(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/verify.yml
[error] 74-74: trailing spaces
(trailing-spaces)
🔇 Additional comments (7)
.github/workflows/verify.yml (7)
31-37: Solid: Python venv cache step
This step restores the Python virtual environment using hashes ofpyproject.tomlandrequirements_test.txt. The key and path correctly capture changes to dependencies.
161-166: Correct: caching Python test venv with dependency hashes
This restore step uses bothpyproject.tomlandrequirements_test.txthashes, ensuring cache invalidation on dependency changes.
200-200: Cache venv correctly in pytest job
The cache key includes hashes ofpyproject.tomlandrequirements_test.txt, so changes to your dependencies will invalidate this cache.
232-237: Cache venv correctly for mypy
Restoring the venv before running mypy with the same dependency-based key ensures the environment is consistent.
286-291: Cache venv for coverage job
Using the established key format withpyproject.tomlandrequirements_test.txthashes ensures the correct environment before combining coverage artifacts.
327-332: Cache venv ahead of TestPyPI publishing
The key includes Python dependency hashes, guaranteeing the build environment is restored accurately prior to publishing.
366-371: Cache venv for complexity analysis
Applying the same cache key pattern maintains consistency and ensures your complexity report runs against the correct dependencies.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/verify.yml (3)
46-54: Separate venv and pre-commit caches or unify keys
MixingvenvandPRE_COMMIT_HOMEunder a key that only hashes.pre-commit-config.yamlleaves Python dependencies stale when they change. Either split this into two cache steps (one forvenvhashing dependency files, one forPRE_COMMIT_HOMEhashing the pre-commit config) or expand the key to include both.
74-82: Avoid mixing venv & pre-commit caches with mismatched keys
This restore step combinesvenvand$PRE_COMMIT_HOMEbut only hashes the pre-commit config. Python dependency changes won't invalidate this cache.
120-127: Avoid mixing venv & pre-commit caches with mismatched keys incommitcheck
The cache key here only includes the pre-commit config, so dependency updates won’t bust the venv. Split or expand as above.
🧹 Nitpick comments (2)
.github/workflows/verify.yml (2)
36-36: DRY up repetitive cache key definitions
The cache key definition is duplicated across multiple jobs. Consider extracting it into a reusable YAML anchor or template to reduce duplication and simplify maintenance.Also applies to: 82-82, 127-127, 169-169, 205-205, 245-245, 300-300, 341-341, 382-382
74-74: Remove trailing spaces
Several lines have trailing spaces that trigger YAML lint errors. Please remove them to satisfy linting.Also applies to: 150-150, 185-185, 226-226, 284-284, 366-366
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/verify.yml(16 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/verify.yml
[error] 74-74: trailing spaces
(trailing-spaces)
[error] 150-150: trailing spaces
(trailing-spaces)
[error] 185-185: trailing spaces
(trailing-spaces)
[error] 226-226: trailing spaces
(trailing-spaces)
[error] 284-284: trailing spaces
(trailing-spaces)
[error] 366-366: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/verify.yml (2)
108-112: Ensurecommitcheckdepends onpreparefor cache availability
Addingpreparetoneedsguarantees the virtual environment cache is populated before restoring incommitcheck.
150-152: No action needed on updatedneedsdeclarations—they correctly sequence job dependencies.Also applies to: 185-187, 226-228, 284-286, 366-368
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/verify.yml (3)
46-53: Avoid mixing venv & pre-commit caches with mismatched keys
The combined cache step restores bothvenvandPRE_COMMIT_HOMEbut only hashes.pre-commit-config.yaml. Python dependency changes won’t bust this cache. Either split into separate caches or include Python dependency files (e.g.,pyproject.toml,requirements_test.txt) in the key.
74-82: Align cache key with all relevant dependency files in Ruff job
In the Ruff job, pre-commit and Python venv are cached together using only.pre-commit-config.yaml. If Python dependencies change, the cache remains stale. Include all Python dependency files in the key or split the steps.
120-127: Avoid mixing venv & pre-commit caches with mismatched keys in commitcheck
Thecommitcheckjob still restores bothvenvandPRE_COMMIT_HOMEwith a key that only hashes.pre-commit-config.yaml. Align this key with Python dependencies or split caches to prevent stale environments.
🧹 Nitpick comments (1)
.github/workflows/verify.yml (1)
74-74: Remove trailing whitespace
Multiple lines contain trailing spaces, causing YAML lint errors. Please strip the extra spaces at the end of these lines to satisfy formatting requirements.Also applies to: 150-150, 185-185, 226-226, 284-284, 366-366
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/verify.yml(16 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/verify.yml
[error] 74-74: trailing spaces
(trailing-spaces)
[error] 150-150: trailing spaces
(trailing-spaces)
[error] 185-185: trailing spaces
(trailing-spaces)
[error] 226-226: trailing spaces
(trailing-spaces)
[error] 284-284: trailing spaces
(trailing-spaces)
[error] 366-366: trailing spaces
(trailing-spaces)
🔇 Additional comments (6)
.github/workflows/verify.yml (6)
108-108: Approve addition ofpreparedependency in commitcheck job
Explicitly addingprepareto thecommitcheckjob’sneedsensures the environment setup completes before linting.
150-152: Approve updated dependencies forprepare-test-cachejob
Adding bothprepareandcommitcheckto this job’sneedsensures the test cache is built only after pre-commit checks pass.
185-187: Approve updated dependencies forpytestjob
Linkingpytesttoprepare-test-cacheensures the test cache is populated before execution.
226-228: Approve updated dependencies formypyjob
Requiringpytestbeforemypyguarantees that test artifacts and caches are available.
284-286: Approve updated dependencies forcoveragejob
Addingpytestas a prerequisite guarantees coverage artifacts are ready for processing.
366-368: Approve updated dependencies forcomplexityjob
Includingcoveragebeforecomplexityensures that all coverage and cache steps complete prior to complexity analysis.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
.github/workflows/restore-venv/action.yml (1)
1-31: Misplaced composite action file
This YAML defines a composite action (runs: using: composite) but is located under.github/workflows, causing actionlint errors (missingon/jobs). Composite actions should reside in.github/actions/restore-venv. Remove or relocate this file and update workflows to reference the correct path.
🧹 Nitpick comments (2)
.github/actions/restore-venv/action.yml (1)
13-16: Typo in default comment
In thefail-on-missinput comment, “DefauLt” has an uppercase L. Consider updating to “Default” for consistency..github/workflows/verify.yml (1)
135-135: Remove trailing whitespace
Lines 135, 138, 173, 223, and 295 contain trailing spaces, which trigger YAML lint errors. Please trim these.Also applies to: 138-138, 173-173, 223-223, 295-295
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/actions/restore-venv/action.yml(1 hunks).github/workflows/restore-venv/action.yml(1 hunks).github/workflows/verify.yml(14 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/restore-venv/action.yml
1-1: "on" section is missing in workflow
(syntax-check)
1-1: "jobs" section is missing in workflow
(syntax-check)
2-2: unexpected key "description" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
4-4: unexpected key "runs" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
22-22: unexpected key "inputs" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
.github/workflows/verify.yml
153-153: property "python--version" is not defined in object type {python-version: number}
(expression)
🪛 YAMLlint (1.37.1)
.github/workflows/verify.yml
[error] 135-135: trailing spaces
(trailing-spaces)
[error] 138-138: trailing spaces
(trailing-spaces)
[error] 173-173: trailing spaces
(trailing-spaces)
[error] 223-223: trailing spaces
(trailing-spaces)
[error] 295-295: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Check commit
🔇 Additional comments (5)
.github/workflows/verify.yml (5)
7-7: Cache version bump
CACHE_VERSIONbumped to2to invalidate existing caches. Verify that all workflows and downstream cache references are updated accordingly.
10-11: IntroduceVENVenvironment variable
DefiningVENV: venvcentralizes the virtual environment path and reduces duplication across jobs.
32-40: Consolidated caching inpreparejob
Using the composite action.github/actions/restore-venvwith a unified key (hashingpyproject.toml,requirements_test.txt, and.pre-commit-config.yaml) andfail-on-miss: falseeffectively addresses stale-cache issues and bootstraps the cache.
70-78: Cache miss behavior inruffjob
You omittedfail-on-miss(defaults totrue), so Linter jobs will error on a cache miss. Confirm this strict behavior is intended; otherwise, setfail-on-miss: falseto allow cache creation.
110-117: Cache miss behavior incommitcheckjob
Similarly,fail-on-missdefaults totrue, causing the job to fail if the cache is missing. Ensure this is desired or explicitly setfail-on-miss: falseto bootstrap the cache.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #264 +/- ##
==========================================
- Coverage 81.13% 81.12% -0.02%
==========================================
Files 36 36
Lines 7502 7502
==========================================
- Hits 6087 6086 -1
- Misses 1415 1416 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/verify.yml (1)
64-64: Remove trailing spaces
Several lines contain trailing spaces which can break YAML parsing or fail linters. Please trim whitespace at end-of-line for the flagged lines.Also applies to: 143-143, 148-148, 183-183, 234-234, 276-276, 316-316
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/restore-venv/action.yml(1 hunks).github/workflows/verify.yml(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/actions/restore-venv/action.yml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/verify.yml
[error] 64-64: trailing spaces
(trailing-spaces)
[error] 143-143: trailing spaces
(trailing-spaces)
[error] 148-148: trailing spaces
(trailing-spaces)
[error] 183-183: trailing spaces
(trailing-spaces)
[error] 234-234: trailing spaces
(trailing-spaces)
[error] 276-276: trailing spaces
(trailing-spaces)
[error] 316-316: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run pytest using Python 3.13
🔇 Additional comments (11)
.github/workflows/verify.yml (11)
7-10: Bump cache version and introduce VENV variable
IncrementingCACHE_VERSIONand definingVENVis essential for invalidating prior caches and standardizing the virtual environment directory. Ensure downstream composite actions referenceenv.VENVconsistently.
20-26: Consolidate cache key computation into a dedicated job
Thiscachejob centralizes the cache-key logic and exposes it via outputs, reducing duplication across jobs. The approach correctly setsoutputs.cache-keyfromsteps.set-key.
35-37: Verify dependency files included in cache key hash
ThehashFilesinvocation coverspyproject.toml,requirements_test.txt, and.pre-commit-config.yaml. Please confirm that all relevant dependency artifacts (e.g.,requirements_commit.txtor lock files) are included to avoid stale caches.
41-41: Adopt compositerestore-venvaction inpreparejob
The new composite action handles cache restore, venv creation, and dependency installation when missing. It correctly passescache-key,fail-on-miss,python-version,venv-dir, andprecommit-home.Also applies to: 51-59
64-66: Standardize caching inruffjob
Addingcacheandprepareas dependencies and reusing therestore-venvcomposite alignsruffwith the new caching strategy. Activation steps remain correct afterward.Also applies to: 82-89
105-107: Unifycommitcheckjob with composite caching
The job now depends oncacheandprepareand usesrestore-venvcorrectly before runningpre-commitchecks. This reduces duplication and ensures all checks use a synchronized environment.Also applies to: 118-126
143-146: Integrate caching into matrixpytestjob
Includingcacheandpreparedependencies and appending-pytest-matrix-${{ matrix.python-version }}to the cache key for each matrix axis ensures both correctness and separable caches per Python version.Also applies to: 158-166
183-187: Apply caching tomypyjob (disabled)
Even though the job is disabled, aligning its dependencies and caching logic using the composite action prepares the ground for re-enablement. Confirm theif: falseguard remains intended.Also applies to: 197-205
234-238: Optimizecoveragejob with composite action
Unifying thecoveragejob's cache steps reduces boilerplate. Dependencies includecache,prepare, andpytest, which match the execution order.Also applies to: 247-255
276-280: Streamlinetest-publishingcaching
The job now properly waits forcacheandprepare, and uses therestore-venvaction before build and publish steps. This ensures the environment and dependencies are consistent.Also applies to: 289-295
316-320: Adopt composite caching incomplexityjob
Adding therestore-venvaction and the correctneedslist streamlines the complexity analysis step. The radon command remains unchanged.Also applies to: 328-335
|
@coderabbitai summary @bouwew @dirixmjm this should clean up the builds - unfortunately completely parallel doesn't work wrt requiring building the cache image before using it - but it's considerably faster, more consistent and imho better to understand for future issues (i.e. the restore-keys and key/keys usage was 'abundant' :P) I have zero clue why codecov has a decrease in coverage though, it's line 91 in messages/init.py ??? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@CoMPaTech I'll provide a full review and summary of your caching improvements PR. Let me first examine the files to understand the changes and investigate the codecov coverage issue you mentioned. 🏁 Script executed: Length of output: 16827 🏁 Script executed: Length of output: 5453 🏁 Script executed: Length of output: 1592 Now I have a complete understanding of your changes. Let me provide a comprehensive review and address the codecov coverage issue. 🔍 Full Review & SummaryExcellent Caching Strategy Improvements ✅Your PR successfully addresses the "abundant" cache key usage by implementing a centralized caching approach: Key Improvements:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/verify.yml (1)
30-37: Include all dependency files in cache key
The unified key hashes onlypyproject.toml,requirements_test.txt, and.pre-commit-config.yaml. We also need to hashrequirements_commit.txt(where used) to avoid stale venv caches on dependency changes.- run: echo "cache-key=${{ runner.os }}-venv-cache-${{ env.CACHE_VERSION }}-${{ steps.python.outputs.python-version }}-${{ hashFiles('pyproject.toml', 'requirements_test.txt', '.pre-commit-config.yaml') }}" >> "$GITHUB_OUTPUT" + run: echo "cache-key=${{ runner.os }}-venv-cache-${{ env.CACHE_VERSION }}-${{ steps.python.outputs.python-version }}-${{ hashFiles('pyproject.toml', 'requirements_test.txt', 'requirements_commit.txt', '.pre-commit-config.yaml') }}" >> "$GITHUB_OUTPUT"
🧹 Nitpick comments (1)
.github/workflows/verify.yml (1)
64-64: Nitpick: Remove trailing whitespace
There are trailing spaces on several lines (needs:statements). Cleaning these up will eliminate YAML lint errors.Also applies to: 143-143, 148-148, 183-183, 234-234, 276-276, 316-316
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/verify.yml(16 hunks)CHANGELOG.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/verify.yml
[error] 64-64: trailing spaces
(trailing-spaces)
[error] 143-143: trailing spaces
(trailing-spaces)
[error] 148-148: trailing spaces
(trailing-spaces)
[error] 183-183: trailing spaces
(trailing-spaces)
[error] 234-234: trailing spaces
(trailing-spaces)
[error] 276-276: trailing spaces
(trailing-spaces)
[error] 316-316: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Process test coverage
🔇 Additional comments (19)
.github/workflows/verify.yml (19)
7-10: Approve: Define cache/version and environment dirs
IntroducingCACHE_VERSION,PRE_COMMIT_HOME, andVENVas top-level env vars makes the caching strategy explicit and reusable across jobs.
20-25: Approve: Add dedicated cache job
The newcachejob cleanly isolates cache key computation and shares it via an output, simplifying downstream restores.
41-41: Approve: Restore or create venv in prepare job
preparecorrectly depends oncacheand uses the compositerestore-venvaction withfail-on-miss=false, ensuring the first run seeds the cache.Also applies to: 51-59
64-66: Approve: Update ruff job dependencies
Adding bothcacheandpreparetoruffensures that lint runs with a populated venv.
77-84: Approve: Use composite restore-venv in ruff
Switching to the centralized action removes redundant cache keys and aligns with the unified approach.
105-107: Approve: Update commitcheck dependencies
commitchecknow explicitly depends oncache,prepare, andruff, ensuring environment and formatting checks run in order.
119-126: Approve: Use restore-venv in commitcheck
Consistent use of the composite action simplifies cache restores and setup.
143-147: Approve: Update pytest dependencies
Requiringcache,prepare, andcommitcheckbeforepytestguarantees correct environment and formatting for tests.
148-148: Approve: Restrict pytest matrix to DEFAULT_PYTHON
Limiting pytest to the default Python version simplifies CI and matches project compatibility.
159-166: Approve: Cache venv per pytest matrix
Appending-pytest-matrix-${{ matrix.python-version }}to the key isolates caches for different interpreters and usesfail-on-miss=falseto seed.
180-180: Approve: Explicitly disable mypy job
Usingif: falsekeeps the job in the graph while bypassing it until the codebase is ready for static typing.
183-186: Approve: Standardize mypy dependencies
Aligningmypywithcache,prepare, andpytestmirrors other jobs’ dependency chain.
197-204: Approve: Restore venv in mypy
Usingrestore-venvinmypymaintains the unified caching and environment setup pattern.
234-237: Approve: Update coverage dependencies
Ensuringcoverageruns aftercache,prepare, andpytestguarantees that all artifacts and environment are ready.
247-254: Approve: Use restore-venv in coverage
Consistent composite action usage streamlines the workflow and reduces duplication.
277-280: Approve: Update test-publishing dependencies
Runningtest-publishingaftercoverageintegrates well with the build and publish pipeline.
289-296: Approve: Restore venv in test-publishing
Composite action restores a fully prepared environment for packaging and publishing.
316-318: Approve: Update complexity job dependencies
Addingcacheandpreparebeforecomplexityensures that the environment and coverage steps complete first.
328-334: Approve: Use restore-venv in complexity job
Maintaining a single pattern for environment setup across all jobs enhances readability and maintainability.
|
Summary by CodeRabbit
New Features
Chores
Bug Fixes