CI: exclude linting dependencies from the unittest environment. #8861
Conversation
mbyrnepr2
added
Skip news 🔇
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8861 /- ##
=======================================
Coverage 95.88% 95.88%
=======================================
Files 173 173
Lines 18552 18552
=======================================
Hits 17789 17789
Misses 763 763 |
mbyrnepr2
force-pushed
the
ci_exclude_linting_deps_from_unittests
branch
from
f89e0af
to
d3eab3d
Compare
mbyrnepr2
force-pushed
the
ci_exclude_linting_deps_from_unittests
branch
from
d3eab3d
to
7c63128
Compare
requirements_test.txt
Outdated
| @@ -9,4 8,4 @@ pytest-xdist~=3.3 | |||
| six | |||
| # Type packages for mypy | |||
| types-pkg_resources==0.1.3 | |||
| tox>=3 | |||
| tox>=4.6.4 | |||
There was a problem hiding this comment.
Why are we bumping this? Is the old version broken?
There was a problem hiding this comment.
The main reason is I wanted to invalidate the cache (for testing purposes) so that I could see that the virtual environment for the Pytest stage is using the test dependencies only and not re-using the older cache from the previous run with previously-installed dependencies (in which case I couldn't prove this change is working).
For a visual inspection you can compare the output above to this run of the unit tests.
So other than that, I can revert this line once I'm finished noodling, or we can bump it since its a new major version - I'm ok with either outcome.
There was a problem hiding this comment.
Ok so I think the pylint job is failing only for the context of the current branch(or caches related to this branch) because the CI is now using the cache created by the first push/commit: f89e0af.
This commit only contains the change to the requirements_test.txt but not the "install pre-commit" command in the venv.
Apart from that I'm not sure why the primer jobs are erroring - do you have an idea about that @DanielNoord ?
mbyrnepr2
force-pushed
the
ci_exclude_linting_deps_from_unittests
branch
from
348d1d8
to
29a0e0a
Compare
mbyrnepr2
force-pushed
the
ci_exclude_linting_deps_from_unittests
branch
from
e83cf1d
to
712ffb8
Compare
| @@ -173,11 174,9 @@ jobs: | |||
| key: | |||
| ${{ runner.os }}-${{ steps.python.outputs.python-version }}-${{ | |||
| needs.prepare-base.outputs.python-key }} | |||
| - name: Install tox | |||
There was a problem hiding this comment.
This step wasn't necessary because tox is installed already via requirements_test.txt and it is available once the venv is activated.
| @@ -1,4 1,3 @@ | |||
| -r requirements_test_pre_commit.txt | |||
There was a problem hiding this comment.
I believe the purpose of requirements_test_pre_commit.txt is to invalidate the cache in the CI and install test a new dependency if a user changes the file manually (or dependabot bumps it).
Apart from that, there is a separate job in the CI to run pre-commit and its dependencies are managed via .pre-commit-config.yml.
mbyrnepr2
force-pushed
the
ci_exclude_linting_deps_from_unittests
branch
from
798193f
to
e7195aa
Compare
mbyrnepr2
changed the title
|
The primer is failing with a message I sometimes see locally (to the best of my recollection) when I have an editable install on my path of |
I haven't looked thoroughly @jacobtylerwalls, I'm going to take a break but I'd like to look into it. Specifically the error is: https://github.com/pylint-dev/pylint/actions/runs/5611758519/jobs/10268523177?pr=8861#step:11:25 |
|
(I'd suggest inserting a |
|
It might also be easier to debug after #8854. |
|
The failing primers are due to the cache miss. We don't handle that correctly apparently. We once decided to only create the project caches on |
Thanks @DanielNoord 👍. So, my understanding is that the clone of the primer packages is done on the main branch and all PR branches attempt to access those packages by hitting that single source of truth. Now, if say, the cache was evicted due to hitting the size limit for GitHub workflow caches, then we may need to re-clone in the PR branches. Maybe, let's see :D |
This comment has been minimized.
This comment has been minimized.
| @@ -1,4 1,3 @@ | |||
| -r requirements_test_pre_commit.txt | |||
There was a problem hiding this comment.
I wonder if we should update the last paragraph here about pre-commit installation to point out the need to install the pre-commit requirements?
Co-authored-by: Jacob Walls <[email protected]>
I was initially happy to see the checks passing but I’m not sure it makes sense to do the clone here. It could contribute to the overall cache size capacity being reached quicker and caches becoming evicted etc. I think I’d like to give it some more consideration before merging anyway😄 |
This current way may also be contributing to the massive diff which doesn’t look right either |
|
Ah, can you explain why there would be more caches? Are we using a different cache key than the one created on main? |
|
@DanielNoord it did occur to me recently that the project cache is not very useful now that so many projects are the in the primer. I think the cache is only good for about an hour at most before a new commit on one of the projects invalidates the whole thing. |
Sorry, I guess the clone I've added wouldn't add a new cache. But it would add overhead if all the parallel jobs for a particular Python version perform the clones at the same time. I do notice a large number of caches in the list (https://github.com/pylint-dev/pylint/actions/caches) so overall I think there should be some way to limit the number; otherwise caches may start to disappear leading to unexpected behaviour. |
|
If you don't mind, I'd prefer to merge ASAP, as the broken primer is adding to the burden of understanding for a new contributor on another PR. |
|
Ok @jacobtylerwalls 👍 |
|
@mbyrnepr2 just noticed the pylint pre-commit job is failing, I suppose we should look at that first |
|
Ah yeah because of this, right? #8861 (comment) |
I think the idea was/is that by allowing only one "environment" to create the cache we avoid this issue. The But I agree, feel free to merge. I'm on holiday with minimal internet so have little time to look into a more sustainable solution myself. |
|
I've been pushing for not using bleeding edge if external repository in primers for a long time, mainly because that way if something change in the result then you would know it must come from pylint. If using bleeding edge also invalidates the cache as soon as there is a new commit in one of the many very popular repo we're using (and it make sense that it does) then using bleeding edge is also making the cache useless and pipelines dramatically slower. I would be strongly in favor of setting the primed version by git sha and discussing the best way to upgrade those periodocally. What do you think @DanielNoord ? |
|
All of those concerns can be/were avoided by only allowing Can't we just increase the retention period? |
I haven’t seen anything in the docs about increasing retention period (apart from simply accessing it at least once per week) but perhaps we could add a workflow which removes caches which are no longer of use (e.g. venv caches created by a PR branch which has been closed or merged to main). Could also see if we can have only one primer package cache at any time and remove others (needs some more thought). |
Sure, but there's some thing to consider to implement the caching correctly on main cached with implicit bleeding edge (which do not seem to be taken into account atm). Some feature branches live for a long time and the closest common commit between main and the feature branch can become old (unavailable in cache ?). If new commits were made on main then there's an issue between the cache from the latest common commit might be too old and invalidated but the latest commit on main is having some unrelated changes from other pylint pull requests. I think it caused issue in the past. Having the git sha being explicitly used in the cache hash and having most pipeline using the same cache unless we upgrade the primed repository or the dependencies seems like it would prevent headaches at implementation. Let me know if I'm mistaken and this cache mechanism is already implemented. But then even if it was already implemented perfectly; there's a second point that will still stand : the number of cached image should be a lot lower with explicit shas (one every time we upgrade a dependency or the primed repo shas, instead of one every time we commit on main, basically) so there should be less cache miss and less image created from scratch overall. |
Yeah this can of course happen. Ideally we would just run the primer twice, but that is too expensive (now)..
We could consider this, but then I would only do this if we also set up a system to automatically bump the sha's. Testing against outdated code is not really useful and might also be harder to debug. |
Type of Changes
Description