GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Implement concurrent execution of individual hooks by chriskuehl · Pull Request #851 · pre-commit/pre-commit

Conversation

chriskuehl commented Oct 20, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Fixes #510

This implements something like the interface described in the ticket. Individual hooks have partitions run in parallel. The current logic is just a sketch (it uses the number of CPUs as the target concurrency, and does not adjust partitioning logic, so partitions are usually very large and the benefit is minimal).

Mainly looking for feedback on the current interface changes. I think it makes sense for xargs() to remain a "hook-ignorant" function (i.e. it's basically a generic Python implementation of xargs, and does not know anything about pre-commit or hook configs), so the logic for determining the target concurrency based on a hook's config lives in language helpers instead. This leads to a bit more boilerplate, but I could imagine different languages wanting to optimize this differently anyhow. (For example, I added TODOs about whether it really makes sense to parallelize the grep languages.)

Any thoughts?

TODO

Smarter partitioning to take advantage of concurrency
Better algorithm for calculating target concurrency?
Tests for require_serial / the above

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dunedan and alexjurkiewicz reacted with thumbs up emoji

alexjurkiewicz, jvperrin, and dkess reacted with heart emoji

All reactions

👍 2 reactions
❤️ 3 reactions

chriskuehl requested a review from asottile

October 20, 2018 20:13

chriskuehl commented

Oct 20, 2018

View reviewed changes

Comment thread

pre_commit/languages/helpers.py Outdated

		return 1
		else:
		# TODO: something smart!
		return multiprocessing.cpu_count()

chriskuehl Oct 20, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know what to do here. Maybe the number of CPUs is actually a decent heuristic? Or the number minus one? Or divided by two???

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

asottile Oct 20, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably need to do something like this: https://github.com/pytest-dev/pytest-xdist/pull/317/files

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

pre_commit/xargs.py Outdated

		)
		# TODO: teach partition to intelligently target our desired concurrency
		# while still respecting max_length.
		partitions = partition(cmd, varargs, **kwargs)

chriskuehl Oct 20, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the eventual change to partition would have some relatively simple logic where it tries to fill up sets of N partitions (to take advantage of the N concurrency) while respecting the max_length, rather than trying to minimize the number of partitions like it does now. Probably with some simple logic to avoid silly things like 4 partitions of 1 item each which are probably non-optimal due to per-invocation overhead (we can probably just hard-code something reasonable here?).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tests/xargs_test.py Outdated

		assert ret == 0
		pids = stdout.splitlines()
		assert len(pids) == 5
		assert elapsed < 1

chriskuehl Oct 20, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of a silly test, but asserting stuff is running in parallel is hard. idk, what do you think?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

asottile reviewed

Oct 20, 2018

View reviewed changes

Comment thread

pre_commit/languages/helpers.py Outdated

		return 1
		else:
		# TODO: something smart!
		return multiprocessing.cpu_count()

asottile Oct 20, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably need to do something like this: https://github.com/pytest-dev/pytest-xdist/pull/317/files

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

pre_commit/languages/pcre.py Outdated

		file_args,
		negate=True,
		# TODO: does it actually make sense to parallelize this hook, given
		# it's probably I/O bound?

asottile Oct 20, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, probably make this always synchronous is fine -- pcre is going away eventually anyway (for pygrep -- though I assume below I'm about to look at pygrep anyway!)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

chriskuehl force-pushed the concurrent-execution branch 3 times, most recently from 9bbf046 to 3c2e3f4 Compare

October 21, 2018 02:36

chriskuehl commented

Oct 21, 2018

View reviewed changes

Comment thread

pre_commit/xargs.py Outdated



		@contextlib.contextmanager
		def _threadpool(size):

chriskuehl Oct 21, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asottile let me know if you have a preference on doing this vs installing a concurrent.futures backport on python2

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

asottile Oct 21, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

up to you, concurrent.futures is slightly more future-proof if the code is simpler (if it isn't then definitely don't bother)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

chriskuehl changed the title ~~[WIP] Implement concurrent execution of individual hooks~~ Implement concurrent execution of individual hooks

Oct 21, 2018

chriskuehl commented Oct 21, 2018

Copy link

Copy Markdown

Member Author

Heh, on AppVeyor:

================================== FAILURES ===================================
___________________________ test_xargs_concurrency ____________________________
    def test_xargs_concurrency():
        bash_cmd = ('bash', '-c')
        print_pid = ('sleep 0.5 && echo $$',)
    
        start = time.time()
        ret, stdout, _ = xargs.xargs(
            bash_cmd, print_pid * 5,
            target_concurrency=5,
            _max_length=len(' '.join(bash_cmd + print_pid)),
        )
        elapsed = time.time() - start
        assert ret == 0
        pids = stdout.splitlines()
        assert len(pids) == 5
>       assert elapsed < 1
E       assert 1.0390000343322754 < 1
tests\xargs_test.py:124: AssertionError
== 1 failed, 491 passed, 6 skipped, 9 xfailed, 10 xpassed in 529.27 seconds ===

It's been passing most of the time on AppVeyor, probably flaky :\

Guess I should increase the elapsed duration... or just delete the test.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asottile commented Oct 21, 2018

Copy link

Copy Markdown

Member

Guess I should increase the elapsed duration... or just delete the test.

🔪 😆

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asottile commented Oct 21, 2018

Copy link

Copy Markdown

Member

hmmm, one other thing I thought about is this is probably going to make the test suite hard to debug with pdb 🤔

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

chriskuehl force-pushed the concurrent-execution branch from deca085 to a74e0d7 Compare

October 22, 2018 16:54

chriskuehl commented Oct 22, 2018

Copy link

Copy Markdown

Member Author

@asottile switched to a concurrent.futures backport and added an env var to force it to run in serial during tests. Let me know what you think!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

chriskuehl force-pushed the concurrent-execution branch from a74e0d7 to 7200945 Compare

October 23, 2018 17:29

chriskuehl added 2 commits

October 24, 2018 16:16


          Implement concurrent execution of individual hooks

ba5e27e


          Handle CPU detection errors and running on Travis

ec0ed8a

chriskuehl force-pushed the concurrent-execution branch from 7200945 to 9cd806c Compare

October 24, 2018 23:17

chriskuehl and others added 4 commits

October 24, 2018 16:33


          Attempt to partition files to use all possible cores

b6926e8


          Allow more time on the concurrency test

231f601

Spawning processes is apparently really slow on Windows, and the test is
occasionally taking slightly more than a second on AppVeyor. I think we
can allow up to the full 2.5 seconds without losing the valuable bits of
the test.


          Switch to using concurrent.futures

aa50a8c


          Force serial hook runs during tests

chriskuehl force-pushed the concurrent-execution branch from 9cd806c to 9125439 Compare

October 24, 2018 23:33

asottile approved these changes

Oct 30, 2018

View reviewed changes

asottile commented Oct 30, 2018

Copy link

Copy Markdown

Member

a few little things I want to touch up, then I'll merge. thanks for the implementation on this, this is great.

Are there any hooks that we should stick into the serial path? maybe flake8 since it also does multiprocessing? any others?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Minor cleanups

6bac405

asottile commented Nov 2, 2018

Copy link

Copy Markdown

Member

I'm excited and terrified for this feature 🎉

pushed my cleanups, will merge after CI finishes

chriskuehl reacted with hooray emoji

alexjurkiewicz reacted with heart emoji

All reactions

🎉 1 reaction
❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asottile merged commit 76e0f11 into pre-commit:master

Nov 2, 2018

chriskuehl deleted the concurrent-execution branch

November 2, 2018 01:52

alexjurkiewicz commented Nov 2, 2018

Copy link

Copy Markdown

nice work!!!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dunedan mentioned this pull request

Nov 7, 2018

Concurrent execution results in uneven work per thread #861

Closed

asottile commented Dec 20, 2018

Copy link

Copy Markdown

Member

This has been released as part of v1.13.0 🎉

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

chriskuehl mentioned this pull request

Aug 10, 2019

Upgrade pre-commit Yelp/paasta#2365

Merged

jiridanek mentioned this pull request

Oct 22, 2022

Failing pre-commit hook due to missing/unexpected files skupperproject/skupper-router#801

Closed

Reviewers

asottile

asottile approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

chriskuehl commented Oct 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriskuehl commented Oct 21, 2018

Uh oh!

asottile commented Oct 21, 2018

Uh oh!

asottile commented Oct 21, 2018

Uh oh!

chriskuehl commented Oct 22, 2018

Uh oh!

asottile commented Oct 30, 2018

Uh oh!

asottile commented Nov 2, 2018

Uh oh!

alexjurkiewicz commented Nov 2, 2018

Uh oh!

asottile commented Dec 20, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

chriskuehl commented Oct 20, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.