Re: Running iotest linters from check-python-* CI jobs

[John Snow]

On 6/23/21 6:55 AM, Kevin Wolf wrote:
> Am 22.06.2021 um 18:24 hat John Snow geschrieben:
> > On 6/22/21 11:52 AM, Max Reitz wrote:
> > > On 22.06.21 16:57, John Snow wrote:
> > > > Hi Kevin:
> > > >
> > > > At one point I had the idea to augment the Python linter CI jobs to
> > > > also run the iotest linters. I thought it would be convenient to
> > > > ensure that while I changed around the QMP and Machine packages that
> > > > I didn't introduce regressions in iotest 297 either.
> > > >
> > > > I sent an RFC, got feedback from Vladimir (Who seemed broadly in
> > > > favor of the idea), and then wrote a v2 that I never sent.
> > > >
> > > > RFC: Message-Id: <20210604163907.1511224-1-jsnow@redhat.com>
> > > >
> > > > mreitz stated (on IRC) in no uncertain terms they were not happy
> > > > with the idea of 297 becoming gating CI, so I held off on pursuing
> > > > the idea. I wanted to reach out and see if you had feelings on the
> > > > matter, or if I should indeed just shelve it entirely.
>
> I like the general idea of making such checks gating CI, because if we
> already have them, what are we gaining by only manually finding
> violations?
>
> The more interesting part is defining our standards, i.e. writing config
> files for the tools, and we already do that for 297 anyway.
>
> The potential problem I could see is different linter versions, but you
> already addressed that below.

(Through great personal pain, I assure you. Why do you think I have 
cooled off on pylint so drastically?...)

> > > My main point was that I don’t want to have to have an opinion on this
> > > topic. ;)
> >
> > Sorry if I put words in your mouth! I wanted to take your feedback/reaction
> > seriously.
> >
> > > It’s true that I’m not happy about linters being part of gating CI, but
> > > I also stated that I cannot defend this gut feeling, and that I feel
> > > like it’s “objectively” wrong.  Therefore, I don’t want to be part of
> > > such a discussion, if I can avoid it.
> > > (To my defense, in virtiofsd-rs I myself made a linter part of the
> > > gating CI.  That’s because we already had another linter in it, and
> > > because my gut feeling is much easier to suppress when it’s about a
> > > small project with few maintainers to annoy.  It has nothing to do with
> > > me hating Python coding style guidelines, because I probably hate Rust
> > > coding style guidelines just as much.)
> >
> > 😅
> >
> > I'll fully admit that pylint in particular is very, very annoying. My RFC
> > does not increase the strictness of its use for iotests, at least.
>
> Yeah, I'm not sure if pylint ever warned about something that I actually
> cared to get changed... Most times it's just failing to meet some
> questionable arbitrary style requirements.

I've seen it say something useful here and again ... though after 
converting everything to mypy strict, as you say, it's generally not as 
useful. It seems most useful for cleaning up old python2 era code, but 
less so for actively written and loved python3 code.

I have grown way less attached to it after my efforts to clean up all 
the Python in the tree. Still, I have some... vague attachment to the 
idea that enforcing a style guide is "nice" for consistency reasons.

Maybe I'll drop it eventually, or just continue to use it with rather 
permissive configurations, I don't know. Nothing to worry about today, I 
think.

> On the other hand, I assume you count mypy as a linter, too, and the
> messages of that one I treat more like compiler warnings or errors. They
> are actually useful, and if your code doesn't pass, then I usually do
> care about it getting fixed.

The "linters" I run in the Python jobs right now are mypy, pylint, 
isort, and flake8. It's all the stuff in python/tests/.

> I would actually prefer our mypy config to become stricter over time.

The mypy config for python/ and (most of, and soon to be all) 
scripts/qapi/ is 100% strict.

It probably wouldn't be *so* bad to finish strictifying all of the 
non-unit-test files for iotests -- I had a hack/WIP series I posted 
about a year ago that tried to do most of it.

I have since learned a few tricks while doing the qapi series to turn 
strictness on/off for individual modules which might allow us to do a 
more gradual conversion. IIRC there was a QED or a qcow module that 
liberally used dynamic typing that was non-trivial to convert to a 
statically typed subset of python. Perfectly reasonable code, just not 
to mypy.

> > There are three linting standards for Python in the tree right now:
> >
> > 1. Those applied to scripts/qapi/  (Manually run only)
> > 2. Those applied to tests/iotests/ (via 297)
> > 3. Those applied to python/qemu/   (via CI)
> >
> > The python/qemu/ ones are the strictest and most annoying. scripts/qapi/ has
> > an almost identical set of rules that will be integrated to python/qemu/
> > once I move the QAPI generator there.
> >
> > The iotests ones are separate and I intend to keep separate -- I think it
> > should remain up to the block maintainers what their own tolerance level for
> > annoying yappy errors are. I have no desire to change that.
> >
> > (I definitely have no desire to scrub and audit everything in iotests to
> > bring it up to speed with the stricter standard. They're just tests, after
> > all. It's not worth it.)
>
> Right, individual tests aren't that important, especially concerning
> style, though I feel shared files like iotests.py and the test
> infrastructure itself are probably worth it.

Yep. I also want to increasingly move certain utilities and 
VM-management functions that make sense to have everywhere into 
python/qemu/machine over time.

(An idea I have in general is to refactor the QMP and Machine classes to 
be more extensible with logging hooks so that more functionality can be 
added to the base classes, and iotests just extends with custom logging 
functions for them. A topic for another day.)

> > > In any case, I had understood you wanted to make 297 part of the
> > > non-gating CI anyway, though, so I wonder what of the things I said made
> > > you shelve that idea.
> >
> > I just don't like pursuing things that might increase your maintenance
> > burden or make your day worse. I know you don't want to be involved, but
> > this kind of necessarily involves you at least indirectly, so ... It
> > genuinely felt a little rude to press onward without getting a bit more
> > information first.
> >
> > I figured I'd ask Kevin what his feelings are to see if that
> > un-muddies the waters.
>
> So my hope is that it would in fact decrease the maintenance burden
> because we would catch bugs in the tests in time, and dealing with false
> positives would cost us less time than dealing with such bugs.
>
> But then, this is something that is mostly a point for mypy, not for
> pylint.

Right, which is why I suggest that I won't apply the standards of 
python/ to tests/iotests/ ... I think your case really does warrant its 
own config that isn't 100% strict across the board. And it can be tuned 
according to *your* desires, so you can decide what's worth your hassle 
and what isn't.

> > > (Another concern I had was linter updates breaking CI, but you promised
> > > to keep the linter in a steady configuration so this wouldn’t happen. So
> > > all in all, I can’t remember I brought any argument that would ac
> > > buttually speak against your idea.)
> >
> > Right. I can't promise stability for iotest 297 itself, because that test is
> > designed to run with "whatever the person running it happens to have
> > installed", which I can't control. But we can't control that right now
> > anyway, so that's not a regression.
> >
> > I *can* control the CI environment, though. There are two python linting
> > jobs that run in CI now:
> >
> > - check-python-pipenv is a CI job that runs against Python 3.6 and a very
> > specific pinned list of dependencies (and their dependencies). These
> > packages do not change unless we/I change them explicitly. It should offer
> > stability to the linting environment in CI. Right now, this job is a
> > "must-pass" CI job.
> >
> > (If you have the right packages, you can run the same exact test locally
> > with 'make venv-check' in the python/ directory.)
>
> So this would be the one additional thing that I should regularly run
> before sending a pull request?

"Yes",

It runs the exact same test as check-python-pipenv using the exact same 
interpreter and exact same package/dependency versions.

If this passes, the check-python-pipenv CI job will also pass.

To run it, though, you need Python 3.6 and pipenv installed. I realize 
this is not always easy to do on certain distributions that do not 
provide multiple Python versions like Fedora does.

So the "next best thing" is to do something like this:

> cd ~/src/qemu/python/
> python3 -m venv ~/.cache/qemu-venv
> source ~/.cache/qemu-venv/bin/activate.fish
(qemu-venv) > make develop
(qemu-venv) > make check
> deactivate

That'll run the tests on whichever python version you have, on whichever 
linter versions happen to fulfill the package criteria. It's less 
targeted, but it SHOULD still pass. If it doesn't, it's a problem that 
ought be brought to my attention. (It's the type of test I have marked 
as may-fail currently, because the package versions are unpredictable 
and may change day-to-day.)

(I have some plans to split 'venv-check' into 'check-venv' and 
'check-pipenv' so that both invocations are readily accessible, I just 
haven't gotten there yet. It's been a busy release cycle for the Python 
code.)

> > - check-python-tox is a CI job that runs against Python 3.6 through 3.10
> > inclusive. The Python versions it tests against are manually configured. The
> > versions of the linters (and their dependencies) it uses are always "the
> > latest ones that fulfill the dependency criteria". This CI job, however, is
> > allowed to fail with a warning.
>
> What does "warning" mean here? Do all linter failures just result in CI
> warnings, or is this the difference between errors and warnings that
> linters are making? So we would ignore only warnings, but errors would
> still make the CI job fail?

This: https://docs.gitlab.com/ee/ci/yaml/#allow_failure

The job will fail and instead of a red [X] you'll see an orange (!).

Here's an example for 'acceptance-system-debian' which failed on an 
older pipeline of mine: https://gitlab.com/jsnow/qemu/-/jobs/1341018158

On the right-hand side status bar, you can see "(!) Pipeline #319584565 
for python" indicating that CI passed, but with failures. Clicking the 
Pipeline number brings you to the CI overview that shows the status as 
"(!) passed".

> > My belief was that it would be useful to find out about new linter or python
> > incompatibilities without holding up the build. They're likely things that
> > will eventually show up in people's manual invocations of 297 as they
> > upgrade their distro, their packages, etc. But it is definitely not a gating
> > test. Just a heads up thing.
> >
> > (If you have the right packages, you can run the same exact test locally
> > with 'make check-tox' in the python/ directory.)
>
> Makes sense to me as a heads up thing.
>
> So I should run this one locally every once in a while, or just check CI
> results of a pull request after it went in?

The way I'm using it for python/ right now is that I usually run "make 
venv-check; and make check-tox", then push to GitLab and keep an eye on 
the pipeline status before I send my PRs. Adding 297 to those jobs 
primarily helps me (and Niteesh, and anyone else who wants to hack on 
the python/ folder) make sure that we didn't break the largest user of 
all the python/ stuff.

In your case, what I was planning to do was to leave iotest 297 in-tact 
so you wouldn't actually have to run a new thing at all, and 297 passing 
locally would be reasonable due diligence. Nothing new for you beyond 
297 getting run as part of CI to help me ensure stability of the python 
packages.

> Kevin

Looks like you have some interest in this, so I'll go ahead and 
polish/send that v2 and you can just play around with it and see what 
you think.

Sincerely,
  John "More information than you require" Snow