New issue
Advanced search Search tips

Issue 819774 link

Starred by 1 user
Status: Assigned
Owner: ----
Cc:
ehmaldonado@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature



Sign in to add a comment

FR: Parallelize tasks from presubmit files rather than run serially

Project Member Reported by dannycochran@google.com, Mar 7 2018 
My team has a mono repo using Git on Borg. Within that mono-repo we have a packages folder, with 6 different packages.

Each package has its own PRESUBMIT.py file, and they are all the same:

```python
import sys
 
def CheckChangeOnUpload(input_api, output_api):
  # Based on https://src.chromium.org/viewvc/chrome/trunk/src/PRESUBMIT.py?view=markup#l502
  original_sys_path = sys.path
  try:
    sys.path = sys.path + [input_api.os_path.join(input_api.PresubmitLocalPath(), '..', '..', 'tools')]
    import presubmit_utils
  finally:
    # Restore sys.path to what it was before.
    sys.path = original_sys_path
 
  cmds = []
  cmds += presubmit_utils.GetYarnCommand(input_api, output_api, 'presubmit')
 
  return input_api.RunTests(cmds)
  pass
```

When making changes to multiple packages and then running git cl upload, each PRESUBMIT.py file from the affected package will be run, but they are run serially rather than in parallel, which can take some time.

Here's the code where they're run serially:

https://chromium.googlesource.com/chromium/tools/depot_tools/+/c29602466dc288e5e4b6d683f3cece88a665a594/presubmit_support.py#1413

It would be great to either have an option, such as: git cl upload --parallel-presubmit, or make it the default behavior.

Comment 1 by aga...@chromium.org, Mar 7 2018

Components: Infra>SDK
Labels: -Performance -Type-Bug Pri-2 Type-Feature
Status: Available (was: Unconfirmed)
This could definitely be a big win, but implementation is likely complicated by the existing within-file multiprocessing that presubmit_support already does.

Comment 2 by aga...@chromium.org, Mar 19 2018

Cc: ehmaldonado@chromium.org
CCing Edward in case he's interesting in taking this on after we talked about it today.
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 3 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/c7d0b34084e589a895875beb9cd7ca0ba961fd4f

commit c7d0b34084e589a895875beb9cd7ca0ba961fd4f
Author: Edward Lesmes <ehmaldonado@chromium.org>
Date: Tue Apr 03 00:14:06 2018

presubmit support: Run all tests in parallel.

Currently all tests in a PRESUBMIT.py file are run in parallel, but not
all tests across PRESUBMIT.py files.

This introduces a pool common to all files so we can run all of them.

Bug: 819774
Change-Id: Ic129af1bc9e6da568fa9fa71827193c6d8ab9af1
Reviewed-on: https://chromium-review.googlesource.com/973691
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

[modify] https://crrev.com/c7d0b34084e589a895875beb9cd7ca0ba961fd4f/presubmit_support.py
[modify] https://crrev.com/c7d0b34084e589a895875beb9cd7ca0ba961fd4f/tests/presubmit_unittest.py
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 3 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4124e572393b02841fa871c52e23008c694c45ac

commit 4124e572393b02841fa871c52e23008c694c45ac
Author: depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Tue Apr 03 02:02:42 2018

Roll src/third_party/depot_tools/ 81b26429c..c7d0b3408 (3 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/81b26429c8f6..c7d0b34084e5

$ git log 81b26429c..c7d0b3408 --date=short --no-merges --format='%ad %ae %s'
2018-03-28 ehmaldonado presubmit support: Run all tests in parallel.
2018-04-02 agable Kick recipe roller
2018-04-02 agable presubmit: don't die when skipping unknown checks

Created with:
  roll-dep src/third_party/depot_tools
BUG=chromium:819774,chromium:828154,chromium:770408,chromium:828154


The AutoRoll server is located here: https://depot-tools-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=agable@chromium.org

Change-Id: I25169163511e6438ed68514290681cc3213ab7d1
Reviewed-on: https://chromium-review.googlesource.com/991352
Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#547600}
[modify] https://crrev.com/4124e572393b02841fa871c52e23008c694c45ac/DEPS
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 3 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/1ad681eca27a4b40dd61ada6da0c8c1bb0c634cc

commit 1ad681eca27a4b40dd61ada6da0c8c1bb0c634cc
Author: Edward Lesmes <ehmaldonado@chromium.org>
Date: Tue Apr 03 21:16:21 2018

Revert "presubmit support: Run all tests in parallel."

This reverts commit c7d0b34084e589a895875beb9cd7ca0ba961fd4f.

Reason for revert:

See https://logs.chromium.org/v/?s=infra%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8950238735438551408%2F%2B%2Fsteps%2Fpresubmit%2F0%2Fstdout
Two go bootstraps executing concurrently and breaking each other. govet and golint presubmit checks both try to bootstrap go at the same time. 


Original change's description:
> presubmit support: Run all tests in parallel.
> 
> Currently all tests in a PRESUBMIT.py file are run in parallel, but not
> all tests across PRESUBMIT.py files.
> 
> This introduces a pool common to all files so we can run all of them.
> 
> Bug: 819774
> Change-Id: Ic129af1bc9e6da568fa9fa71827193c6d8ab9af1
> Reviewed-on: https://chromium-review.googlesource.com/973691
> Reviewed-by: Aaron Gable <agable@chromium.org>
> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

TBR=agable@chromium.org,ehmaldonado@chromium.org

Change-Id: Ia5b5ae5af8d6cf9bd72388f58ff0f032a4367e10
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 819774
Reviewed-on: https://chromium-review.googlesource.com/994032
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>

[modify] https://crrev.com/1ad681eca27a4b40dd61ada6da0c8c1bb0c634cc/presubmit_support.py
[modify] https://crrev.com/1ad681eca27a4b40dd61ada6da0c8c1bb0c634cc/tests/presubmit_unittest.py

Comment 6 by dannycochran@google.com, Apr 3 2018 

@ehmaldonado -- it looks like running in parallel didn't work for the repo you mentioned in comment #5, so you had to revert?

It might make sense to put the parallel functionality behind a feature flag, as there are likely a lot of projects that inadvertently depend on serialized task execution.

Comment 7 by ehmaldonado@chromium.org, Apr 3 2018 

Yes, when I reland, I'll reland it behind a flag.

Comment 8 by dannycochran@google.com, Apr 3 2018 

Awesome, thanks for jumping on this. It's going to speed up my team's presubmits significantly.
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 4 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5b8f78c80cfc0da771e9eaf79569376fb7222742

commit 5b8f78c80cfc0da771e9eaf79569376fb7222742
Author: depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Wed Apr 04 03:19:10 2018

Roll src/third_party/depot_tools/ adcf030f6..a1df57cdc (2 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/adcf030f6910..a1df57cdc657

$ git log adcf030f6..a1df57cdc --date=short --no-merges --format='%ad %ae %s'
2018-04-03 ehmaldonado roll-dep: Support dictionaries in deps revisions.
2018-04-03 ehmaldonado Revert "presubmit support: Run all tests in parallel."

Created with:
  roll-dep src/third_party/depot_tools
BUG= chromium:760633 ,chromium:819774


The AutoRoll server is located here: https://depot-tools-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=agable@chromium.org

Change-Id: I965e7d61676852711ede33839ace1676344891d2
Reviewed-on: https://chromium-review.googlesource.com/994130
Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#547961}
[modify] https://crrev.com/5b8f78c80cfc0da771e9eaf79569376fb7222742/DEPS
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 12 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/8e282797599178db83308aa1ee424397aeb9bb10

commit 8e282797599178db83308aa1ee424397aeb9bb10
Author: Edward Lesmes <ehmaldonado@chromium.org>
Date: Thu Apr 12 22:44:29 2018

Reland "presubmit support: Run all tests in parallel."

Currently all tests in a PRESUBMIT.py file are run in parallel, but not
all tests across PRESUBMIT.py files.

This introduces a flag that will allow presubmit to run all tests across
PRESUBMIT files in parallel.

Bug: 819774
Change-Id: Idd3046cb3c85e9c28932a9789ba7b207a01d9f99
Reviewed-on: https://chromium-review.googlesource.com/994241
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

[modify] https://crrev.com/8e282797599178db83308aa1ee424397aeb9bb10/presubmit_support.py
[modify] https://crrev.com/8e282797599178db83308aa1ee424397aeb9bb10/git_cl.py
[modify] https://crrev.com/8e282797599178db83308aa1ee424397aeb9bb10/tests/presubmit_unittest.py
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 13 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da08af2142a81207c9d54921d52c9db374187431

commit da08af2142a81207c9d54921d52c9db374187431
Author: depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Apr 13 06:10:21 2018

Roll src/third_party/depot_tools/ b1c21a5af..53a115e67 (5 commits; 1 trivial rolls)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/b1c21a5af2ec..53a115e67e50

$ git log b1c21a5af..53a115e67 --date=short --no-merges --format='%ad %ae %s'
2018-04-12 asanka Add a '--inject-current' option to 'git new-branch'
2018-04-12 petermayo Remove petermayo from OWNERS
2018-04-03 ehmaldonado Reland "presubmit support: Run all tests in parallel."
2018-04-12 ehmaldonado gclient: Don't parse DEPS files that we won't recurse into.

Created with:
  roll-dep src/third_party/depot_tools
BUG=chromium:819774,chromium:830306


The AutoRoll server is located here: https://depot-tools-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=agable@chromium.org

Change-Id: I9f5c9eed3ed2ee72ff566d0ff08f6e751e4d89d8
Reviewed-on: https://chromium-review.googlesource.com/1011301
Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#550528}
[modify] https://crrev.com/da08af2142a81207c9d54921d52c9db374187431/DEPS
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da08af2142a81207c9d54921d52c9db374187431

commit da08af2142a81207c9d54921d52c9db374187431
Author: depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Apr 13 06:10:21 2018

Roll src/third_party/depot_tools/ b1c21a5af..53a115e67 (5 commits; 1 trivial rolls)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/b1c21a5af2ec..53a115e67e50

$ git log b1c21a5af..53a115e67 --date=short --no-merges --format='%ad %ae %s'
2018-04-12 asanka Add a '--inject-current' option to 'git new-branch'
2018-04-12 petermayo Remove petermayo from OWNERS
2018-04-03 ehmaldonado Reland "presubmit support: Run all tests in parallel."
2018-04-12 ehmaldonado gclient: Don't parse DEPS files that we won't recurse into.

Created with:
  roll-dep src/third_party/depot_tools
BUG=chromium:819774,chromium:830306


The AutoRoll server is located here: https://depot-tools-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=agable@chromium.org

Change-Id: I9f5c9eed3ed2ee72ff566d0ff08f6e751e4d89d8
Reviewed-on: https://chromium-review.googlesource.com/1011301
Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#550528}
[modify] https://crrev.com/da08af2142a81207c9d54921d52c9db374187431/DEPS

Comment 13 by ehmaldonado@chromium.org, Apr 20 2018

Status: Assigned (was: Available)
dannycochran: Have you tried using the --parallel flag to test this?

Comment 14 by esprehn@chromium.org, Apr 20 2018 

Unfortunately dannycochran@ is no longer at Google. Is this enabled by default?

I'm currently having the problem where when I Ctrl+C while presubmit is running that it no longer aborts and goes back to the bash prompt. You need to do it a couple times and then hit enter.

Comment 15 by mullens@google.com, Jun 20 2018 

--parallel doesn't seem to work for me. I'm on the latest depot_tools and the --parallel flag exists, but it runs in series:
"""
mullens-macbookpro1:packages mullens$ git cl presubmit --upload --verbose --parallel
Running presubmit upload checks ...
Running /Users/mullens/grasshopper/PRESUBMIT.py
Running /Users/mullens/grasshopper/packages/admin/PRESUBMIT.py
Running /Users/mullens/grasshopper/packages/app/PRESUBMIT.py
Running /Users/mullens/grasshopper/packages/common/PRESUBMIT.py

** Presubmit Messages **
presubmit (28.72s)

presubmit (21.59s)

presubmit (7.39s)

Presubmit checks took 57.7s to calculate.
"""

Comment 16 by ehmaldonado@chromium.org, Jun 20 2018 

Is there a way for me to check this locally?
Project Member

Comment 17 by bugdroid1@chromium.org, Jul 20 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/7e3c67f2c3322f146e8e81fabb8d85dde1d04a3c

commit 7e3c67f2c3322f146e8e81fabb8d85dde1d04a3c
Author: Edward Lemur <ehmaldonado@chromium.org>
Date: Fri Jul 20 20:52:49 2018

presubmit_support: Fix parallel execution of presubmit tests.

The flag value was not being propagated correctly.

Bug: 819774
Change-Id: I42519e1c84704b9a4a613005d3441b7ee12ea427
Reviewed-on: https://chromium-review.googlesource.com/1142533
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

[modify] https://crrev.com/7e3c67f2c3322f146e8e81fabb8d85dde1d04a3c/presubmit_support.py

Comment 18 by ehmaldonado@chromium.org, Jul 20 

Please check again :)
Project Member

Comment 19 by bugdroid1@chromium.org, Jul 20 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18c48e9661f4bc46fa027905e81a9b3365e17844

commit 18c48e9661f4bc46fa027905e81a9b3365e17844
Author: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Jul 20 23:56:30 2018

Roll src/third_party/depot_tools 60b9b6fb9147..7e3c67f2c332 (1 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/60b9b6fb9147..7e3c67f2c332


git log 60b9b6fb9147..7e3c67f2c332 --date=short --no-merges --format='%ad %ae %s'
2018-07-20 ehmaldonado@chromium.org presubmit_support: Fix parallel execution of presubmit tests.


Created with:
  gclient setdep -r src/third_party/depot_tools@7e3c67f2c332

The AutoRoll server is located here: https://depot-tools-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG=chromium:819774
TBR=agable@chromium.org

Change-Id: Idefb9827c26bf03d9a0705392db13f99b3daf9aa
Reviewed-on: https://chromium-review.googlesource.com/1145800
Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#577042}
[modify] https://crrev.com/18c48e9661f4bc46fa027905e81a9b3365e17844/DEPS
►

Sign in to add a comment