Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parallel invocation of tox environments #439 #1102

Merged
merged 27 commits into from
Jan 11, 2019

Conversation

gaborbernat
Copy link
Member

@gaborbernat gaborbernat commented Dec 16, 2018

This solution is subprocess based. We're already heavily using subprocesses, so I consider this implementation safe.

TBD:

  • for non-live parallel show some progress notification,
  • correctly forward all status-es and their messages from parallel invocations,
  • better tests.

Resolves #439.

@gaborbernat gaborbernat force-pushed the parallel branch 3 times, most recently from f059f7c to bc134c8 Compare December 16, 2018 15:28
adamantike added a commit to adamantike/pytest-django that referenced this pull request Dec 17, 2018
When tests are executed using Tox in parallel, modify the test database
names, to avoid name collisions between processes.

This change renames the existing
`django_db_modify_db_settings_xdist_suffix` fixture, to a generic
`django_db_modify_db_settings_parallel_suffix` one, in case more
scenarios/tools have to be considered in the future.

It also handles projects where both `pytest-xdist` and parallel `tox`
are being using, generating database names like
`test_default_py37-django21_gw0`.

**Notes**:
* WIP at the moment, until `tox` decides how to expose that it's running
in parallel. (Current WIP pull request on their side [0])
* This could break an existing contract for the
`django_db_modify_db_settings_xdist_suffix`, depending on projects using
it directly. Probably, we want to avoid a major release just for this
fix, so if that fixture is not intended to be used, maybe we're fine?

Resolves pytest-dev#678.

[0] tox-dev/tox#1102
@michaelmanganiello-eb
Copy link

michaelmanganiello-eb commented Dec 17, 2018

Hi @gaborbernat !

I'm currently working on adding support to pytest-django to rename Django databases and avoid name collisions (pytest-dev/pytest-django#680).

For this scenario at least, it would be great to have a public-facing way to check if Tox is running in parallel or not. The _PARALLEL_TOX environment variable seems to achieve this, but I think it shouldn't be part of a private API that could be easily changed.

@gaborbernat
Copy link
Member Author

Sure we can add some environment variable, but I believe shouldn't two parallel pytest invocations work always, even if not made through tox, aka parallel shell invocations?

@adamantike
Copy link

I'm not sure we would like to rename DBs by default, even in projects that won't run in parallel or have other requirements/limitations for test DB generation. For two parallel pytest invocations to always work, we should append the associated PID or any random number to DB names, something that could be not very straightforward.

However, to support scenarios where tox --parallel is executed, and also scenarios where the orchestation is implemented manually (e.g. for loop running tox -vve {tox_env} asynchronously), my idea is to only use the value of the (current) _PARALLEL_TOX variable just as a boolean, and, when that's set, change the DB name based on the content of the TOX_ENV_NAME environment variable.

That way, even if developers aren't using tox --parallel, they could change their own execution method, to use _PARALLEL_TOX=1 tox ... instead.

@gaborbernat
Copy link
Member Author

I'm not sure I follow why always adding the PID is a bad thing🤔

@obestwalter
Copy link
Member

... it would be great to have a public-facing way to check if Tox is running in parallel or not. The _PARALLEL_TOX environment variable seems to achieve this, but I think it shouldn't be part of a private API that could be easily changed.

I also think that this could be useful as a public facing env var.

About the name: I think it would be good to always have TOX_ as the prefix for all tox related ones.

@obestwalter
Copy link
Member

Utilising subprocess is a great idea I think, as it just seems glaringly obvious rather than using gevent or something simlilarly fragile.

This will obviously not work for setups with hundreds of environments, but I honestly hope that those are extremely rare anyway and projects with setups like that do not rely on something like detox or soon to be tox --parallel.

@gaborbernat
Copy link
Member Author

Thinking this through I now actually want to implement some throttling similar to how pytest-xdist has. --parallel-count flag would be a choice of {all,auto,count}. all would be the default but users could switch to auto (CPU count), or explicitly specify a number if they wish.

src/tox/config.py Outdated Show resolved Hide resolved
src/tox/config.py Outdated Show resolved Hide resolved
src/tox/session.py Outdated Show resolved Hide resolved
src/tox/session.py Outdated Show resolved Hide resolved
src/tox/session.py Outdated Show resolved Hide resolved
@obestwalter
Copy link
Member

Really nice and simple way to "merge" detox into tox, I think.

How about also creating an entry point detox that internally calls tox --parallel? This would also be another reason to not need an extra flag for log levels but using verbosity levels, like mentioned in one of the code comments.

@obestwalter
Copy link
Member

I tried running tox parallel now locally with tox itself and have the following output:

tox --parallel

.package create: /home/ob/tmp/tox/.tox/.package
.package installdeps: setuptools >= 40.0.4, setuptools_scm >= 2.0.0, <4, wheel >= 0.29.0
___________ summary ____________
  py27: commands succeeded
  py34: commands succeeded
  py35: commands succeeded
  py36: commands succeeded
  py37: commands succeeded
ERROR:   pypy: 1
  pypy3: commands succeeded
ERROR:   coverage: 1
  fix_lint: commands succeeded
  docs: commands succeeded
  package_description: commands succeeded

I can't find anything in the logs about the source of the error either :( this might well be a problem with tox itself. I have to admit I hardly ever look into the logs, so I don't even know exactly what I should expect.

Would it be feasible to always capture the output and print it out for all failed envs?

@gaborbernat
Copy link
Member Author

This should already happen will need to check why not did in your case.

@obestwalter
Copy link
Member

This should already happen will need to check why not did in your case.

Can you post an example output what it looks like when it works?

@gaborbernat
Copy link
Member Author

@obestwalter turns out I did not implement reporting of failed environments, now added it 👍

@gaborbernat gaborbernat added feature:new something does not exist yet, but should area:configuration area:commands-execution area:reporting area:documentation needs:review somebody who thinks they know what they are doing should have a look at this level:hard rought estimate that this might be quite hard to implement labels Jan 3, 2019
@gaborbernat gaborbernat force-pushed the parallel branch 3 times, most recently from e1a543e to 4660bef Compare January 3, 2019 12:44
@gaborbernat
Copy link
Member Author

@obestwalter
Copy link
Member

obestwalter commented Jan 10, 2019

Thanks for your patience. I have time tonight and tomorrow to prepare a proper funeral and come up with a plan how to make sure that this will be as painless as possible for everybody involved :)

@obestwalter
Copy link
Member

obestwalter commented Jan 11, 2019

Thinking about it again: adding detox as command for this new functionality was a BadIdea(TM), so this should not go forward. I'll make a new detox release with an uppper bound for tox and add a warning and that should be it. that new parallel functionality can have a fresh start then without pretending to be something else.

@gaborbernat
Copy link
Member Author

going meta here:

 env DIFF_AGAINST=upstream/master PIP_INDEX_URL=http://localhost:3141/bernat/dev ../tox/.tox/dev/bin/tox -e fix_lint,py27,py37,coverage -p all                           1239ms  Fri 11 Jan 12:56:22 2019
.package create: /Users/bernat/git/tox/.tox/.package
.package installdeps: setuptools >= 40.0.4, setuptools_scm >= 2.0.0, <4, wheel >= 0.29.0
✔ OK fix_lint in 12.201 seconds
✔ OK py37 in 3 minutes, 44.444 seconds
✔ OK py27 in 3 minutes, 45.882 seconds
✔ OK coverage in 12.08 seconds
coverage recreate: /Users/bernat/git/tox/.tox/coverage
coverage installdeps: coverage >= 4.4.1, < 5, diff_cover
coverage installed: coverage==4.5.2,diff-cover==1.0.6,inflect==2.1.0,Jinja2==2.10,jinja2-pluralize==0.3.0,MarkupSafe==1.1.0,Pygments==2.3.1,six==1.12.0
coverage run-test-pre: PYTHONHASHSEED='4156706002'
coverage runtests: commands[0] | coverage erase
coverage runtests: commands[1] | coverage combine
coverage runtests: commands[2] | coverage report -m
Name                                  Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------------------------------------
src/tox/_pytestplugin.py                277     10     70      7    95%   27, 29, 69, 88, 135, 139, 249-252, 26->27, 28->29, 68->69, 87->88, 138->139, 169->182, 396->393
src/tox/_quickstart.py                  122     18     57      7    85%   76, 83, 91, 99, 117-128, 132-134, 275-277, 75->76, 82->83, 90->91, 98->99, 116->117, 176->186, 178->177
src/tox/cli.py                            7      7      0      0     0%   1-11
src/tox/config/__init__.py              897      3    341     15    99%   971, 1132, 1245, 191->199, 341->342, 570->582, 853->855, 883->884, 967->971, 1024->1025, 1084->exit, 1131->1132, 1244->1245, 1475->1476, 1498->1508, 1540->1541, 1554->1555, 1571->1590
src/tox/config/parallel.py               35      4      6      0    90%   14-15, 26-27
src/tox/interpreters.py                 120     35     32      3    63%   55-57, 74, 85, 87, 90-91, 141-179, 84->85, 86->87, 130->141
src/tox/package/__init__.py              50      4     10      2    90%   21-22, 55-56, 20->21, 51->59
src/tox/package/builder/isolated.py      64      3     25      2    94%   36-40, 24->27, 35->36
src/tox/package/view.py                  26      3     10      2    86%   34-35, 37, 30->36, 36->37
src/tox/result.py                        47      1      4      1    96%   14, 13->14
src/tox/session.py                      632     15    230     12    97%   59, 205, 225, 235-236, 268-270, 345, 511-513, 518, 552-553, 665, 58->59, 203->205, 224->225, 249->251, 256->258, 267->268, 332->exit, 510->511, 517->518, 627->629, 642->645, 664->665
src/tox/util/graph.py                    48      0     40      1    99%   65->exit
src/tox/util/spinner.py                 115      5     40      8    92%   16-18, 117, 125, 129, 15->16, 36->exit, 74->78, 113->114, 119->exit, 124->125, 130->exit, 149->152
src/tox/venv.py                         382     10    152     11    95%   95, 149, 186, 469-473, 498-499, 523, 94->95, 143->146, 146->149, 185->186, 189->188, 190->189, 266->exit, 396->397, 468->469, 495->459, 522->523
---------------------------------------------------------------------------------
TOTAL                                  2952    118   1049     71    95%

9 files skipped due to complete coverage.
coverage runtests: commands[3] | coverage xml -o /Users/bernat/git/tox/.tox/coverage.xml
coverage runtests: commands[4] | coverage html -d /Users/bernat/git/tox/.tox/htmlcov
coverage runtests: commands[5] | diff-cover --compare-branch upstream/master /Users/bernat/git/tox/.tox/coverage.xml
-------------
Diff Coverage
Diff: upstream/master...HEAD, staged and unstaged changes
-------------
src/tox/config/__init__.py (100%)
src/tox/config/parallel.py (88.6%): Missing lines 14-15,26-27
src/tox/session.py (99.2%): Missing lines 665
src/tox/util/__init__.py (100%)
src/tox/util/graph.py (100%)
src/tox/util/spinner.py (95.7%): Missing lines 16,18,117,125,129
src/tox/venv.py (100%)
-------------
Total:   337 lines
Missing: 10 lines
Coverage: 97%
-------------
_________________

@gaborbernat gaborbernat merged commit b6de343 into tox-dev:master Jan 11, 2019
@gaborbernat gaborbernat deleted the parallel branch January 11, 2019 13:54
@gaborbernat
Copy link
Member Author

Releasing now via #1134.

thanks all

@dunossauro
Copy link

I love it ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:commands-execution area:configuration area:documentation area:reporting feature:new something does not exist yet, but should level:hard rought estimate that this might be quite hard to implement needs:review somebody who thinks they know what they are doing should have a look at this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow parallel execution of envs (merge or replace detox?)
7 participants