The fate of iotest 297

The fate of iotest 297

[John Snow]

Hi Kevin,

I remember that you wanted some minimum Niceness threshold in order to
agree to me removing iotest 297.

I've already moved it onto GitLab CI in the form of the
check-python-pipenv job, but I recall you wanted to be able to run it
locally as well before agreeing to axe 297. I remember that you didn't
think that running "make check-pipenv" from the python directory was
sufficiently Nice enough.

Do you need it to be part of "make check", or are you OK with
something like "make check-python" from the build directory?

I have a bit more work to do if you want it to be part of 'make check'
(if you happen to have the right packages installed), but it's pretty
easy right now to give you a 'make check-python' (where I just
forcibly install those packages to the testing venv.)

--js

Re: The fate of iotest 297

[Kevin Wolf]

Am 18.05.2022 um 01:28 hat John Snow geschrieben:
> Hi Kevin,
> 
> I remember that you wanted some minimum Niceness threshold in order to
> agree to me removing iotest 297.
> 
> I've already moved it onto GitLab CI in the form of the
> check-python-pipenv job, but I recall you wanted to be able to run it
> locally as well before agreeing to axe 297. I remember that you didn't
> think that running "make check-pipenv" from the python directory was
> sufficiently Nice enough.
> 
> Do you need it to be part of "make check", or are you OK with
> something like "make check-python" from the build directory?
> 
> I have a bit more work to do if you want it to be part of 'make check'
> (if you happen to have the right packages installed), but it's pretty
> easy right now to give you a 'make check-python' (where I just
> forcibly install those packages to the testing venv.)

Hm, what is the reason for 'make check-python' not being part of 'make
check'?

I'm currently running two things locally, 'make check' (which is the
generic one that everyone should run) and iotests (for which it is
reasonable enough that I need to run it separately because it's the
special thing for my own subsystem).

Now adding a third one 'make check-python' certainly isn't the end of
the world, but it's not really something that is tied to my subsystem
any more. Having to run test cases separately for other subsystems
doesn't really scale for me, so I would prefer not to start doing that.
I can usually get away with not running the more special tests of other
subsystems before the pull request because I'm unlikely to break things
in other subsystems, but Python style warnings are easy to get.

If we're going to have 'make check-python' separate, but CI checks it,
we'll get pull requests that don't pass it and would then only be caught
by CI after a long test run, requiring a v2 pull request. I feel for
something that checks things like style (and will fail frequently on
code where nobody ran the check before), that's a bit too unfriendly.

Kevin

Re: The fate of iotest 297

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 5599 bytes --]

On Wed, May 18, 2022, 12:37 PM Kevin Wolf <kwolf@redhat.com> wrote:

> Am 18.05.2022 um 01:28 hat John Snow geschrieben:
> > Hi Kevin,
> >
> > I remember that you wanted some minimum Niceness threshold in order to
> > agree to me removing iotest 297.
> >
> > I've already moved it onto GitLab CI in the form of the
> > check-python-pipenv job, but I recall you wanted to be able to run it
> > locally as well before agreeing to axe 297. I remember that you didn't
> > think that running "make check-pipenv" from the python directory was
> > sufficiently Nice enough.
> >
> > Do you need it to be part of "make check", or are you OK with
> > something like "make check-python" from the build directory?
> >
> > I have a bit more work to do if you want it to be part of 'make check'
> > (if you happen to have the right packages installed), but it's pretty
> > easy right now to give you a 'make check-python' (where I just
> > forcibly install those packages to the testing venv.)
>
> Hm, what is the reason for 'make check-python' not being part of 'make
> check'?
>

Oh, it just needs more logic so that it performs correctly in RPM building
environments. As a manual test, I'm free to just grab stuff from PyPI and
build a venv to some precise specification and automate it. This is how
"check-pipenv" and "check-tox" work. The RPM environment can't dial out to
PyPI, so it shouldn't try any venv-based tests by default.

To wire it up to "make check" by *default*, I believe I need to expand the
configure script to poll for certain requisites and then create some
wrapper script of some kind that only engages the python tests if the
requisites were met ... and I lose some control over the mypy/pylint
versioning windows. I have to tolerate a wider versioning, or it'll never
get run in practice.

I have some reluctance to doing this, because pylint and mypy change so
frequently that I don't want "make check" to fail spuriously in the future.

(In practice, these failures occur 100% of the time when I am on vacation.)

The gitlab ci job check-python-tox pulls whatever the latest and greatest
are, and these jobs fail so constantly we had to mark the job as optional.
The check-pipenv job by contrast is extremely stable (its still must-pass)
because it can concoct its own lil' universe.

So, I can add something to make check by default but it needs some
scaffolding to skip the test based on environment, and I have some
reliability concerns.

Ultimately, I don't believe tolerating a wide matrix for mypy/pylint really
adds any value to 297; it only really matters if a specific environment
comes up green, and that a developer like you or I can replicate that test
locally and quickly.

That said ... maybe I can add a controlled venv version of "check-python"
and just have a --disable-check-python or something that spec files can opt
into. Maybe that will work well enough?

i.e. maybe configure can check for the presence of pip, the python venv
module (debian doesnt ship it standard...), and PyPI connectivity and if
so, enables the test. Otherwise, we skip it.

Something like that.



> I'm currently running two things locally, 'make check' (which is the
> generic one that everyone should run) and iotests (for which it is
> reasonable enough that I need to run it separately because it's the
> special thing for my own subsystem).
>

Pretty much exactly what I do. (Except I run the python tests these days,
too.)


> Now adding a third one 'make check-python' certainly isn't the end of
> the world, but it's not really something that is tied to my subsystem
> any more. Having to run test cases separately for other subsystems
> doesn't really scale for me, so I would prefer not to start doing that.
> I can usually get away with not running the more special tests of other
> subsystems before the pull request because I'm unlikely to break things
> in other subsystems, but Python style warnings are easy to get.
>

Reasonable. I already forget to run things like avocado and vm tests, and I
am sympathetic to not wanting to expand the list of manually run tests.

(What avocado and vm tests have in common is that they need to fetch stuff
from the internet, which I am learning makes them unsuitable for make
check, which must work without internet. """Coincidentally""", tests that
require internet seem to break an awful lot more often because they are
getting run a lot less and in fewer places.)


> If we're going to have 'make check-python' separate, but CI checks it,
> we'll get pull requests that don't pass it and would then only be caught
> by CI after a long test run, requiring a v2 pull request. I feel for
> something that checks things like style (and will fail frequently on
> code where nobody ran the check before), that's a bit too unfriendly.
>
> Kevin
>

Got it. I'll see what I can come up with that checks the boxes for
everyone, thanks for clarifying yours.

I want to make everything "just work" but I'm also afraid of writing too
much magic crap that could break and frustrate people who have no desire to
understand python packaging junk, so I'm trying to balance that.

(Parting thought: the python ecosystem mantra of "just use a venv!"
unfortunately influences a lot of upstream developer attitude which is then
increasingly hard to square with environments in which I cannot count on
internet or the ability to spin up a venv, and thus my headache.
Overwhelmingly, it seems to be the expectation that you'd just pin or
vendor things like pylint/mypy, because they're not usually runtime deps
for Python packages. Urgh, blegh.)

--js

>

[-- Attachment #2: Type: text/html, Size: 7900 bytes --]

Re: The fate of iotest 297

[Kevin Wolf]

Am 18.05.2022 um 20:21 hat John Snow geschrieben:
> To wire it up to "make check" by *default*, I believe I need to expand the
> configure script to poll for certain requisites and then create some
> wrapper script of some kind that only engages the python tests if the
> requisites were met ... and I lose some control over the mypy/pylint
> versioning windows. I have to tolerate a wider versioning, or it'll never
> get run in practice.
> 
> I have some reluctance to doing this, because pylint and mypy change so
> frequently that I don't want "make check" to fail spuriously in the future.
> 
> (In practice, these failures occur 100% of the time when I am on vacation.)

So we seem to agree that it's something that we do expect to fail from
time to time. Maybe this is how I could express my point better: If it's
a hard failure, it should fail as early as possible - i.e. ideally
before the developer sends a patch, but certainly before failing a pull
request.

This allows another option that would work for me (but probably not for
you): Don't make it a hard failure. In practice, it would probably mean
that you end up fixing up things after people like we sometimes have to
do for the non-auto iotests.

> That said ... maybe I can add a controlled venv version of "check-python"
> and just have a --disable-check-python or something that spec files can opt
> into. Maybe that will work well enough?
> 
> i.e. maybe configure can check for the presence of pip, the python venv
> module (debian doesnt ship it standard...), and PyPI connectivity and if
> so, enables the test. Otherwise, we skip it.

I think this should work. If detecting the right environment is hard, I
don't think there is even a requirement to do so. You can make
--enable-check-python the default and if people don't want it, they can
explicitly disable it. (I understand that until you run 'make check', it
doesn't make a difference anyway, so pure users would never have to
change the option, right?)

> Got it. I'll see what I can come up with that checks the boxes for
> everyone, thanks for clarifying yours.
> 
> I want to make everything "just work" but I'm also afraid of writing too
> much magic crap that could break and frustrate people who have no desire to
> understand python packaging junk, so I'm trying to balance that.

Yes, sounds like we need to find some balance there. Test infrastructure
breaking locally for no obvious reason can be quite frustrating. But
sending a patch and getting it queued, only to be notified that it's
dropped again because of a mypy problem two weeks later when the
maintainer sends the pull request, can be equally (if not even more)
frustrating.

Kevin

Re: The fate of iotest 297

[Daniel P. Berrangé]

On Thu, May 19, 2022 at 09:54:56AM +0200, Kevin Wolf wrote:
> Am 18.05.2022 um 20:21 hat John Snow geschrieben:
> > To wire it up to "make check" by *default*, I believe I need to expand the
> > configure script to poll for certain requisites and then create some
> > wrapper script of some kind that only engages the python tests if the
> > requisites were met ... and I lose some control over the mypy/pylint
> > versioning windows. I have to tolerate a wider versioning, or it'll never
> > get run in practice.
> > 
> > I have some reluctance to doing this, because pylint and mypy change so
> > frequently that I don't want "make check" to fail spuriously in the future.
> > 
> > (In practice, these failures occur 100% of the time when I am on vacation.)
> 
> So we seem to agree that it's something that we do expect to fail from
> time to time. Maybe this is how I could express my point better: If it's
> a hard failure, it should fail as early as possible - i.e. ideally
> before the developer sends a patch, but certainly before failing a pull
> request.

At least with pylint we can make an explicit list of which lint
checks we want to run, so we should not get new failures when a
new pylint is released. If there are rare cases where we none
the less see a new failure from a new release, then so be it,
whoever hits it first can send a patch. IOW, I think we should
just enable pylint all the time with a fixed list of tests we
care about. Over time we can enable more of its checks when
desired.

I don't know enough about mypy to know if it can provide similar
level of control. Possibly the answer for "should we run it by default"
will be different for pylint vs mypy.

Still, none of this is all that different from the case where
new GCC or CLang are released and developers find new warnings
have arrived. People just send patches when they hit this.
Given python is a core part of QEMU's dev tooling, I think it
is reasonable to expect developers to cope with this for python
too, as long as the frequency of problems is not unreasonably
high.

> > That said ... maybe I can add a controlled venv version of "check-python"
> > and just have a --disable-check-python or something that spec files can opt
> > into. Maybe that will work well enough?
> > 
> > i.e. maybe configure can check for the presence of pip, the python venv
> > module (debian doesnt ship it standard...), and PyPI connectivity and if
> > so, enables the test. Otherwise, we skip it.
> 
> I think this should work. If detecting the right environment is hard, I
> don't think there is even a requirement to do so. You can make
> --enable-check-python the default and if people don't want it, they can
> explicitly disable it. (I understand that until you run 'make check', it
> doesn't make a difference anyway, so pure users would never have to
> change the option, right?)

I think it should just be the default too. Contributors have to accept
that python is a core part of our project and we expect such code to
pass various python quality control tests, on the wide variety of OS
platforms we run on.

> > Got it. I'll see what I can come up with that checks the boxes for
> > everyone, thanks for clarifying yours.
> > 
> > I want to make everything "just work" but I'm also afraid of writing too
> > much magic crap that could break and frustrate people who have no desire to
> > understand python packaging junk, so I'm trying to balance that.
> 
> Yes, sounds like we need to find some balance there. Test infrastructure
> breaking locally for no obvious reason can be quite frustrating. But
> sending a patch and getting it queued, only to be notified that it's
> dropped again because of a mypy problem two weeks later when the
> maintainer sends the pull request, can be equally (if not even more)
> frustrating.

The other benefit to having the checks turned on by default for everyone
is that it removes John as the centralized point responsible for finding,
investigating and addressing python style issues, and distributes it
amongst all the QEMU contributors.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: The fate of iotest 297

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 6403 bytes --]

On Thu, May 19, 2022, 4:25 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, May 19, 2022 at 09:54:56AM +0200, Kevin Wolf wrote:
> > Am 18.05.2022 um 20:21 hat John Snow geschrieben:
> > > To wire it up to "make check" by *default*, I believe I need to expand
> the
> > > configure script to poll for certain requisites and then create some
> > > wrapper script of some kind that only engages the python tests if the
> > > requisites were met ... and I lose some control over the mypy/pylint
> > > versioning windows. I have to tolerate a wider versioning, or it'll
> never
> > > get run in practice.
> > >
> > > I have some reluctance to doing this, because pylint and mypy change so
> > > frequently that I don't want "make check" to fail spuriously in the
> future.
> > >
> > > (In practice, these failures occur 100% of the time when I am on
> vacation.)
> >
> > So we seem to agree that it's something that we do expect to fail from
> > time to time. Maybe this is how I could express my point better: If it's
> > a hard failure, it should fail as early as possible - i.e. ideally
> > before the developer sends a patch, but certainly before failing a pull
> > request.
>
> At least with pylint we can make an explicit list of which lint
> checks we want to run, so we should not get new failures when a
> new pylint is released. If there are rare cases where we none
> the less see a new failure from a new release, then so be it,
> whoever hits it first can send a patch. IOW, I think we should
> just enable pylint all the time with a fixed list of tests we
> care about. Over time we can enable more of its checks when
> desired.
>

Yeh, this might help a bit. If we use system packages by default, we'll
also generally avoid using bleeding edge packages and I'll (generally)
catch those myself via check-tox before people run into them organically.


> I don't know enough about mypy to know if it can provide similar
> level of control. Possibly the answer for "should we run it by default"
> will be different for pylint vs mypy.
>

Yeah, we can probably do different things. mypy is actually much more
stable than pylint IMO, it's probably actually okay to just let that one
behave as-is.

(I know I have a fix for 0.950 in my recent rfc series, but anecdotally I
feel mypy changes behavior a lot less often than pylint. isort and flake8
have basically never ever broken on update for me, either.)

Still, none of this is all that different from the case where
> new GCC or CLang are released and developers find new warnings
> have arrived. People just send patches when they hit this.
> Given python is a core part of QEMU's dev tooling, I think it
> is reasonable to expect developers to cope with this for python
> too, as long as the frequency of problems is not unreasonably
> high.
>

To some extent, though it's still a bummer to get warnings and errors that
have nothing to do with your changes. I have made sure I test a wide matrix
to the best of my ability, so it should be fine. I guess I'm just super
conservative about it ...

(Well, and even when I had the check-tox test set to allow failure, the
yellow exclamation mark still annoyed people. I'm just keen to avoid more
nastygrams.)


> > > That said ... maybe I can add a controlled venv version of
> "check-python"
> > > and just have a --disable-check-python or something that spec files
> can opt
> > > into. Maybe that will work well enough?
> > >
> > > i.e. maybe configure can check for the presence of pip, the python venv
> > > module (debian doesnt ship it standard...), and PyPI connectivity and
> if
> > > so, enables the test. Otherwise, we skip it.
> >
> > I think this should work. If detecting the right environment is hard, I
> > don't think there is even a requirement to do so. You can make
> > --enable-check-python the default and if people don't want it, they can
> > explicitly disable it. (I understand that until you run 'make check', it
> > doesn't make a difference anyway, so pure users would never have to
> > change the option, right?)
>
> I think it should just be the default too. Contributors have to accept
> that python is a core part of our project and we expect such code to
> pass various python quality control tests, on the wide variety of OS
> platforms we run on.
>

I meant that I'd have the default be "auto", but if you're arguing for the
default to be "on", I suppose I could. I have a weak preference for keeping
the min requisites for a no-option configure set small. This should be
trivial to change in either direction, though.

The requisites aren't steep: you just need python and the venv stdlib
module. If you have python, you meet that requisite on every platform
except debian/ubuntu, which ships venv separately. In practice, it probably
will be enabled for most people by default.


> > > Got it. I'll see what I can come up with that checks the boxes for
> > > everyone, thanks for clarifying yours.
> > >
> > > I want to make everything "just work" but I'm also afraid of writing
> too
> > > much magic crap that could break and frustrate people who have no
> desire to
> > > understand python packaging junk, so I'm trying to balance that.
> >
> > Yes, sounds like we need to find some balance there. Test infrastructure
> > breaking locally for no obvious reason can be quite frustrating. But
> > sending a patch and getting it queued, only to be notified that it's
> > dropped again because of a mypy problem two weeks later when the
> > maintainer sends the pull request, can be equally (if not even more)
> > frustrating.
>
> The other benefit to having the checks turned on by default for everyone
> is that it removes John as the centralized point responsible for finding,
> investigating and addressing python style issues, and distributes it
> amongst all the QEMU contributors.
>

Haha. I won't hold my breath for anyone else to have patience enough to
track it down, but I appreciate the sentiment :)

Thanks for the feedback. I'm still working on my RFC for the actual unit
testing venv but ran into some difficulties with vm tests and Avocado tests
being a little flaky, and testing has been slow.

So: v2 RFC coming up soon and I'll put code and test results to all these
worrrrrrrrrds.

--js

[-- Attachment #2: Type: text/html, Size: 8476 bytes --]