seemingly random changes in pre-commit

[ltalirz]

Something strange happened in PR #3875

After some time working on the PR, pylint suddenly started complaining about invalid variable names in .ci/workchains.py (a file untouched by the PR):

aiida-core/.ci/workchains.py

Lines 71 to 72 in 035a9da

	self.ctx.inputs.x = Int(abs(node.inputs.x.value))
	self.ctx.inputs.y = Int(abs(node.inputs.y.value))

I added a commit that added # pylint: disable=invalid-name comments and the problem went away.

@sphuber in the end rebased my PR, somehow getting rid of these additional changes in the process. So far, so good.

The strange thing is: The pre-commit check on the final commit of the PR passed, despite the fact the comments were no longer there.

It seems we are witnessing the same again in PR #3738 - pre-commit now started complaining about these two lines again.

@csadorf Do you have any idea what could be going on?

[ltalirz]

Just to follow up on this, here are links to recent github actions runs:

• passed (despite the # pylint:disable comments missing)
• failed

Looking at the python environment, the only differences are:

7c7
< Werkzeug-1.0.0
---
> Werkzeug-1.0.1
42c42
< django-2.2.11
---
> django-2.2.12
84c84
< nbformat-5.0.4
---
> nbformat-5.0.5

I don't think those packages have anything to do with it.
Also, if the reason for this would be updates of python dependencies, this would not explain why the pre-commit hooks worked, then broke, then worked again, then broke again.

[csadorf]

@ltalirz The only thing that I can imagine is that there were somehow different versions of pylint installed locally and remotely, but that's very weird, since pylint is explicitly pinned to prevent this.

[ltalirz]

@ltalirz The only thing that I can imagine is that there were somehow different versions of pylint installed locally and remotely

I'm referring exclusively to what happens on Github actions here.

[csadorf]

@ltalirz I also looked at the differences in the Python environments, and found the exact same that you found. Absolutely no clue what could be going on here since none of the packages that differed are dependencies of pylint.

[giovannipizzi]

BTW, I saw the same exact issue also in #3650

[csadorf]

@giovannipizzi How did you resolve it? Also rebase?

[giovannipizzi]

No, I actually added # pylint: disable=invalid-name by hand (I noticed this issue only later).
You don't see this as a single commit because I used git commit --amend and git push -f

[csadorf]

@giovannipizzi Since all tests pass we should probably not mess with #3650 any further, but should this issue occur again, I propose that we rebase on develop as a first attempt to resolve this.

[ltalirz]

I'm now getting this warning in #3894

.ci/workchains.py
  Line: 71
    pylint: invalid-name / Attribute name "x" doesn't conform to '(([a-z][a-z0-9_]{2,32})|(_[a-z0-9_]*))$' pattern (col 8)
  Line: 72
    pylint: invalid-name / Attribute name "y" doesn't conform to '(([a-z][a-z0-9_]{2,32})|(_[a-z0-9_]*))$' pattern (col 8)

despite the fact that the # pylint: disable=invalid-name comments are already in the codebase (added by @giovannipizzi in f40f0d2, here the branch of my fork: https://github.com/ltalirz/aiida-core/blob/issue_3301_input_validation/.ci/workchains.py#L71-L72 ).

@csadorf Given the issues with the git checkout action on the update-requirements workflow, I almost start to suspect that this is related to the git checkout...
That would be bad.

[csadorf]

@ltalirz I looked at the output of the checkout action and found that it checks out a special merge commit e0f7cb9 :

  Note: switching to 'refs/remotes/pull/3894/merge'.
  
  You are in 'detached HEAD' state. You can look around, make experimental
  changes and commit them, and you can discard any commits you make in this
  state without impacting any branches by switching back to a branch.
  
  If you want to create a new branch to retain commits you create, you may
  do so (now or later) by using -c with the switch command. Example:
  
    git switch -c <new-branch-name>
  
  Or undo this operation with:
  
    git switch -
  
  Turn off this advice by setting config variable advice.detachedHead to false
  
  HEAD is now at e0f7cb9 Merge c59c32f823cb4b2285052eeba7d8a4a3afc7c508 into 5da7120645b6cdb751d6db54431ead38417d2345
/usr/bin/git log -1
commit e0f7cb9e4b6a0d074f0b32e36689e9bc34ba8212
Author: Leopold Talirz <leopold.talirz@gmail.com>
Date:   Mon Apr 6 07:53:37 2020 -0500

    Merge c59c32f823cb4b2285052eeba7d8a4a3afc7c508 into 5da7120645b6cdb751d6db54431ead38417d2345

For this commit, the pylint overrides are indeed present: https://github.com/aiidateam/aiida-core/blob/e0f7cb9e4b6a0d074f0b32e36689e9bc34ba8212/.ci/workchains.py#L71-L72

So in conclusion: I have absolutely no clue how it is possible that pylint complains about these two lines.

We could try to run git status or so right before the linter runs.

[ltalirz]

I did a few more tests locally: even if I remove the # pylint: disable comments, I cannot get pylint to complain, neither using:

prospector -w pylint .ci/workchains.py

nor using

pylint .ci/workchains.py

nor using

git commit -a

I also tested (only once, since it takes time):

pre-commit run --all-files

Besides the code being different (which we can check just by adding cat .ci/workchains.py), the only other explanation would seem that we've hit a weird pylint bug here.

If that's the case, the question is: which "disturbance in the force" triggers whether we hit the bug?
There are PRs where pre-commit is passing just fine...