Open Stack

On Mon, Dec 14, 2020 at 09:54, Lee Yarwood <lyarwood at redhat.com> wrote:
> On 13-12-20 16:33:39, Jeremy Stanley wrote:
>>  On 2020-12-13 14:39:58 +0100 (+0100), Luigi Toscano wrote:
>>  > On Saturday, 12 December 2020 00:12:36 CET Jeremy Stanley wrote:
>>  > > On 2020-12-11 20:38:30 +0000 (+0000), Sorin Sbarnea wrote:
>>  > > [...]
>>  > > > Regarding decoupling linting from test-requirements: yes! 
>> This was
>>  > > > already done by some when conflicts appeared. For old 
>> branches I
>>  > > > personally do not care much even if maintainers decide to 
>> disable
>>  > > > linting, their main benefit is on main branches.
>>  > > [...]
>>  > >
>>  > > To be honest, if I had my way, test-requirements.txt files 
>> would die
>>  > > in a fire. Sure it's a little more work to be specific about the
>>  > > individual requirements for each of your testenvs in tox.ini, 
>> but
>>  > > the payoff is that people aren't needlessly installing bandit 
>> when
>>  > > they run flake8 (for example). The thing we got into the PTI 
>> about
>>  > > using a separate doc/requirements.txt is a nice compromise in 
>> that
>>  > > direction, at least.
>>  >
>>  > Wouldn't this mean tracking requirements into two different kind
>>  > of places:the main requirements.txt file, which is still going to
>>  > be needed even for tests, and the tox environment definitions?
>> 
>>  Technically we already do. The requirements.txt file contains actual
>>  runtime Python dependencies of the software (technically
>>  setup_requires in Setuptools parlance). Then we have this vague
>>  test-requirements.txt file which installs everything under the sun
>>  a test might want, including the kitchen sink. Tox doesn't reuse one
>>  virtualenv for multiple testenv definitions, it creates a separate
>>  one for each, so for example...
> 
> That isn't technically true within Nova, multiple tox envs use the
> {toxworkdir}/shared envdir for the virtualenv.
> 
> mypy, pep8, fast8, genconfig, genpolicy, cover, debug and bandit.
> 
>>  In the nova repo, if you `tox -e bandit` or `tox -e pep8` it's going
>>  to install coverage, psycopg2, PyMySQL, requests,
>>  python-barbicanclient, python-ironicclient, and a whole host of
>>  other stuff, including the entire transitive dependency set for
>>  everything in there, rather than just the one tool it needs to run.
> 
> Yup that's pointless.
> 
>>  I can't even run the pep8 testenv locally because to do that I
>>  apparently need a Python package named zVMCloudConnector which wants
>>  root access to create files like
>>  /lib/systemd/system/sdkserver.service and
>>  /etc/sudoers.d/sudoers-zvmsdk and /var/lib/zvmsdk/* and
>>  /etc/zvmsdk/* in my system. WHAT?!? Do nova's developers actually
>>  ever run any of this themselves?
> 
> ...
> 
> Which version of that package is the pep8 env pulling in for you?
> 
> I don't see any such issues with zVMCloudConnector==1.4.1 locally on
> Fedora 33, tox 3.19.0, pip 20.2.2 etc.
> 
> Would you mind writing up a launchpad bug for this?
> 
>>  Okay, so that one's actually in requirements.txt (might be a good
>>  candidate for a separate extras in the setup.cfg instead), but
>>  seriously, it's trying to install 182 packages (present count on
>>  master) just to do a "quick" style check, and the resulting .tox
>>  created from that is 319MB in size. How is that in any way sane? If
>>  I tweak the testenv:pep8 definition in tox.ini to set
>>  deps=flake8,hacking,mypy and and usedevelop=False, and set
>>  skipsdist=True in the general tox section, it installs a total of 9
>>  packages for a 36MB .tox directory. It's an extreme example, sure,
>>  but remember this is also happening in CI for each patch uploaded,
>>  and this setup cost is incurred every time in that context.
> 
> EWww yeah this is awful.
> 
>>  This is already solved in a few places in the nova repo, in
>>  different ways. One is the docs testenv, which installs
>>  doc/requirements.txt (currently 10 mostly Sphinx-related entries)
>>  instead of combining all that into test-requirements.txt too.
>>  Another is the osprofiler extra in setup.cfg allowing you to `pip
>>  install nova[osprofiler]` to get that specific dependency. Yet still
>>  another is the bindep testenv, which explicitly declares deps=bindep
>>  and so installs absolutely nothing else (save bindep's own
>>  dependencies)... or, well, it would except skipsdist got set to
>>  False by https://review.openstack.org/622972 making that testenv
>>  effectively pointless because now `tox -e bindep` has to install
>>  nova before it can tell you what packages you're missing to be able
>>  to install nova. *sigh*
>> 
>>  So anyway, there's a lot of opportunity for improvement, and that's
>>  just in nova, I'm sure there are similar situations throughout many
>>  of our projects. Using a test-requirements.txt file as a dumping
>>  ground for every last package any tox testenv could want may be
>>  convenient for tracking things, but it's far from convenient to
>>  actually use. The main thing we risk losing is that the
>>  requirements-check job currently reports whether entries in
>>  test-requirements.txt are compatible with the global
>>  upper-constraints.txt in openstack/requirements, so extending that
>>  to check dependencies declared in tox.ini or in package extras or
>>  additional external requirements lists would be needed if we wanted
>>  to preserve that capability.
> 
> Gibi, should we track all of this in a few launchpad bugs for Nova?

Sure, we can open couple of low prio low-hanging-fruit bugs for these.

Cheers,
gibi

> 
> Cheers,
> 
> --
> Lee Yarwood                 A5D1 9385 88CB 7E5F BE64  6618 BCA6 6E33 
> F672 2D76