Re: Running iotest linters from check-python-* CI jobs
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-js...@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, af
Re: Running iotest linters from check-python-* CI jobs
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-js...@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. > > 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. 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. I would actually prefer our mypy config to become stricter over time. > 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. > > 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. > > (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 prom
Re: Running iotest linters from check-python-* CI jobs
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-js...@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. 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. 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.) 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. (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.) - 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. 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.) Max --js
Re: Running iotest linters from check-python-* CI jobs
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-js...@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. My main point was that I don’t want to have to have an opinion on this topic. ;) 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.) 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. (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 actually speak against your idea.) Max
