Description

chriskuehl

opened on Mar 25, 2017

Issue body actions

I have this one hook that runs puppet parser validate which is extremely slow. Running it on just 185 files takes 105 seconds which makes my tests (which call pre-commit run --all-files, among other things) annoying enough to run that most people don't run them locally.

What if pre-commit's xargs implementation could do things in parallel? Here's a sketch of a patch that does that (only works on Python 3): https://i.fluffy.cc/t43V5vqd3VH9lTQfl8djnfZWBV2zDDTZ.html

This takes my test time from 105 seconds to 15 seconds.

Some thoughts:

• If any hooks write to files besides the ones they are linting, this could break. This is a problem, though pre-commit is heavily designed around operations on individual files, so the vast majority of hooks should be okay. We could offer an opt-in or opt-out at the individual hook level?
• Parallelizing different hooks (running puppet-lint at the same time as puppet-validate) would be neat but I think is way more problematic (and introduces crazy locking problems since you can't have two hooks running on the same file at once).
• Because pre-commit captures output and displays it at the end, I don't think we have any of the usual problems of interleaved/confusing output. The printing happens in the main thread when collecting statuses and shouldn't have races.
• concurrent.futures works great here, but is Python 3 only. I'm not sure how hard this is to do in a compatible way.

@asottile what do you think? Is this too complicated to be worth pursuing?

Reacted by Jethro Yu, Krishna Vudata, Daniel Roschka, Pierre Snell, elijahchancey-arena, Samuel Colvin, Will Da Silva, Éloi Rivard and Igor Jerosimić

Activity

1. chriskuehl

   added

   feature

   question

   on Mar 25, 2017
2. asottile commented on Mar 25, 2017

   

   asottile

   on Mar 25, 2017

   Member

   More actions

   I've put a little bit of thought into this in the past. The problem is there's no guarantee as to whether a hook writes a file or not. Hooks could document whether they perform writes and those could be done serially. That probably complicates everything? The actual parallel execution part probably isn't the difficult part but making sure that read/write hooks are excluded from parallelism.

   So maybe an opt-in readonly: true or something? (which would enable parallel execution?)
3. asottile commented on Mar 25, 2017

   

   asottile

   on Mar 25, 2017

   Member

   More actions

   Maybe single_file: true would be more accurate, since read/write hooks could internally parallelize and still be correct.
4. asottile commented on Mar 25, 2017

   

   asottile

   on Mar 25, 2017

   Member

   More actions

   Or allow_parallel: true, and just document the caveats
5. chriskuehl commented on Mar 25, 2017

   

   chriskuehl

   on Mar 25, 2017

   MemberAuthor

   More actions

   Does it actually matter if a hook writes files, as long as it only writes to the files it's been passed on the command line? E.g. I don't think it's an issue to run autopep8 in parallel. The only issue I see is if you had a hook that was writing to unrelated files that might be part of another bundle being executed at the same time, but I can't think of any off the top of my head.

   It's probably still worth to have an option though since some hooks already do parallel execution (e.g. flake8) and it would probably be slower to parallelize those.
6. asottile commented on Mar 25, 2017

   

   asottile

   on Mar 25, 2017

   Member

   More actions

   Yeah it's not necessarily a problem if a hook writes files. The actual problem cases I can think of are:

   • A hook reads files not listed on CLI and also writes files
   • A hook writes files not listed on CLI
7. asottile commented on Jul 28, 2017

   

   asottile

   on Jul 28, 2017

   Member

   More actions

   @chriskuehl and I discussed this again today -- here's my thoughts / summary of that:

   • In parallel mode, the xargs batch size would be decreased (can probably come up with a clever way to optimize this, maybe optimize for each process to cover two chunks or something?)
   • enable parallel mode by default, all hooks (except for one closed source hook) that I can think of would function completely fine in parallel
   • opt out of parallel mode on a per-hook basis by require_serial: true (default false)
8. asottile

   closed this as completedin #851on Nov 2, 2018
9. asottile commented on Dec 20, 2018

   

   asottile

   on Dec 20, 2018

   Member

   More actions

   This has been released as part of v1.13.0 🎉

   Reacted by Dan Strokirk
10. avegancafe commented on Jan 24, 2025

    

    avegancafe

    on Jan 24, 2025 · Hidden as off-topic

    show commentMore actions
11. pre-commit

    locked as off topic and limited conversation to collaborators on Jan 24, 2025

Issue actions