Skip to content

Fix caching issue that led to incorrect all-options.rst being merged on main #10320

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

Merged
merged 6 commits into from
Mar 30, 2025

Conversation

Julfried
Copy link
Contributor

Type of Changes

Type
βœ“ πŸ“œ Docs

Description

Somehow #10314 did not change the .rst file in docs, which messed up pre-commit ci for me

@correctmost
Copy link
Contributor

The top of the file says it is auto-generated, so I thought it would get updated as part of the release process. I didn't encounter any pre-commit failures locally when preparing that PR, so maybe my environment is different somehow?

Copy link

codecov bot commented Mar 29, 2025

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 95.89%. Comparing base (4471242) to head (dc4dfe8).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #10320   +/-   ##
=======================================
  Coverage   95.89%   95.89%           
=======================================
  Files         175      175           
  Lines       19084    19084           
=======================================
  Hits        18301    18301           
  Misses        783      783           
πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Julfried
Copy link
Contributor Author

Julfried commented Mar 29, 2025

As far as I understand the .rst files are generated when running make html before you push. If you have pre-commit hooks installed this should be executed automatically when pushing. Maybe it is also an error with my environment, since ci fails here aswell :)

@Pierre-Sassoulas Pierre-Sassoulas added Documentation πŸ“— Skip news πŸ”‡ This change does not require a changelog entry labels Mar 29, 2025
@Pierre-Sassoulas
Copy link
Member

pre-commit is not regenerating all the doc all the time (tox -e docs / make html is very slow), but I think we check in the CI now (we were simply regenerating before a release before).

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

If there's a mistake in all-options it means the mistake is elsewhere too as this file is generated.

@Julfried
Copy link
Contributor Author

Julfried commented Mar 29, 2025

Maybe I need to clarify. I think what happened is that in #10314 the description in typecheck.py was changed from minimum to maximum, however make html was never executed, which left all-options.rst unchanged. So when I wanted to push something in my fork, running the pre-commit hooks complained that executing make html produced a diff for me and preventing me to push

@Pierre-Sassoulas
Copy link
Member

What is strange is that the CI runs a check in Checks/ Documentation (and also runs pre-commit) so, we have the branch for #10314 passing test, we merge it then main fail, and then fixing it here fail the check again (while supposedly working for you locally). Do you mind resetting your local installation to make sure the version of pylint used to generate the doc is the right one ?

@Julfried
Copy link
Contributor Author

Julfried commented Mar 29, 2025

I pulled the main branch on a different machine with a fresh environment. Running make html still produces this diff for me:

diff --git a/doc/user_guide/configuration/all-options.rst b/doc/user_guide/configuration/all-options.rst
index 7a9cec81a..82ade5b52 100644
--- a/doc/user_guide/configuration/all-options.rst
+++ b/doc/user_guide/configuration/all-options.rst
@@ -1501,7 +1501,7 @@ Standard Checkers

 --missing-member-hint-distance
 """"""""""""""""""""""""""""""
-*The minimum edit distance a name should have in order to be considered a similar match for a missing member name.*
+*The maximum edit distance a name should have in order to be considered a similar match for a missing member name.*
:

@Pierre-Sassoulas
Copy link
Member

I can reproduce the issue locally (a diff on main, no diff on this branch when regenerating the doc). We'll end up merging it but I have no idea what is going wrong in CI atm.

This comment has been minimized.

@Julfried
Copy link
Contributor Author

Julfried commented Mar 29, 2025

I was wondering if this is a caching issue with pre-commit? Because, when looking at the logs for the first run when I opened this PR, it somehow wanted to revert my changes based on the git diff that is produced. However this can not be from the right source code because typecheck.py was updated in my branch, so maybe pre-commit has cached older files and running pre-commit run --hook-stage push sphinx-generated-doc --all-files in CI uses that?

logs:

2025-03-29T09:57:00.0543760Z git diff:
2025-03-29T09:57:00.0544058Z diff --git a/doc/user_guide/configuration/all-options.rst b/doc/user_guide/configuration/all-options.rst
2025-03-29T09:57:00.0544146Z index 82ade5b..7a9cec8 100644
2025-03-29T09:57:00.0544264Z --- a/doc/user_guide/configuration/all-options.rst
2025-03-29T09:57:00.0544378Z +++ b/doc/user_guide/configuration/all-options.rst
2025-03-29T09:57:00.0544584Z @@ -1501,7 +1501,7 @@ Standard Checkers
2025-03-29T09:57:00.0544650Z  
2025-03-29T09:57:00.0544748Z  --missing-member-hint-distance
2025-03-29T09:57:00.0544816Z  """"""""""""""""""""""""""""""
2025-03-29T09:57:00.0545108Z -*The maximum edit distance a name should have in order to be considered a similar match for a missing member name.*
2025-03-29T09:57:00.0545384Z +*The minimum edit distance a name should have in order to be considered a similar match for a missing member name.*
2025-03-29T09:57:00.0545447Z  
2025-03-29T09:57:00.0545516Z  **Default:**  ``1``
2025-03-29T09:57:00.0545580Z  
2025-03-29T09:57:00.0545647Z End of 'git diff'

@Julfried
Copy link
Contributor Author

This would also explain why the check is passing now when using tox. What do you think?

@Pierre-Sassoulas
Copy link
Member

I agree it must be a caching issue but it's probably not from pre-commit but from our 'check documentation' github action caching.

@Julfried
Copy link
Contributor Author

What do you think of replacing setup-python with uv in this job? In my experience lately, uv without cache is as fast as setup-python + cache, but we would avoid having to deal with these caching side effects

@Pierre-Sassoulas
Copy link
Member

Sounds good (tox is slower than make because it create the whole env before launching make, bypassing the cache)

@Julfried Julfried marked this pull request as ready for review March 30, 2025 10:07
@Julfried
Copy link
Contributor Author

Julfried commented Mar 30, 2025

Alright, should we do it here or in a follow up?

@Pierre-Sassoulas
Copy link
Member

I'm wondering if caching the result of an editable install is a problem. Maybe we need to install the current pylint outside of the cached mechanism. (The problem also appeared in the test here: https://github.com/pylint-dev/pylint/actions/runs/14155616302/job/39654454133?pr=9962)

@Pierre-Sassoulas
Copy link
Member

Doing it here is fine, I see this PR as "fixing the cache and the generated doc that was broken by the cache issue"

@DanielNoord
Copy link
Collaborator

Locally without this commit pre-commit run --hook-stage push sphinx-generated-doc --all-files also doesn't create a git diff for me.

I think we just shouldn't use pre-commit here.

@Pierre-Sassoulas Pierre-Sassoulas changed the title Fix incorrect description for --missing-member-hint-distance in all-options.rst Fix caching issue that led to incorrect all-options.rst being merged on main Mar 30, 2025
@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Mar 30, 2025
@Pierre-Sassoulas
Copy link
Member

Locally without this commit pre-commit run --hook-stage push sphinx-generated-doc --all-files also doesn't create a git diff for me.

Personally, on the contrary I have a diff on main or branch rebased on main and I don't have a diff on this branch. Not sure about the reason for the discrepancy here.

I think we just shouldn't use pre-commit here.

We can remove the pre-commit though as probably no one install it and it's very slow on every push even unrelated to doc chances it's a nightmate. (I use tox -e docs only on doc change personally, but there's 3 ways to do it right now, which is bad)

@@ -180,11 +180,10 @@ jobs:
- name: Check documentation build and links
run: |
. venv/bin/activate
cd doc
pre-commit run --hook-stage push sphinx-generated-doc --all-files || {
tox -e docs || {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure this is correct? Wouldn't this always exit 1?

Copy link
Member

Choose a reason for hiding this comment

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

If tox -e docs is true this exit with 0

DanielNoord
DanielNoord previously approved these changes Mar 30, 2025
@Pierre-Sassoulas Pierre-Sassoulas dismissed their stale review March 30, 2025 19:16

Took over this MR, not in a position to review anymore.

@Pierre-Sassoulas
Copy link
Member

@Julfried the uv refactor is still welcome but let's do it in a follow-up MR, using tox is not efficient but it's a fast fix that could be permanent and it will unblock the other PR.

@Pierre-Sassoulas Pierre-Sassoulas enabled auto-merge (squash) March 30, 2025 19:27
@Julfried
Copy link
Contributor Author

@Pierre-Sassoulas thank you, will do it :)

@Pierre-Sassoulas Pierre-Sassoulas merged commit ce7346b into pylint-dev:main Mar 30, 2025
35 checks passed
@Julfried Julfried deleted the fix-docs branch March 30, 2025 19:34
Copy link
Contributor

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit dc4dfe8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation πŸ“— Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news πŸ”‡ This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants